Enables LCM auto-upgrade - #97
Conversation
A separate phase for LCM upgrade is added as the first phase for LCM so that it gets upgraded before anything else. Signed-off-by: Dharmit Shah <dharmit.shah@suse.com>
| Expect(status.Message).To(Equal("All 2 LCM charts upgraded successfully (0 skipped)")) | ||
| }) | ||
|
|
||
| FIt("should not error even if the charts fail to upgrade", func() { |
There was a problem hiding this comment.
Created this for the purpose of discussion. In this test, CRD charts fail to upgrade, but that doesn't prevent LCM chart upgrades from getting triggered.
Do we want to keep this behaviour or break if the LCM CRD chart fails to upgrade?
Another question: do we want to return an error if either of the LCM CRD or LCM charts fail to upgrade so that the next phase doesn't get triggered? IIUC, that's not the case presently and we return a nil in error which leaves it upon the user to figure what went wrong with chart upgrades. I understand why this is OK in case of other Helm charts that are being upgraded towards the end but, for LCM charts, what behaviour do we think is more appropriate?
There was a problem hiding this comment.
Sharing my own thoughts. I personally feel we should not attempt to upgrade LCM chart if its CRDs fail to upgrade. And for the second one, assuming I understand things correctly, I feel we should stop the overall upgrade instead of letting OS upgrade phase get triggered.
There was a problem hiding this comment.
I have modified the PR to include the implementation for failure on LCM chart upgrade. It's in a separate commit to make removal easier if we decide not to fail the overall upgrade when one of these charts' upgrades fails.
There was a problem hiding this comment.
I fully agree, any failure in that state should stop the whole pipe and report the error IMHO.
d3bec23 to
521a4f3
Compare
davidcassany
left a comment
davidcassany
left a comment
There was a problem hiding this comment.
Looks good to me
Just a couple of minor comments which do not necessarily imply any requested change, they are just arguable and minor improvements.
| var workloadCharts []*upgrade.HelmChartConfig | ||
| for _, chartCfg := range chartConfigs { | ||
| name := chartCfg.Chart.GetName() | ||
| if !isLCMChart(name) { |
There was a problem hiding this comment.
nit: do we actually need to prevent LCMchart to be processed here? Up to my understanding it should not hurt attempting to upgrade it again, the upgrade if it is already at the desired version should do nothing.
If so, then we can probably keep the helm reconciler unmodified and if we ever set an upgrade pipe without LCMreconciler the helm reconciler would still run the full upgrade.
There was a problem hiding this comment.
I don't see any technical issue in removing isLCMChart check from here. Except it being a brain thing where it feels weird to keep it in spite of having a dedicated phase for it earlier in the flow.
if we ever set an upgrade pipe without LCMreconciler the helm reconciler would still run the full upgrade.
If we do this, it would be for the purpose of prevent LCM charts' upgrade, right? But removing isLCMChart check from Helm reconciler would still cause the LCM charts' upgrade. Just presenting a counter point.
I'm fine by removing the check from here if it makes more sense. Keeping this open for others' feedback/suggestions.
| case helm.ChartStateFailed: | ||
| failed++ | ||
| if failedChart == "" { | ||
| failedChart = result.chartName |
There was a problem hiding this comment.
nit: this is just keeping the last failed chart and the number of failures is not user any where. Probably you could simply directly return here with:
return &upgrade.PhaseStatus{
State: lifecyclev1alpha1.UpgradeFailed,
Message: fmt.Sprintf("Chart %s upgrade failed", failedChart),
}
without the need to count the number of failures and then check if it is bigger than zero.
There was a problem hiding this comment.
This function was copy-pasted from Helm reconciler. In LCM reconciler, I don't see a reason to have this check and keep a counter at all if we agree to failing the entire upgrade if any of the LCM charts fails to upgrade.
Signed-off-by: Dharmit Shah <dharmit.shah@suse.com>
2 participants
A separate phase for LCM upgrade is added as the first phase for LCM so that it gets upgraded before anything else.