Ability to move data between JBOD disks using Cruise Control #10644

ShubhamRwt · 2024-09-26T12:30:48Z

Type of change

This PR adds the ability to move data between two JBOD disks using Cruise Control. To do the same, we have introduced a new mode called remove-disks.
You can use it like this:

yaml
apiVersion: kafka.strimzi.io/v1beta2
kind: KafkaRebalance
metadata:
  name: my-rebalance
  labels:
    strimzi.io/cluster: my-cluster
spec:
  # setting the mode as `remove-disks` to move data between the JBOD disks
  mode: remove-disks
  # providing the list of brokers, and the corresponding volumes from which you want to move the replicas
  moveReplicasOffVolumes:
    - brokerId: 0
      volumeIds: [1, 2]
    - brokerId: 2
      volumeIds: [1]
# ...

Bugfix
Enhancement / new feature
Refactoring
Documentation

Description

Please describe your pull request

Checklist

Please go through this checklist and make sure all applicable tasks have been done

Write tests
Make sure all tests pass
Update documentation
Check RBAC rights for Kubernetes / OpenShift roles
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging
Update CHANGELOG.md
Supply screenshots for visual changes, such as Grafana dashboards

scholzj · 2024-09-30T15:02:37Z

/azp run regression

azure-pipelines · 2024-09-30T15:02:52Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj

I left some comments. But I'm not really an CC expert. So the CC related parts have to be reviewed by @ppatierno as the SME.

scholzj · 2024-09-30T21:21:43Z

api/src/main/java/io/strimzi/api/kafka/model/rebalance/BrokerAndVolumeIds.java

+    private List<Integer> volumeIds;
+    private Map<String, Object> additionalProperties;
+
+    @Description("Id of the broker which contains the disk from which you want to move the the partition replicas from")


Here and in the other doc comments -> I think we use ID in the docs. Id is only the Java capitalization.

Okay, I will correct it

scholzj · 2024-09-30T21:23:24Z

api/src/main/java/io/strimzi/api/kafka/model/rebalance/KafkaRebalanceSpec.java

