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

Bump default ScyllaDB version used in E2E's to 6.0.1 #1991

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

zimnx
Copy link
Collaborator

@zimnx zimnx commented Jun 24, 2024

Bumps default ScyllaDB version used in E2E's to 6.0.1, and align tests to 6.0 changes.

Scaling E2E was aligned to make sure it doesn't break minimal required quorum on scaling changes.
Existing test scaled below keyspace RF which is no longer possible, as Scylla rejects decommision when there's a keyspace having RF higher than node count.
Test step checking decommission of drained node was moved earlier to fix the same quorum breakage.

Alternator E2E required a table name change from which we are gettingpassword. Table name was renamed in 6.0.

Restore E2E was parametrized to make sure we test the procedure for both default ScyllaDB version and 2024.1 where a workaround explained in the documentation is required.

Fixes #1954

@zimnx zimnx added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jun 24, 2024
Copy link
Contributor

@zimnx: GitHub didn't allow me to request PR reviews from the following users: zimnx.

Note that only scylladb members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Bumps default ScyllaDB version used in E2E's to 6.0.1.

Restore E2E which fails due to missing support for ScyllaDB 6.0.x in Scylla Manager is skipped until fix is available.

/cc

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@scylla-operator-bot scylla-operator-bot bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 24, 2024
@zimnx zimnx force-pushed the bump-6.0.0 branch 2 times, most recently from 0023aa6 to f289f83 Compare June 24, 2024 17:18
Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2024
@zimnx zimnx changed the title Bump default ScyllaDB to 6.0.1 in E2Es [WIP] Bump default ScyllaDB to 6.0.1 in E2Es Jun 25, 2024
@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2024
@zimnx
Copy link
Collaborator Author

zimnx commented Jun 25, 2024

New issue with our tests:
scylladb/scylladb#16677 (comment)

Do we now require the nr of nodes in the cluster must bigger than the RF of any tablet tables?

Yes, this is working as expected.

Our tests doesn't lower RF upon scaling down.

@tnozicka
Copy link
Member

Do we now require the nr of nodes in the cluster must bigger than the RF of any tablet tables?

I am fine bumping the default replicas to maintain the RF where needed.

Our tests doesn't lower RF upon scaling down.

I am not sure what you mean by that? Is there something else needed then just raising the default replicas?

@zimnx
Copy link
Collaborator Author

zimnx commented Jun 25, 2024

I am not sure what you mean by that? Is there something else needed then just raising the default replicas?

It's not matter of default replicas. In scaling test where we scaled cluster from 3->2->1->3 we didn't lower RF of keyspaces. It was a violation which started to be validated within Scylla.

@tnozicka
Copy link
Member

It's not matter of default replicas. In scaling test where we scaled cluster from 3->2->1->3 we didn't lower RF of keyspaces. It was a violation which started to be validated within Scylla.

guess my point was to go say 5->4->3->4->5 with RF=3

@zimnx
Copy link
Collaborator Author

zimnx commented Jun 25, 2024

It's not matter of default replicas. In scaling test where we scaled cluster from 3->2->1->3 we didn't lower RF of keyspaces. It was a violation which started to be validated within Scylla.

guess my point was to go say 5->4->3->4->5 with RF=3

This would lower case coverage compared to scenario with rf change and scaling down. But it's not what we have now, we can always add it. I'll amend node count then.

@scylla-operator-bot scylla-operator-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 25, 2024
@tnozicka
Copy link
Member

tnozicka commented Jun 25, 2024

This would lower case coverage compared to scenario with rf change and scaling down. But it's not what we have now, we can always add it. I'll amend node count then.

Got it, I find the case you describe valuable as well. Please make an issue and send a PR to cover RF change.

@zimnx
Copy link
Collaborator Author

zimnx commented Jun 26, 2024

can you try with https://github.com/scylladb/scylla-manager/releases/tag/3.3.0-0.20240624.9dd98ab35 ?

It went well. I asked Manager team about next release, it should be out very soon.

I'll unwip this PR once it's available.

@zimnx zimnx changed the title [WIP] Bump default ScyllaDB to 6.0.1 in E2Es Bump default ScyllaDB version used in E2E's to 6.0.1 Jun 28, 2024
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2024
@zimnx
Copy link
Collaborator Author

zimnx commented Jun 28, 2024

Manager was released and bumped on master so this is no longer WIP.

@rzetelskik
Copy link
Member

rzetelskik commented Jun 28, 2024

Manager was released and bumped on master so this is no longer WIP.

fwiw manager client might need a bump as well

@zimnx
Copy link
Collaborator Author

zimnx commented Jun 28, 2024

Flake - #1525 (comment)
/retest

Scaling E2E was aligned to make sure it doesn't break minimal required quorum on scaling changes.
Existing test scaled below keyspace RF which is no longer possible, as Scylla rejects decommision when there's a keyspace having RF higher than node count.
Test step checking decommission of drained node was moved earlier to fix the same quorum breakage.

Alternator E2E required a table name change from which we are getting
password. Table name was renamed in 6.0.

Restore E2E was parametrized to make sure we test the procedure for both default ScyllaDB version and 2024.1 where a workaround explained in the documentation is required.
@zimnx
Copy link
Collaborator Author

zimnx commented Jun 28, 2024

fwiw manager client might need a bump as well

API wasn't changed, so it's not required.

@rzetelskik rzetelskik self-requested a review June 28, 2024 16:40
Copy link
Member

@rzetelskik rzetelskik left a comment

Choose a reason for hiding this comment

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

lgtm
/assign tnozicka

@zimnx
Copy link
Collaborator Author

zimnx commented Jun 28, 2024

Possible flake - #1996

/retest

Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

/approve

Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rzetelskik, tnozicka, zimnx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tnozicka tnozicka added the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2024
@zimnx
Copy link
Collaborator Author

zimnx commented Jul 2, 2024

/hold cancel

@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2024
@scylla-operator-bot scylla-operator-bot bot merged commit 670bf62 into scylladb:master Jul 2, 2024
12 checks passed
@tnozicka tnozicka added kind/machinery Categorizes issue or PR as related to Makefile, scripts or similar changes. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/machinery Categorizes issue or PR as related to Makefile, scripts or similar changes. lgtm Indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ScyllaDB version in e2e to 6.0.x
3 participants