Skip to content
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

[DPE-5711] Add warnings to destructive actions #336

Merged
merged 4 commits into from
Dec 9, 2024

Conversation

sinclert-canonical
Copy link
Contributor

This PR adds warning level messages when a destructive action is performed. The definition of destructive seems to be associated with data loss, which I think it can only happen, from user action, whenever they force a primary failover without checking considering the health of such instance.

Additional info

  • Paulo confirmed that this ticket was created to familiarised myself with the codebases.
  • Alex referenced this PostgreSQL operator PR as the reason behind the need for warnings.

Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain the benefits of merging this? Warning for force action?

Can we warn it globally in a moment of setting force=True?

Up 2 Paulo and Carl.

@sinclert-canonical
Copy link
Contributor Author

Can you please explain the benefits of merging this? Warning for force action?

I think that the upgrade action will not execute the upgrade if the unit is considered unhealthy (whatever that means). This action could be forced by using the --force flag, which, may lead to data-loss.

Can we warn it globally in a moment of setting force=True?

We could put it in the charm's _on_resume_upgrade_action method (only one that calls the Upgrade.reconcile_upgrade method with an event), but we would be raising warnings even if the upgrade is not actually executed. Do we want that?

Copy link
Collaborator

@paulomach paulomach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take on this is that, having the differentiation can be useful for a post morten check if things go bad and logs where not in debug level.

@sinclert-canonical
Copy link
Contributor Author

Added info messages to Workload._diable_router method.

src/kubernetes_upgrade.py Outdated Show resolved Hide resolved
src/workload.py Show resolved Hide resolved
@@ -222,6 +222,7 @@ def determine_partition() -> int:
action_event.fail(message)
return
if force:
logger.warning(f"Resume upgrade action ran with {force=}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.warning(f"Resume upgrade action ran with {force=}")
logger.warning(f"Resume upgrade action ran with force=True")

nit (#336 (comment))

technically end result is the same, but think we should avoid using repr syntax (e.g. for type str) when the output is intended for the user instead of developer, since repr syntax does not line up with juju cli syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see many usages of this syntax throughout the repo (see GitHub search). Keeping it for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those usages are when the output is intended for developer, and to show the developer what the value of a python variable is

those usages are not for displaying what the user inputs of a juju action were

@sinclert-canonical sinclert-canonical merged commit cb486eb into main Dec 9, 2024
6 of 9 checks passed
@sinclert-canonical sinclert-canonical deleted the sinclert/force-upgrade-warnings branch December 9, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants