🐛 update mca progressing condition when desired and last applied matches #513
Conversation
|
/hold |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #513 +/- ##
==========================================
+ Coverage 62.08% 62.11% +0.02%
==========================================
Files 141 141
Lines 11740 11744 +4
==========================================
+ Hits 7289 7295 +6
+ Misses 3678 3677 -1
+ Partials 773 772 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
haoqing0110
force-pushed
the
br_rm-mca-skip
branch
2 times, most recently
from
c703e9d to
02adb45
Compare
|
/assign @qiujian16 @xuezhaojun |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haoqing0110, qiujian16 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 |
| @@ -285,18 +285,6 @@ func workIsReady(work *workapiv1.ManifestWork) bool { | |||
|
|
|||
| // set addon progressing condition and last applied | |||
| func setAddOnProgressingAndLastApplied(reason string, message string, addon *addonapiv1alpha1.ManagedClusterAddOn) { | |||
| // always update progressing condition when there is no config | |||
There was a problem hiding this comment.
why do we need to remove this skip?
There was a problem hiding this comment.
Meet an issue: when addon applied configs successfully and then configs become unaccessible (eg, removed), the desired spec hash will be empty and report "mca and work configs mismatch", and when the configs added back, since lastapplied already the same as desired, so the error message will leave in the condition and can never trigger an update.
2 reasons feel safe to remove the skip:
- In the previous code, add a skip to avoid "install" message changes to "upgrade" after a fresh install. With PR https://github.com/open-cluster-management-io/ocm/pull/499/files merged, won't have such issue.
- Another reason is once the mca applied config successfully (lastapplied = desired), do not want to change the "Progressing" condition any more, to avoid back and forth in rollout. Feel safe to remove the skip because the setRolloutStatus only look at the lastapplied and desired, not message.
There was a problem hiding this comment.
could we add some comments here?
haoqing0110
force-pushed
the
br_rm-mca-skip
branch
from
02adb45 to
ecfb572
Compare
haoqing0110
force-pushed
the
br_rm-mca-skip
branch
from
ecfb572 to
244ac3d
Compare
Signed-off-by: haoqing0110 <qhao@redhat.com>
haoqing0110
force-pushed
the
br_rm-mca-skip
branch
from
244ac3d to
1deac6b
Compare
|
/lgtm |
|
/unhold |
openshift-merge-bot
bot
merged commit 462992f
into
open-cluster-management-io:main
open-cluster-management-io#513) Signed-off-by: haoqing0110 <qhao@redhat.com>
Summary
Related issue(s)
Fixes #