-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add _on_upgrade #216
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at the comment regarding setting unit status. Apart from that, the changes are looking good in a way that they implement the desired strategy. I think that what is missing now is to review the upgrade strategy and for that I would like more people adding their review
maas-agent/src/charm.py
Outdated
if current := MaasHelper.get_installed_channel(): | ||
if current > MAAS_SNAP_CHANNEL: | ||
msg = f"Cannot downgrade {current} to {MAAS_SNAP_CHANNEL}" | ||
# XXX: It looks like ErrorStatus is not valid in the version of Juju used by maas-charms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the version of Juju or in the version of ops
library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is something with version of Juju, then the charm itself is not related. It is rather a maas-anvil issue that is using a Juju not supporting error status. Right?
maas-agent/src/helper.py
Outdated
@@ -16,6 +18,25 @@ | |||
MAAS_SERVICE = "pebble" | |||
|
|||
|
|||
def _join_cohort_(maas: Snap) -> str: | |||
"""Join the maas snap cohort if not part of it already. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Join the maas snap cohort if not part of it already. | |
"""Join the MAAS snap cohort if not part of it already. |
maas-agent/src/helper.py
Outdated
"""Join the maas snap cohort if not part of it already. | ||
|
||
Args: | ||
maas (Snap): Instance of maas snap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maas (Snap): Instance of maas snap. | |
maas (Snap): Instance of MAAS snap. |
maas-region/src/charm.py
Outdated
if current := MaasHelper.get_installed_channel(): | ||
if current > MAAS_SNAP_CHANNEL: | ||
msg = f"Cannot downgrade {current} to {MAAS_SNAP_CHANNEL}" | ||
# XXX: It looks like ErrorStatus is not valid in the version of Juju used by maas-charms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the comment above.
maas-region/src/helper.py
Outdated
@@ -16,6 +18,25 @@ | |||
MAAS_SERVICE = "pebble" | |||
|
|||
|
|||
def _join_cohort_(maas: Snap) -> str: | |||
"""Join the maas snap cohort if not part of it already. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Join the maas snap cohort if not part of it already. | |
"""Join the MAAS snap cohort if not part of it already. |
maas-region/src/helper.py
Outdated
"""Join the maas snap cohort if not part of it already. | ||
|
||
Args: | ||
maas (Snap): Instance of maas snap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maas (Snap): Instance of maas snap. | |
maas (Snap): Instance of MAAS snap. |
maas-agent/src/helper.py
Outdated
maas._cohort = cohort | ||
return cohort | ||
|
||
if _cohort := re.match(r"cohort-key:\s+([^\n]+)", maas._snap("create-cohort")): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is something that maas-region
should set, the juju leader should put it in the peer relation databag, and the other units should consume from it. In addition, maas-agent should receive it through the relationship. But I would like @alexsander-souza to provide his feedback on this
Adds the _on_upgrade method to hopefully resolve #214, allowing the charm to install the correct MAAS snap version when an upgrade is detected