search

scylladb / scylla-operator

The Kubernetes Operator for ScyllaDB
https://operator.docs.scylladb.com/
Apache License 2.0
332 stars 162 forks source link

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

Closed zimnx closed 3 months ago

zimnx commented 3 months ago

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

scylla-operator-bot[bot] commented 3 months ago

@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](https://github.com/scylladb/scylla-operator/pull/1991): >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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
zimnx commented 3 months ago

New issue with our tests: https://github.com/scylladb/scylladb/issues/16677#issuecomment-1894071518

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

tnozicka commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

rzetelskik commented 3 months ago

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

fwiw manager client might need a bump as well

zimnx commented 3 months ago

Flake - https://github.com/scylladb/scylla-operator/issues/1525#issuecomment-2197238093 /retest

zimnx commented 3 months ago

fwiw manager client might need a bump as well

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

zimnx commented 3 months ago

Possible flake - https://github.com/scylladb/scylla-operator/issues/1996

/retest

scylla-operator-bot[bot] commented 3 months ago

[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: - ~~[OWNERS](https://github.com/scylladb/scylla-operator/blob/master/OWNERS)~~ [tnozicka,zimnx] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
zimnx commented 3 months ago

/hold cancel