@@ -31,6 +31,17 @@ public class KafkaRebalanceSpec extends Spec {
    private KafkaRebalanceMode mode = KafkaRebalanceMode.FULL;
    private List<Integer> brokers;

+    @Description("List of the brokers and the volumes corresponding to them whose replicas needs to be moved")


Suggested change

@Description("List of the brokers and the volumes corresponding to them whose replicas needs to be moved")

@Description("List of the brokers and the volumes corresponding to them whose replicas need to be moved")

Maybe tweak it a bit more:

@Description("List of brokers and their corresponding volumes from which replicas need to be moved.")

scholzj · 2024-09-30T21:24:21Z

api/src/main/java/io/strimzi/api/kafka/model/rebalance/KafkaRebalanceSpec.java

+    @Description("List of the brokers and the volumes corresponding to them whose replicas needs to be moved")
+    public List<BrokerAndVolumeIds> getMoveReplicasOffVolumes() {
+        return moveReplicasOffVolumes;
+    }
+
+    public void setMoveReplicasOffVolumes(List<BrokerAndVolumeIds> moveReplicasOffVolumes) {
+        this.moveReplicasOffVolumes = moveReplicasOffVolumes;
+    }
+
+    private List<BrokerAndVolumeIds> moveReplicasOffVolumes;
+


Please keep the separation/order of the field declaration and the methods.

scholzj · 2024-09-30T21:26:13Z

.../main/java/io/strimzi/operator/cluster/operator/assembly/KafkaRebalanceAssemblyOperator.java

+                if (brokerAndVolumeIds != null && !brokerAndVolumeIds.isEmpty()) {
+                    ((RemoveDisksOptions.RemoveDisksOptionsBuilder) rebalanceOptionsBuilder).withBrokersandVolumeIds(brokerAndVolumeIds);
+                } else {
+                    throw new IllegalArgumentException("The brokers and volume id list is mandatory when using the " + mode.toValue() + " rebalancing mode");


ID in capital again

.../main/java/io/strimzi/operator/cluster/operator/assembly/KafkaRebalanceAssemblyOperator.java

api/src/main/java/io/strimzi/api/kafka/model/rebalance/KafkaRebalanceSpec.java

ppatierno · 2024-10-02T07:43:07Z

api/src/main/java/io/strimzi/api/kafka/model/rebalance/KafkaRebalanceSpec.java

+    @Description("List of the brokers and the volumes corresponding to them whose replicas needs to be moved")
+    public List<BrokerAndVolumeIds> getMoveReplicasOffVolumes() {
+        return moveReplicasOffVolumes;
+    }
+
+    public void setMoveReplicasOffVolumes(List<BrokerAndVolumeIds> moveReplicasOffVolumes) {
+        this.moveReplicasOffVolumes = moveReplicasOffVolumes;
+    }
+
+    private List<BrokerAndVolumeIds> moveReplicasOffVolumes;
+


api/src/test/java/io/strimzi/api/kafka/model/rebalance/KafkaRebalanceCrdIT.java

.../main/java/io/strimzi/operator/cluster/operator/assembly/KafkaRebalanceAssemblyOperator.java

...ava/io/strimzi/operator/cluster/operator/resource/cruisecontrol/CruiseControlClientTest.java

...test/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/MockCruiseControl.java

...c/test/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/PathBuilderTest.java

ppatierno · 2024-10-03T11:01:11Z

@ShubhamRwt I would rebase against main, because a refactoring around getting task status of rebalance was just merged. It would be better for you to have the code up to date while addressing comments on this PR. Thanks!

ShubhamRwt · 2024-10-03T11:03:05Z

Yep I am working on that @ppatierno, trying to address all the comments and rebase. After that I will test things in a cluster to be sure everything works

PaulRMellor

I made a few suggestions on the descriptions

api/src/main/java/io/strimzi/api/kafka/model/rebalance/BrokerAndVolumeIds.java

PaulRMellor · 2024-10-14T15:53:28Z

api/src/main/java/io/strimzi/api/kafka/model/rebalance/KafkaRebalanceSpec.java

@@ -31,6 +31,17 @@ public class KafkaRebalanceSpec extends Spec {
    private KafkaRebalanceMode mode = KafkaRebalanceMode.FULL;
    private List<Integer> brokers;

+    @Description("List of the brokers and the volumes corresponding to them whose replicas needs to be moved")


Maybe tweak it a bit more:

@Description("List of brokers and their corresponding volumes from which replicas need to be moved.")

.../main/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/CruiseControlApi.java

.../main/java/io/strimzi/operator/cluster/operator/assembly/KafkaRebalanceAssemblyOperator.java

.../main/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/CruiseControlApi.java

...r/src/main/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/PathBuilder.java

...ain/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/RemoveDisksOptions.java

...test/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/MockCruiseControl.java

ppatierno · 2024-10-16T15:59:04Z

@ShubhamRwt could you rebase against main, I merged another PR fixing some reconciliation issues on KafkaRebalance (You got conflicts as well).

.../main/java/io/strimzi/operator/cluster/operator/assembly/KafkaRebalanceAssemblyOperator.java

.../test/java/io/strimzi/operator/cluster/operator/assembly/KafkaRebalanceStateMachineTest.java

ppatierno

LGTM. Thanks!

ppatierno · 2024-10-21T10:23:20Z

@scholzj @PaulRMellor could you have another pass to check if Shubham addressed your comments as well?

PaulRMellor

Looks good.
I left a few suggestions around changing ID's to IDs to be consistent

PaulRMellor · 2024-10-21T10:21:55Z

.../main/java/io/strimzi/operator/cluster/operator/assembly/KafkaRebalanceAssemblyOperator.java

+                if (brokerAndVolumeIds != null && !brokerAndVolumeIds.isEmpty()) {
+                    ((RemoveDisksOptions.RemoveDisksOptionsBuilder) rebalanceOptionsBuilder).withBrokersandVolumeIds(brokerAndVolumeIds);
+                } else {
+                    throw new IllegalArgumentException("The brokers and volume ID's list is mandatory when using the " + mode.toValue() + " rebalancing mode");


Suggested change

throw new IllegalArgumentException("The brokers and volume ID's list is mandatory when using the " + mode.toValue() + " rebalancing mode");

throw new IllegalArgumentException("The brokers and volume IDs list is mandatory when using the " + mode.toValue() + " rebalancing mode");

PaulRMellor · 2024-10-21T10:22:12Z

...r/src/main/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/PathBuilder.java

@@ -73,6 +75,23 @@ public PathBuilder withParameter(CruiseControlParameters param, List<String> val
        return this;
    }

+    /**
+     * Converts the list of broker and volume ID's to a list of string


Suggested change

* Converts the list of broker and volume ID's to a list of string

* Converts the list of broker and volume IDs to a list of string

PaulRMellor · 2024-10-21T10:22:38Z

...ain/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/RemoveDisksOptions.java

+ * Rebalance options for removing disks from brokers within the Kafka cluster
+ */
+public class RemoveDisksOptions extends AbstractRebalanceOptions {
+    /** list with the ID's of the broker and volumes which will be used by remove-disks endpoint */


Suggested change

/** list with the ID's of the broker and volumes which will be used by remove-disks endpoint */

/** list with the IDs of the broker and volumes which will be used by remove-disks endpoint */

PaulRMellor · 2024-10-21T10:22:47Z

...ain/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/RemoveDisksOptions.java

+    private final List<BrokerAndVolumeIds> brokerAndVolumeIds;
+
+    /**
+     * @return  List of brokers and volume ID's which will be used by remove-disks endpoint


Suggested change

* @return List of brokers and volume ID's which will be used by remove-disks endpoint

* @return List of brokers and volume IDs which will be used by remove-disks endpoint

PaulRMellor · 2024-10-21T10:23:08Z

...ain/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/RemoveDisksOptions.java

+        }
+
+        /**
+         * List of broker and volume ID's to be used


Suggested change

* List of broker and volume ID's to be used

* List of broker and volume IDs to be used

PaulRMellor · 2024-10-21T10:23:17Z

...ain/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/RemoveDisksOptions.java

+        /**
+         * List of broker and volume ID's to be used
+         *
+         * @param brokerAndVolumeIdsList   List of broker and volume ID's


Suggested change

* @param brokerAndVolumeIdsList List of broker and volume ID's

* @param brokerAndVolumeIdsList List of broker and volume IDs

PaulRMellor · 2024-10-21T10:23:39Z

api/src/main/java/io/strimzi/api/kafka/model/rebalance/BrokerAndVolumeIds.java

+import java.util.Map;
+
+/**
+ * Configures the broker and Volume ID's for the remove-disks endpoint for Cruise Control


Suggested change

* Configures the broker and Volume ID's for the remove-disks endpoint for Cruise Control

* Configures the broker and Volume IDs for the remove-disks endpoint for Cruise Control

scholzj · 2024-10-21T12:59:35Z

packaging/helm-charts/helm3/strimzi-kafka-operator/crds/040-Crd-kafka.yaml

@@ -8229,6 +8230,7 @@ spec:
                              - full
                              - add-brokers
                              - remove-brokers
+                              - remove-disks


Why is this changing here? Has nothing to do with the Kafka CR. I would expect these to be using different types to not have unused fields.

I think it's changing because the autorebalance related API class definition is using the same enum to get the modes (so also "full" is included even if not really supported) :-/ And when adding remove-disks, it's there as well.
The only solution I see is not using the enum for autorebalance but just a String and adding additional checks on the values, or just sticking with what we have.

Or maybe better, adding a new KafkaAutoRebalanceMode with just add-brokers and remove-brokers mode.

Yeah, I think you can have a separate Enum with the values supported there.

Ok then, I am going to open a trivial PR for this because it's unrelated to this PR. @ShubhamRwt you can then rebase and continue with this one.

I opened this one for it #10744

scholzj · 2024-10-21T13:12:36Z

api/src/main/java/io/strimzi/api/kafka/model/rebalance/BrokerAndVolumeIds.java

+
+    @Description("ID of the broker that contains the disk from which you want to move the partition replicas.")
+    @JsonInclude(JsonInclude.Include.NON_NULL)
+    @JsonProperty("brokerId")


Is this needed?

scholzj · 2024-10-21T13:12:43Z

api/src/main/java/io/strimzi/api/kafka/model/rebalance/BrokerAndVolumeIds.java

+
+    @Description("IDs of the disks from which the partition replicas need to be moved.")
+    @JsonInclude(JsonInclude.Include.NON_NULL)
+    @JsonProperty("volumeIds")


Is this needed?

ShubhamRwt · 2024-10-25T11:57:04Z

@scholzj Hi, can we run the pipeline tests in this?

scholzj · 2024-10-25T12:01:31Z

/azp run regression

azure-pipelines · 2024-10-25T12:01:51Z

Azure Pipelines successfully started running 1 pipeline(s).

ShubhamRwt · 2024-10-27T06:01:49Z

The regression failure doesn't look related to this PR

scholzj · 2024-10-27T12:28:51Z

The regression failure doesn't look related to this PR

Ok, let's re-run it.

scholzj · 2024-10-27T12:28:57Z

/azp run regression

azure-pipelines · 2024-10-27T12:29:09Z

Azure Pipelines successfully started running 1 pipeline(s).

ShubhamRwt · 2024-10-28T10:07:36Z

The test failing in the regression is passing for me locally even when I have rebased to the main branch. @see-quick Do you have any idea why it fails?

Signed-off-by: ShubhamRwt <[email protected]>

ShubhamRwt · 2024-11-01T07:46:53Z

@scholzj I have rebased the PR and made the minor fix. Do you have any other suggestions which I need to fix?

scholzj

Thanks @ShubhamRwt. LGTM.

ShubhamRwt force-pushed the moveReplicasOffDisk branch 4 times, most recently from 923b3a0 to 01ee408 Compare September 30, 2024 11:55

ShubhamRwt requested a review from scholzj September 30, 2024 12:19

ShubhamRwt marked this pull request as ready for review September 30, 2024 12:19

ShubhamRwt requested a review from ppatierno September 30, 2024 12:19

ShubhamRwt added this to the 0.44.0 milestone Sep 30, 2024

scholzj reviewed Sep 30, 2024

View reviewed changes

scholzj requested a review from PaulRMellor October 1, 2024 09:33

ppatierno requested changes Oct 2, 2024

View reviewed changes

PaulRMellor reviewed Oct 14, 2024

View reviewed changes

ShubhamRwt force-pushed the moveReplicasOffDisk branch 2 times, most recently from 0b03a1e to f46d8f5 Compare October 16, 2024 09:05

ppatierno requested changes Oct 16, 2024

View reviewed changes

ShubhamRwt force-pushed the moveReplicasOffDisk branch 2 times, most recently from 6214287 to f626564 Compare October 18, 2024 13:10

ppatierno requested changes Oct 21, 2024

View reviewed changes

.../main/java/io/strimzi/operator/cluster/operator/assembly/KafkaRebalanceAssemblyOperator.java Outdated Show resolved Hide resolved

.../test/java/io/strimzi/operator/cluster/operator/assembly/KafkaRebalanceStateMachineTest.java Outdated Show resolved Hide resolved

ppatierno approved these changes Oct 21, 2024

View reviewed changes

PaulRMellor approved these changes Oct 21, 2024

View reviewed changes

scholzj reviewed Oct 21, 2024

View reviewed changes

ppatierno mentioned this pull request Oct 21, 2024

Refactored autorebalance by adding a dedicated Enum for modes #10744

Merged

2 tasks

scholzj modified the milestones: 0.44.0, 0.45.0 Oct 21, 2024

ShubhamRwt force-pushed the moveReplicasOffDisk branch from b30b09e to 7f2960b Compare October 24, 2024 08:58

ShubhamRwt force-pushed the moveReplicasOffDisk branch from 7f2960b to 9e812eb Compare October 27, 2024 06:12

ShubhamRwt added 8 commits November 1, 2024 13:02

Added the ability to move data between the JBOD disks using CC

f740981

Signed-off-by: ShubhamRwt <[email protected]>

Added the suggestion by Paolo, Jakub and Paul

7e04d93

Signed-off-by: ShubhamRwt <[email protected]>

fixed minor nits

03b767b

Signed-off-by: ShubhamRwt <[email protected]>

Added suggested changes by Paolo

d923918

Signed-off-by: ShubhamRwt <[email protected]>

Fix the errors

db7d047

Signed-off-by: ShubhamRwt <[email protected]>

Added the changes from Paolo

c4897e7

Signed-off-by: ShubhamRwt <[email protected]>

Added suggested changes by Paul

6ff9778

Signed-off-by: ShubhamRwt <[email protected]>

Added the suggestion by Jakub

158bd1f

Signed-off-by: ShubhamRwt <[email protected]>

ShubhamRwt force-pushed the moveReplicasOffDisk branch from 9e812eb to 158bd1f Compare November 1, 2024 07:32

Minor fix based upon Tina's fix

184a0a7

Signed-off-by: ShubhamRwt <[email protected]>

scholzj approved these changes Nov 1, 2024

View reviewed changes

scholzj merged commit a55eabf into strimzi:main Nov 1, 2024
13 checks passed

ppatierno mentioned this pull request Nov 11, 2024

Make it possible to use Cruise Control to move all data between two JBOD disks #7532

Closed

see-quick mentioned this pull request Dec 4, 2024

[system test] Add CruiseControl removeDisks test case #10910

Merged

2 tasks

	@Description("List of the brokers and the volumes corresponding to them whose replicas needs to be moved")
	@Description("List of the brokers and the volumes corresponding to them whose replicas need to be moved")

	throw new IllegalArgumentException("The brokers and volume ID's list is mandatory when using the " + mode.toValue() + " rebalancing mode");
	throw new IllegalArgumentException("The brokers and volume IDs list is mandatory when using the " + mode.toValue() + " rebalancing mode");

	* Converts the list of broker and volume ID's to a list of string
	* Converts the list of broker and volume IDs to a list of string

	/** list with the ID's of the broker and volumes which will be used by remove-disks endpoint */
	/** list with the IDs of the broker and volumes which will be used by remove-disks endpoint */

	* @return List of brokers and volume ID's which will be used by remove-disks endpoint
	* @return List of brokers and volume IDs which will be used by remove-disks endpoint

	* List of broker and volume ID's to be used
	* List of broker and volume IDs to be used

	* @param brokerAndVolumeIdsList List of broker and volume ID's
	* @param brokerAndVolumeIdsList List of broker and volume IDs

	* Configures the broker and Volume ID's for the remove-disks endpoint for Cruise Control
	* Configures the broker and Volume IDs for the remove-disks endpoint for Cruise Control

Ability to move data between JBOD disks using Cruise Control #10644

Ability to move data between JBOD disks using Cruise Control #10644

Conversation

ShubhamRwt commented Sep 26, 2024 • edited Loading

Type of change

Description

Checklist

scholzj commented Sep 30, 2024

azure-pipelines bot commented Sep 30, 2024

scholzj 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

ppatierno commented Oct 3, 2024

ShubhamRwt commented Oct 3, 2024

PaulRMellor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppatierno commented Oct 16, 2024

ppatierno left a comment

Choose a reason for hiding this comment

ppatierno commented Oct 21, 2024

PaulRMellor 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

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

ShubhamRwt commented Oct 25, 2024

scholzj commented Oct 25, 2024

azure-pipelines bot commented Oct 25, 2024

ShubhamRwt commented Oct 27, 2024

scholzj commented Oct 27, 2024

scholzj commented Oct 27, 2024

azure-pipelines bot commented Oct 27, 2024

ShubhamRwt commented Oct 28, 2024

ShubhamRwt commented Nov 1, 2024

scholzj left a comment

Choose a reason for hiding this comment

ShubhamRwt commented Sep 26, 2024 •

edited

Loading