Add in-place upgrades #88
Conversation
carlcsaposs-canonical
force-pushed
the
upgrade
branch
4 times, most recently
from
4cf6375 to
9a95a17
Compare
carlcsaposs-canonical
requested review from
paulomach,
taurus-forever and
shayancanonical
| if self._upgrade.in_progress: | ||
| logger.warning( | ||
| "Modifying relations during an upgrade is not supported. The charm may be in a broken, unrecoverable state. Re-deploy the charm" | ||
| ) |
There was a problem hiding this comment.
should we bubble this message up to the charm status?
There was a problem hiding this comment.
It's not a status that can be acted upon (unless we set blocked and stop the charm from executing)—so that's why I used a log
Also in most upgrades it shouldn't be an issue—I added a log so that if we get a bug report in one of the cases where it will be an issue that we don't waste time debugging a non-supported usage
carlcsaposs-canonical
force-pushed
the
upgrade
branch
from
8c11d5b to
cacf3b5
Compare
| # installation/setup | ||
| self._upgrade.set_versions_in_app_databag() | ||
|
|
||
| def reconcile(self, event=None) -> None: # noqa: C901 |
There was a problem hiding this comment.
Can you break this function (instead of noqa), it's hard to follow when you are not constantly looking at the code.
Maybe split into a function the first block that's evaluating the upgrade states
or move VM/k8s specific tests to the concrete class, where feasible
There was a problem hiding this comment.
IMO it's easier to read in a single function (since all of this logic is fairly tightly coupled—so any splitting of it would hide the coupling and make explicit assumptions implicit)
I personally think the noqa is fine because ops is forcing this pattern—IMO the "reconcile" function should just be top-level code in charm.py (and a linter wouldn't mind that), but it has to go into a function because of ops—hence the noqa for just this function
|
|
||
| def reconcile(self, event=None) -> None: # noqa: C901 | ||
| """Handle most events.""" | ||
| if not self._upgrade: |
There was a problem hiding this comment.
I see why return on lacking self._upgrade but had to check implementation. Can you add a comment?
There was a problem hiding this comment.
what do you want me to add a comment about? I think I'm missing what is unclear
There was a problem hiding this comment.
It's not clear that not self._upgrade is related to the peer upgrade relation not existing, as per the log message.
There was a problem hiding this comment.
imo the log message is enough—no need to duplicate the log message in a comment
lmk if you disagree and what comment you want me to add
There was a problem hiding this comment.
something like
| if not self._upgrade: | |
| if not self._upgrade: | |
| # upgrade property is not set while peer upgrade relation not available |
|
|
||
| def reconcile(self, event=None) -> None: # noqa: C901 | ||
| """Handle most events.""" | ||
| if not self._upgrade: |
There was a problem hiding this comment.
It's not clear that not self._upgrade is related to the peer upgrade relation not existing, as per the log message.
|
@carlcsaposs-canonical todo: sync upgrade.py with k8s (charm versioning with git hash compatibility check) |
There was a problem hiding this comment.
This stuck for the unpolite amount of time as we have been focused on PostgreSQL customers launches. It is a time to share some love with MySQL Router VM as well and move from here.
I have made several quick tests and was not able to reproduce issues I was in Autumn... I will continue testing in edge, but it is a time to merge this to unblock COS merges.
Carl, thank you for the great work here and my apologies to long queued merge here! Let's bring M-R-VM to K8s quality level!
| if self._upgrade.unit_state == "restarting": # Kubernetes only | ||
| if not self._upgrade.is_compatible: | ||
| logger.info( | ||
| "Upgrade incompatible. If you accept potential *data loss* and *downtime*, you can continue with `resume-upgrade force=true`" |
There was a problem hiding this comment.
What is the difference in resume-upgrade force=true and force-upgrade action?
There was a problem hiding this comment.
resume-upgrade force=true is k8s-only
force-upgrade is vm-only
this was done to make implementation more robust & significantly simpler (tried to keep consistent UX as much as possible, but this difference is a reflection of how the vm/k8s upgrade process is significantly different)
carlcsaposs-canonical
force-pushed
the
upgrade
branch
from
a967e96 to
d1894b6
Compare
Ported from canonical/mysql-router-operator#88 upgrade.py, machine_upgrade.py and event handlers in charm.py are mostly shared with upgrade.py, machine_upgrade.py and event handlers in abstract_charm.py/machine_charm.py in MySQL Router VM charm Non-upgrade event handler guards implemented per https://docs.google.com/spreadsheets/d/1_qjINUoj5Mi7PlXhCdpwQ0170bIZSgpvNFMAnTOtuQw/edit#gid=0 --------- Co-authored-by: phvalguima <[email protected]>
Derived from canonical/mysql-router-k8s-operator#138