Run node cleanup after scaling the ScyllaCluster #1294

zimnx · 2023-07-03T16:29:29Z

Running node cleanup after scaling a cluster allows to avoid the
accumulation of unnecessary data on the node disks. When nodes are added
or removed from the cluster, they gain or lose some tokens, which can
result in files stored on the node disks still containing data
associated with lost tokens. Over time, this can lead to a build-up of
unnecessary data and cause disk space issues. By running node cleanup
after scaling, these files can be cleared, freeing up disk space.

Scylla Operator was extended with controllers responsible for executing
a cleanup on nodes after horizontally scaling.

Fixes #1317

tzach · 2023-07-03T18:02:49Z

@zimnx to validate the logic:
If a cluster scales from X to 2X nodes, for example, how many times is cleanup executed, X times (on for each new node) or once?

zimnx · 2023-07-04T07:05:26Z

If a cluster scales from X to 2X nodes, for example, how many times is cleanup executed, X times (on for each new node) or once?

It's executed once on every node except last one that joined, only when Operator is done with scaling.
So if you have N nodes, and you scale to 2*N, Operator will add N new nodes first, and only then execute 2*N-1 node cleanups, ~~one node at a time.~~

Edit:
I changed code to run cleanup Jobs in parallel as ScyllaDB core team member pointed out that cleanup no longer influence user traffic that much and it's fine to remove this limitation.

rzetelskik

Left a few comments. Very nice job overall!

pkg/cmd/operator/cleanupjob.go

pkg/controller/sidecar/sync.go

test/e2e/set/scyllacluster/scyllacluster_cleanup.go

pkg/controller/sidecar/sync.go

rzetelskik · 2023-07-05T21:39:53Z

pkg/controller/scyllacluster/resource_test.go

