fix sidecarset hot upgrade reset empty container bug #1054
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1054 +/- ##
==========================================
+ Coverage 49.34% 49.36% +0.02%
==========================================
Files 125 125
Lines 12355 12363 +8
==========================================
+ Hits 6096 6103 +7
+ Misses 5320 5319 -1
- Partials 939 941 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| sidecarSet := control.GetSidecarset() | ||
| if sk, err := sidecarcontrol.PatchPodMetadata(&pod.ObjectMeta, sidecarSet.Spec.PatchPodMetadata); err != nil { | ||
| klog.Errorf("sidecarSet(%s) update pod(%s/%s) metadata failed: %s", sidecarSet.Name, pod.Namespace, pod.Name, err.Error()) | ||
| return false, err |
There was a problem hiding this comment.
i suggest that we all use return statement with explicit return variables, it's more clear . and when patch pod meta data fails, it seems that we should return true, err
| admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Object: req.AdmissionRequest.OldObject}}, | ||
| oldPod); err != nil { | ||
| return false, err | ||
| return |
There was a problem hiding this comment.
it changes the return logic, it return false previously and now return true, plz confirm it is expected
There was a problem hiding this comment.
In fact, if error, skip will not take effect. I'll revert to the previous code
| if err := h.Client.List(ctx, sidecarsetList, utilclient.DisableDeepCopy); err != nil { | ||
| return false, err | ||
| if err = h.Client.List(ctx, sidecarsetList, utilclient.DisableDeepCopy); err != nil { | ||
| return |
There was a problem hiding this comment.
it changes the return logic, it return false previously and now return true, plz confirm it is expected
| } | ||
| } | ||
|
|
||
| klog.V(3).Infof("[sidecar inject] begin to operation(%s) pod(%s/%s) resources(%s) subResources(%s)", | ||
| req.Operation, req.Namespace, req.Name, req.Resource, req.SubResource) | ||
| // patch pod metadata, annotations & labels | ||
| for _, control := range matchedSidecarSets { |
There was a problem hiding this comment.
plz comment in the code why the code block should be move to this position
zmberg
force-pushed
the
fix-sidecarset-upgrade-bug
branch
from
8fb4255 to
dacc23f
Compare
Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com>
zmberg
force-pushed
the
fix-sidecarset-upgrade-bug
branch
from
dacc23f to
e1dbf76
Compare
|
/approve |
1 similar comment
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: furykerry, zmberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com> Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com> Signed-off-by: Liu Zhenwei <zwliu@thoughtworks.com>
Signed-off-by: liheng.zms liheng.zms@alibaba-inc.com
Ⅰ. Describe what this PR does