refactor: unify k8s+vm upgrade pattern #83
Conversation
marcoppenheimer
requested review from
marceloneppel and
paulomach
and removed request for
marceloneppel
marcoppenheimer
changed the title
| if self.substrate == "k8s": | ||
| return "safe" | ||
|
|
||
| return self.peer_relation.data[self.charm.app].get("resume-strategy", "auto") |
There was a problem hiding this comment.
I think we can allow users to also select the auto resume strategy on k8s.
|
|
||
| # if unit top of stack and all units ready (i.e stack), emit granted event | ||
| # only runs on VM, as K8s units will be 'idle' due to not setting 'ready' in upgrade-charm |
There was a problem hiding this comment.
L1081? Or do you mean something like as not upgraded k8s units will be 'idle' due to upgrade-charm event have not being fired yet?
| # only runs on VM, as K8s units will be 'idle' due to not setting 'ready' in upgrade-charm | |
| # only runs on VM, as K8s units will be 'upgrading' due to not setting 'ready' in upgrade-charm |
| logger.info( | ||
| f"Please evaluate unit {self.charm.app}/{self.upgrade_stack[-1]}, and run `resume-upgrade` action to continue upgrade" | ||
| ) | ||
| self.charm.unit.status = BlockedStatus("awaiting resume-upgrade action...") |
| if not self.peer_relation or not self.upgrade_stack: | ||
| return True | ||
|
|
||
| return len(self.upgrade_stack) == len(self.peer_relation.units) |
There was a problem hiding this comment.
The following change is needed as the upgrade stack has not changed when the resume upgrade action is called on k8s.
| return len(self.upgrade_stack) == len(self.peer_relation.units) | |
| return len(self.upgrade_stack) == len(self.peer_relation.units) + 1 |
However, it makes the upgrade process not respect the resume strategy (making it not wait for the call the the resume-upgrade action), so I think we need different comparisons for those two cases.
| @@ -1056,32 +1070,24 @@ def _on_upgrade_charm(self, event: UpgradeCharmEvent) -> None: | |||
| logger.error("Cluster upgrade failed, ensure pre-upgrade checks are ran first.") | |||
| return | |||
|
|
|||
| # for VM, run version checks on leader only | |||
| # for K8s, run version checks on first unit only | |||
| if (self.charm.unit.is_leader() and self.substrate == "vm") or ( | |||
There was a problem hiding this comment.
on VM does it really matters where the upgrade_supported_check is done, since every unit will have a new charm code.
I think we could have same approach for both, i.e. check on the first
There was a problem hiding this comment.
You're probably right now that I think about it. For VM, the unit that does the check is the last unit to mark itself as 'ready'. I suppose it could be the first!
|
Closing as no longer relevant. |
TODO
Changes Made
refactor: remove upgrade-finished event + handler, use relation-changedset_unit_completed. Needs testingfeat: add resume-strategy to pre-upgrade-checkauto- all units upgrade automaticallysafe- upgrade pauses on first unitrefactor: add unified _update_stack()resume-upgradeaction, and if completed top of stack + first unit + 'safe'