Support scaling changes during ongoing scaling #1283

zimnx · 2023-06-16T14:56:04Z

Decommissioning label reconcilation was moved from StatefulSet controller into Service controller.
StatefulSet controller can now react dynamically to scaling events occuring while scaling operations are still ongoing.

Fixes #1188

rzetelskik

Overall well done, I like how much more readable the scaling code is now. I left some comments though, especially regarding the edge case of scaling down and up immediatelly.

rzetelskik · 2023-07-06T08:59:15Z

pkg/cmd/operator/sidecar.go

+		ctx,
+		service.Name,
+		types.MergePatchType,
+		[]byte(fmt.Sprintf(`{"metadata": {"annotations": {%q: %q} } }`, naming.NodeInitialized, naming.LabelValueTrue)),


I don't understand the semantics behind this annotation. The comment for naming.NodeInitialized says "NodeInitialized means given node was started.", while the Scylla process is run at a later time. Should this be moved to after the process is started?

rzetelskik · 2023-07-06T09:08:16Z

pkg/controller/scyllacluster/sync_services.go

-		// Do not delete services that weren't properly decommissioned.
-		if svc.Labels[naming.DecommissionedLabel] != naming.LabelValueTrue {
+		// Do not delete services that were bootstrapped but weren't properly decommissioned.
+		if svc.Annotations[naming.NodeInitialized] != "" && svc.Labels[naming.DecommissionedLabel] != naming.LabelValueTrue {
 			klog.Warningf("Refusing to cleanup service %s/%s whose member wasn't decommissioned.", svc.Namespace, svc.Name)


nit: Can you also switch from using "%s/%s" to KObj or similar while we're at it?

rzetelskik · 2023-07-06T09:23:58Z

pkg/controller/scyllacluster/sync_statefulsets.go

+			if sts.Status.Replicas < *sts.Spec.Replicas {
+				scale.Spec.Replicas = sts.Status.Replicas
+			} else {
+				// Scale bootstrapped nodes one by one.


nit: Scale is a variable in this context so I found it confusing. s/Scale/Downscale

rzetelskik · 2023-07-06T09:24:59Z

pkg/controller/scyllacluster/sync_statefulsets.go

-					return progressingConditions, err
+		// Scale down is required
+		if *req.Spec.Replicas < *sts.Spec.Replicas {
+			// Stop ongoing scaling up


nit: s/scaling up/scale-up

rzetelskik · 2023-07-06T09:25:16Z

pkg/controller/scyllacluster/sync_statefulsets.go

-				_, err := scc.kubeClient.CoreV1().Services(lastSvcCopy.Namespace).Update(ctx, lastSvcCopy, metav1.UpdateOptions{})
-				if err != nil {
-					return progressingConditions, err
+		// Scale down is required


nit: s/Scale down/Scale-down

rzetelskik · 2023-07-06T12:08:56Z

pkg/controller/scyllacluster/sync_statefulsets.go

-					Message:            fmt.Sprintf("Waiting for rack service %q to decommission.", naming.ObjRef(svc)),
-					ObservedGeneration: sc.Generation,
-				})
+		if *req.Spec.Replicas == *sts.Spec.Replicas {


This isn't a regression, but since you're changing the logic a bit, I think we should handle the case I described here: #1293 (comment), i.e. a scale-down followed immediately by a scale-up should result in the node being decommissioned and deleted, which with current implementation won't trigger a downscale.

rzetelskik · 2023-07-06T13:20:46Z

test/e2e/set/scyllacluster/scyllacluster_scaling.go

 )

 var _ = g.Describe("ScyllaCluster", func() {
 	defer g.GinkgoRecover()

 	f := framework.NewFramework("scyllacluster")

+	g.It("should react on scaling up in the middle of scaling down", func() {


Suggested change

g.It("should react on scaling up in the middle of scaling down", func() {

g.It("should react to scaling up in the middle of scaling down", func() {

rzetelskik · 2023-07-06T13:21:36Z

test/e2e/set/scyllacluster/scyllacluster_scaling.go

+		o.Expect(secondNodeWasLeaving.Load()).To(o.BeFalse())
+	})
+
+	g.It("should react on scaling down in the middle of scaling up", func() {


Suggested change

g.It("should react on scaling down in the middle of scaling up", func() {

g.It("should react to scaling down in the middle of scaling up", func() {

rzetelskik · 2023-07-06T13:37:21Z

test/e2e/set/scyllacluster/scyllacluster_scaling.go

+			}, time.Second, observerCtx.Done())
+		}()
+
+		framework.By("Scaling up the ScyllaCluster to 4 replicas")


For some reason scaling to 4 replicas makes it not hit the edge case described here #1293 (comment). Try changing it to 3 and maybe lower the polling interval a bit and you'll see the test failing.

rzetelskik · 2023-07-06T13:38:28Z

test/e2e/utils/helpers.go

+	}
+
+	if s.Spec.Type != corev1.ServiceTypeClusterIP {
+		return "", fmt.Errorf("service %s/%s is of type %q instead of %q", s.Namespace, s.Name, s.Spec.Type, corev1.ServiceTypeClusterIP)


nit : use KObj or similar instead of %s/%s

kobj is to be used with klog, there is an equivalent function in naming package for everything else

Decommissioning label reconcilation was moved from StatefulSet controller into Service controller. StatefulSet controller can now react dynamically to scaling events occuring while scaling operations are still ongoing.

scylla-operator-bot · 2023-09-12T11:40:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [zimnx]

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

scylla-operator-bot · 2023-09-19T00:11:33Z

PR needs rebase.

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/test-infra repository.

scylla-operator-bot · 2024-05-16T11:49:52Z

@zimnx: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gke	`431e7cd`	link	true	`/test e2e-gke`
ci/prow/verify-vendorability	`5b0b343`	link	true	`/test verify-vendorability`
ci/prow/helm-build	`5b0b343`	link	true	`/test helm-build`
ci/prow/verify	`5b0b343`	link	true	`/test verify`
ci/prow/unit	`5b0b343`	link	true	`/test unit`
ci/prow/verify-deps	`5b0b343`	link	true	`/test verify-deps`
ci/prow/e2e-gke-serial	`5b0b343`	link	true	`/test e2e-gke-serial`
ci/prow/e2e-gke-parallel	`5b0b343`	link	true	`/test e2e-gke-parallel`
ci/prow/build	`5b0b343`	link	true	`/test build`
ci/prow/docs	`5b0b343`	link	true	`/test docs`
ci/prow/e2e-gke-parallel-clusterip	`5b0b343`	link	true	`/test e2e-gke-parallel-clusterip`
ci/prow/images	`5b0b343`	link	true	`/test images`
ci/prow/e2e-gke-release-script-latest	`5b0b343`	link	true	`/test e2e-gke-release-script-latest`

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

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

zimnx added this to the v1.10 milestone Jun 16, 2023

zimnx force-pushed the mz/1188-shortest-scale-path branch from 593ed6b to cf5178c Compare June 16, 2023 15:02

zimnx force-pushed the mz/1188-shortest-scale-path branch from cf5178c to 431e7cd Compare June 27, 2023 09:38

rzetelskik mentioned this pull request Jul 6, 2023

The node is already decommissioned error after second decommission of the same Scylla member which was recreated #1293

Open

rzetelskik requested changes Jul 6, 2023

View reviewed changes

tnozicka removed this from the v1.10 milestone Aug 17, 2023

scylla-operator-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2023

Support scaling changes during ongoing scaling

5b0b343

Decommissioning label reconcilation was moved from StatefulSet controller into Service controller. StatefulSet controller can now react dynamically to scaling events occuring while scaling operations are still ongoing.

zimnx force-pushed the mz/1188-shortest-scale-path branch from 431e7cd to 5b0b343 Compare September 12, 2023 11:40

scylla-operator-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2023

scylla-operator-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 12, 2023

scylla-operator-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support scaling changes during ongoing scaling #1283

Support scaling changes during ongoing scaling #1283

zimnx commented Jun 16, 2023

rzetelskik left a comment

rzetelskik Jul 6, 2023

rzetelskik Jul 6, 2023

rzetelskik Jul 6, 2023

rzetelskik Jul 6, 2023

rzetelskik Jul 6, 2023

rzetelskik Jul 6, 2023

rzetelskik Jul 6, 2023

rzetelskik Jul 6, 2023

rzetelskik Jul 6, 2023

rzetelskik Jul 6, 2023

tnozicka Jul 20, 2023

scylla-operator-bot bot commented Sep 12, 2023

scylla-operator-bot bot commented Sep 19, 2023

scylla-operator-bot bot commented May 16, 2024

	g.It("should react on scaling up in the middle of scaling down", func() {
	g.It("should react to scaling up in the middle of scaling down", func() {

	g.It("should react on scaling down in the middle of scaling up", func() {
	g.It("should react to scaling down in the middle of scaling up", func() {

Support scaling changes during ongoing scaling #1283

Are you sure you want to change the base?

Support scaling changes during ongoing scaling #1283

Conversation

zimnx commented Jun 16, 2023

rzetelskik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scylla-operator-bot bot commented Sep 12, 2023

scylla-operator-bot bot commented Sep 19, 2023

scylla-operator-bot bot commented May 16, 2024