Bugfix: sync pod /status correctly in kwok-based simulator cluster #434
Conversation
k8s-ci-robot
added
kind/bug
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Park-Jiyeonn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @Park-Jiyeonn. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
k8s-ci-robot
added
the
ok-to-test
k8s-ci-robot
removed
the
needs-ok-to-test
|
@sanposhiho: GitHub didn't allow me to request PR reviews from the following users: saza-ku, ordovicia. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Sorry for being late. I'll review your PRs this weekend. |
| func (s *Service) updateFunc(oldObj, newObj interface{}) { | ||
| ctx := context.Background() | ||
| unstructObj, ok := newObj.(*unstructured.Unstructured) | ||
| newUnstructuredObj, ok := newObj.(*unstructured.Unstructured) | ||
| if !ok { | ||
| klog.Error("Failed to convert runtime.Object to *unstructured.Unstructured") | ||
| return | ||
| } | ||
|
|
||
| err := s.resourceApplierService.Update(ctx, unstructObj) | ||
| oldUnstructuredObj, ok := oldObj.(*unstructured.Unstructured) | ||
| if !ok { | ||
| klog.Error("Failed to convert runtime.Object to *unstructured.Unstructured") | ||
| return | ||
| } | ||
|
|
||
| if newUnstructuredObj.GetKind() == "Pod" && !podStatusEqual(oldUnstructuredObj, newUnstructuredObj) { | ||
| if patchErr := s.resourceApplierService.PatchPodStatus(ctx, newUnstructuredObj); patchErr != nil { | ||
| klog.Errorf("Failed to patch pod status: %v", patchErr) | ||
| } | ||
| } | ||
|
|
||
| err := s.resourceApplierService.Update(ctx, newUnstructuredObj) |
There was a problem hiding this comment.
The logic of applying resource of real clusters is encapsulated in resourceapplier.
And this fix should also be applied to oneshotimporter and recorder.
So we should confine this fix to resourceapplier.
We could use defer to bypass the filters in updateFunc.
There was a problem hiding this comment.
Thank you, it's good suggestion. Let me make some modifications.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes an issue where Pod status is not synced correctly when using a kwok-based cluster as the simulator target. This PR ensures that the
/statussubresource is patched during Pod sync, including terminal phases likeSucceededandFailed.Which issue(s) this PR fixes:
Fixes #429
Special notes for your reviewer:
/label tide/merge-method-squash