@@ -433,13 +434,13 @@ func TestStatefulSetForRack(t *testing.T) {
 						Kind:               "ScyllaCluster",
 						Name:               "basic",
 						UID:                "the-uid",
-						Controller:         pointer.Bool(true),
-						BlockOwnerDeletion: pointer.Bool(true),
+						Controller:         pointer.Ptr(true),


Shouldn't these changes be in a separate commit? They're not directly related to this feature.

I assume they could technically even ship separately (but the bar is lower until we get a new CI)

pkg/controller/sidecar/sync.go

test/e2e/set/scyllacluster/scyllacluster_cleanup.go

test/e2e/utils/observer.go

test/e2e/set/scyllacluster/scyllacluster_cleanup.go

rzetelskik

Thanks, lgtm now

tnozicka

scanned through the important parts, got a few question before I go through the rest, thanks!

pkg/cmd/operator/cleanupjob.go

pkg/controller/nodeconfigpod/sync_configmaps.go

pkg/controller/sidecar/sync.go

pkg/controller/scyllacluster/resource.go

tnozicka

round 2

pkg/controller/scyllacluster/resource.go

pkg/controller/scyllacluster/sync_jobs.go

tnozicka · 2023-07-27T15:10:28Z

pkg/controller/scyllacluster/sync_jobs.go

+		progressingMessages = append(progressingMessages, fmt.Sprintf("Waiting for ScyllaCluster %q to rollout.", naming.ObjRef(sc)))
+	}
+
+	if len(progressingMessages) != 0 {


note: being discussed on the proposal - #1300 (comment)

pkg/controller/scyllacluster/resource.go

pkg/cmd/operator/cleanupjob.go

pkg/controller/scyllacluster/resource_test.go

pkg/controller/sidecar/sync.go

pkg/controllerhelpers/scylla.go

pkg/naming/constants.go

pkg/scyllaclient/client.go

test/e2e/set/scyllacluster/scyllacluster_cleanup.go

mykaul · 2023-08-08T13:20:19Z

What's the latest with this PR? I see most / all conversations marked as 'outdated' - can we resolve those that were and move forward?

zimnx · 2023-08-08T13:35:35Z

What's the latest with this PR? I see most / all conversations marked as 'outdated' - can we resolve those that were and move forward?

It's waiting for @tnozicka feedback about review remarks resolution and approval.

tnozicka

thanks for the updates

/lgtm

tnozicka

/lgtm

Running node cleanup after scaling a cluster allows to avoid the accumulation of unnecessary data on the node disks. When nodes are added or removed from the cluster, they gain or lose some tokens, which can result in files stored on the node disks still containing data associated with lost tokens. Over time, this can lead to a build-up of unnecessary data and cause disk space issues. By running node cleanup after scaling, these files can be cleared, freeing up disk space. Scylla Operator was extended with controllers responsible for executing a cleanup on nodes after horizontally scaling.

tnozicka

/lgtm

scylla-operator-bot · 2023-08-09T13:36:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

OWNERS

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

Operator v1.10 runs cleanups [1] on each of the old nodes after adding a new one like it is recommended by the Scylla documentation [2]. So, cover this functionality with the functional test. [1] scylladb/scylla-operator/pull/1294 [2] https://opensource.docs.scylladb.com/stable/operating-scylla/ \ procedures/cluster-management/add-node-to-cluster.html

zimnx added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 3, 2023

zimnx added this to the v1.10 milestone Jul 3, 2023

zimnx requested review from tnozicka and rzetelskik July 3, 2023 16:29

zimnx force-pushed the mz/cleanup branch from 80bf692 to 10357c6 Compare July 4, 2023 08:03

rzetelskik requested changes Jul 5, 2023

View reviewed changes

zimnx force-pushed the mz/cleanup branch 2 times, most recently from 7d1f6ce to fc4c472 Compare July 17, 2023 12:05

rzetelskik self-requested a review July 19, 2023 10:17

rzetelskik previously approved these changes Jul 19, 2023

View reviewed changes

zimnx marked this pull request as draft July 20, 2023 10:28

tnozicka reviewed Jul 20, 2023

View reviewed changes

zimnx dismissed rzetelskik’s stale review via 79b1c0d July 21, 2023 12:13

zimnx force-pushed the mz/cleanup branch from fc4c472 to 79b1c0d Compare July 21, 2023 12:13

zimnx marked this pull request as ready for review July 21, 2023 12:19

zimnx force-pushed the mz/cleanup branch 2 times, most recently from d18639a to 79b1c0d Compare July 24, 2023 13:20

zimnx requested a review from tnozicka July 25, 2023 14:03

tnozicka reviewed Jul 27, 2023

View reviewed changes

tnozicka reviewed Jul 28, 2023

View reviewed changes

zimnx force-pushed the mz/cleanup branch from 79b1c0d to 9c05b96 Compare July 31, 2023 10:29

zimnx marked this pull request as draft July 31, 2023 12:34

zimnx force-pushed the mz/cleanup branch 2 times, most recently from 88e6b3a to f0cdcd8 Compare July 31, 2023 17:01

zimnx marked this pull request as ready for review August 2, 2023 09:56

zimnx requested a review from tnozicka August 2, 2023 09:56

zimnx force-pushed the mz/cleanup branch from f0cdcd8 to a1ee047 Compare August 3, 2023 10:21

tnozicka previously approved these changes Aug 9, 2023

View reviewed changes

scylla-operator-bot bot assigned tnozicka Aug 9, 2023

scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2023

tnozicka force-pushed the mz/cleanup branch from a1ee047 to a68f551 Compare August 9, 2023 11:47

zimnx dismissed tnozicka’s stale review via 9982df4 August 9, 2023 12:24

zimnx force-pushed the mz/cleanup branch from a68f551 to 9982df4 Compare August 9, 2023 12:24

scylla-operator-bot bot removed do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. do-not-merge/contains-merge-commits labels Aug 9, 2023

tnozicka approved these changes Aug 9, 2023

View reviewed changes

scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2023

zimnx added 2 commits August 9, 2023 15:34

Use generic pointer.Ptr in scyllacluster resource unit tests

934a58f

zimnx force-pushed the mz/cleanup branch from 9982df4 to d8e7d3c Compare August 9, 2023 13:34

scylla-operator-bot bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2023

zimnx enabled auto-merge August 9, 2023 13:35

tnozicka approved these changes Aug 9, 2023

View reviewed changes

scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2023

zimnx merged commit d131f8e into scylladb:master Aug 10, 2023
39 of 41 checks passed

zimnx deleted the mz/cleanup branch August 10, 2023 10:22

vponomaryov mentioned this pull request Aug 15, 2023

test(k8s): add new functional test to cover DB node cleanups scylladb/scylla-cluster-tests#6501

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run node cleanup after scaling the ScyllaCluster #1294

Run node cleanup after scaling the ScyllaCluster #1294

zimnx commented Jul 3, 2023 •

edited by tnozicka

tzach commented Jul 3, 2023

zimnx commented Jul 4, 2023 •

edited

rzetelskik left a comment

rzetelskik Jul 5, 2023

zimnx Jul 17, 2023

tnozicka Jul 20, 2023 •

edited

rzetelskik left a comment

tnozicka left a comment

tnozicka left a comment

tnozicka Jul 27, 2023

mykaul commented Aug 8, 2023

zimnx commented Aug 8, 2023

tnozicka left a comment

tnozicka left a comment

tnozicka left a comment

scylla-operator-bot bot commented Aug 9, 2023

Run node cleanup after scaling the ScyllaCluster #1294

Run node cleanup after scaling the ScyllaCluster #1294

Conversation

zimnx commented Jul 3, 2023 • edited by tnozicka

tzach commented Jul 3, 2023

zimnx commented Jul 4, 2023 • edited

rzetelskik left a comment

Choose a reason for hiding this comment

rzetelskik Jul 5, 2023

Choose a reason for hiding this comment

zimnx Jul 17, 2023

Choose a reason for hiding this comment

tnozicka Jul 20, 2023 • edited

Choose a reason for hiding this comment

rzetelskik left a comment

Choose a reason for hiding this comment

tnozicka left a comment

Choose a reason for hiding this comment

tnozicka left a comment

Choose a reason for hiding this comment

tnozicka Jul 27, 2023

Choose a reason for hiding this comment

mykaul commented Aug 8, 2023

zimnx commented Aug 8, 2023

tnozicka left a comment

Choose a reason for hiding this comment

tnozicka left a comment

Choose a reason for hiding this comment

tnozicka left a comment

Choose a reason for hiding this comment

scylla-operator-bot bot commented Aug 9, 2023

zimnx commented Jul 3, 2023 •

edited by tnozicka

zimnx commented Jul 4, 2023 •

edited

tnozicka Jul 20, 2023 •

edited