Node Role - Forbid Scylla upgrade when not explicitly requested #140
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit introduces an early check to determine whether Scylla is already installed on the target system. It provides a mechanism to prevent incorrectly (and dangerously) upgrading a system which may be already part of a running cluster. In such scenarios, we now require `upgrade_version: True`, which delegates ALL upgrade logic to its relevant upgrade section in the role.
A funny situation arose when we introduced the option to fail early when Scylla is already installed and upgrade_version == False: It may happen that the role failed during its runtime, therefore leaving the target systems in an inconsistent state. When this happens, if the failure happened AFTER the Scylla package was installed, then the user would be left in an chicken-and-egg situation: They can not re-execute the playbook installing the specified version, as the upgrade will abort since we are upgrading to the same release. Similarly, they can not install Scylla again, as it will complain that the package is already installed. We circumvent that situation by introducing the `skip_upgrade_check` option, which is a failsafe mechanism to allow a role failure to proceed on installation even when `upgrade_version` is False. Needless to say, this option is False by default.
|
For some reason I can't add a reviewer -- so @vladzcloudius :-) |
vladzcloudius
requested changes
Aug 25, 2022
There was a problem hiding this comment.
We can't merge it because we use a Role's state is not only scylla packages but rather a lot more: swap configuration, systemd configuration, additional packages and lot-lot more.
And sometimes we run a Role on a node/nodes with already installed Scylla in order to apply some changes to that state, e.g. to change io_properties.yml values.
Therefore the new logic this PR suggests is no-go.
What DO need to prevent unintentional upgrades and this IS the case today AFAIK.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series introduces an early check to determine whether Scylla is already installed on the target system.
It provides a mechanism to prevent incorrectly (and dangerously) upgrading a system which may be already part of a running cluster.
In such scenarios, we now require
upgrade_version: True, which delegates ALL upgrade logic to its relevant upgrade section in the role.To prevent a "chicken and egg" situation when we fail in the middle of a role execution AND after Scylla has been installed, we introduce the
skip_upgrade_checkoption as a failsafe mechanism, which - obviously - defaults to false.Tests ran:
skip_upgrade_check: Trueoption, which allows us to get through and proceed with the installation/configuration processskip_upgrade_check: Trueoption, we see a failure complaining that Scylla is already installed.upgrade_version: True&&upgrade_major: Falseoptions, upgrade completes.Fixes #132