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

fix(upgrade): upgrade with raft topology procedure #7295

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aleksbykov
Copy link
Contributor

@aleksbykov aleksbykov commented Mar 25, 2024

After upgrade to latest master(6.0) raft topology feature or
tablets + raft topology features will be enabled by default
To switch cluster from gossiper to raft topology, raft topology
procedure should be executed. It is described here:
https://github.com/scylladb/scylladb/blob/c5601a749e21fc710958a7c84316ecdf5943022c/docs/dev/topology-over-raft.md
section: Upgrade from legacy topology to raft-based topology

parameter 'enable_tablets_on_upgradeis used to control upgrade with/without tablets. By default tablets are enabled with scylla parameterenable_tablets` and it is added to scylla.yaml starting from 6.0. Upon upgrade this parameter should be set explicitly to true to enable tablets or set to false or not be added to scylla.yaml so tablets stay disabled after upgrade

raft topology is enabled by default, and could be disabled only with parameter enforce_gossip_topology_changes. if this parameter is added to scylla.yaml before upgrade, then raft feature: consistent topology changes will not be enabled

Testing

run upgrade from 5.4 -> 6.0.

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

Reminders

  • Add New configuration option and document them (in sdcm/sct_config.py)
  • Add unit tests to cover my changes (under unit-test/ folder)
  • Update the Readme/doc folder relevent to this change (if needed)

@aleksbykov aleksbykov force-pushed the raft-topology-exp-feature-upgrade branch 2 times, most recently from eec8251 to f80b512 Compare March 27, 2024 07:54
@aleksbykov aleksbykov force-pushed the raft-topology-exp-feature-upgrade branch from f80b512 to e8d533a Compare April 15, 2024 09:14
@aleksbykov aleksbykov added the backport/none Backport is not required label Apr 15, 2024
@aleksbykov aleksbykov marked this pull request as ready for review April 15, 2024 09:19
sdcm/rest/raft_upgrade_procedure.py Outdated Show resolved Hide resolved
upgrade_test.py Outdated Show resolved Hide resolved
sdcm/sct_config.py Outdated Show resolved Hide resolved
sdcm/rest/raft_upgrade_procedure.py Show resolved Hide resolved
upgrade_test.py Outdated Show resolved Hide resolved
@temichus
Copy link
Contributor

blocked by scylladb/scylladb#18098

@kbr-scylla
Copy link

I'm unassigning myself -- it is blocked by scylladb/scylladb#18098 but that issue does not "belong" to me

@kbr-scylla kbr-scylla removed their assignment May 16, 2024
@mykaul
Copy link
Contributor

mykaul commented May 27, 2024

blocked by scylladb/scylladb#18098

Is this still the case?

@kbr-scylla
Copy link

Should be no longer -- @aleksbykov please retest

(on nightly build -- the fix is not backported to 6.0 yet)

@aleksbykov aleksbykov force-pushed the raft-topology-exp-feature-upgrade branch 5 times, most recently from 90b5607 to 8b75680 Compare June 9, 2024 09:37
After upgrade to latest master(6.0) raft topology feature or
tablets + raft topology features will be enabled by default
To switch cluster from gossiper to raft topology, raft topology
procedure should be executed. It is described in scylla doc
upgrade from 5.4-> 6.0
section: Upgrade from legacy topology to raft-based topology
@aleksbykov aleksbykov force-pushed the raft-topology-exp-feature-upgrade branch from 8b75680 to e5f6f03 Compare June 9, 2024 09:51
@aleksbykov aleksbykov added backport/6.0 Ready for review and removed backport/none Backport is not required labels Jun 9, 2024
@temichus temichus requested a review from soyacz June 9, 2024 13:53
Copy link
Contributor

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

Is it right to put it as default and not providing tests with false?
Especially, that false path is current default for enterprise (IIRC it was not decided yet to turn on tablets by default in enterprise).

scylla_yaml_updates.update({"experimental_features": ["tablets", "consistent-topology-changes"]})
scylla_yaml_updates.update({"enable_tablets": True})

if self.params.get("enable_force_`gossip_topology_changes"):
Copy link
Contributor

Choose a reason for hiding this comment

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

typo here in param - so it looks this path was never tested

@@ -339,6 +339,8 @@ def set_authorizer(cls, authorizer: str):
audit_categories: str = None # None
audit_tables: str = None # None
audit_keyspaces: str = None # None
force_gossip_topology_changes: bool = None # False
enable_tablets: bool = None # False
Copy link
Contributor

Choose a reason for hiding this comment

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

bit misleading comment - 6.0.0 comes with this set explicitly to True - I think worth to add this info (like we do for e.g. rpc_port).

@@ -1550,6 +1550,10 @@ class SCTConfiguration(dict):

dict(name="kafka_connectors", env="SCT_KAFKA_CONNECTORS", type=str_or_list_or_eval,
help="configuration for setup up kafka connectors"),

dict(name="enable_force_gossip_topology_changes", env="SCT_ENABLE_FORCE_GOSSIP_TOPOLOGY_CHANGES", type=boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

this param is looking like global one while it is only used in upgrade tests - misleading.

Btw. Can't we just add force_gossip_topology_changes to append_scylla_yaml? Now when it is used as dict and merged with other append_scylla_yaml params, maybe we don't need this one?

@@ -250,3 +250,5 @@ teardown_validators:

kafka_backend: null
kafka_connectors: []

enable_force_gossip_topology_changes: false
Copy link
Contributor

Choose a reason for hiding this comment

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

why enable_force_gossip_topology_changes and not force_gossip_topology_changes?

@@ -1550,6 +1550,10 @@ class SCTConfiguration(dict):

dict(name="kafka_connectors", env="SCT_KAFKA_CONNECTORS", type=str_or_list_or_eval,
help="configuration for setup up kafka connectors"),

dict(name="enable_force_gossip_topology_changes", env="SCT_ENABLE_FORCE_GOSSIP_TOPOLOGY_CHANGES", type=boolean,
help="""Enable gossip topology changes (disable raft topology)"""),
Copy link
Contributor

Choose a reason for hiding this comment

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

--> """Force gossip topology changes (disable raft topology)""" ?

scylla_yaml_updates.update({"enable_tablets": True})

if self.params.get("enable_force_`gossip_topology_changes"):
scylla_yaml_updates.update({"force_gossip_topology_changes": True, "enable_tablets": False})
Copy link
Contributor

Choose a reason for hiding this comment

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

enable_tablets is "false" by default of binary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants