Prevent broker scaledown if broker contains partition replicas - Part 1 #9042

ShubhamRwt · 2023-08-29T09:48:26Z

Type of change

Select the type of your PR

Bugfix
Enhancement / new feature
Refactoring
Documentation

Description

This PR introduces the mechanism to stop the scale down if the broker contains partition replicas. If the partition replicas are present on the broker then scale down will be halted and we fail the reconciliation. In the second part of this work, we plan to revert the replicas instead of just failing the reconciliation.

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

I left some nits. You should also consider adding some tests.

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

...src/main/java/io/strimzi/operator/cluster/operator/assembly/PreventBrokerScaleDownUtils.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

operator-common/src/main/java/io/strimzi/operator/common/Annotations.java

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

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaPool.java

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

...src/main/java/io/strimzi/operator/cluster/operator/assembly/PreventBrokerScaleDownUtils.java

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

scholzj

You should also adda note to the CHANGELOG.md abotu this.

ShubhamRwt · 2023-09-14T12:25:22Z

@scholzj Hi, I have incorporated all the changes just mentioning the things in the changelog is left. Does this look good apart from that?

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

...ain/java/io/strimzi/operator/cluster/operator/assembly/PreventBrokerScaleDownOperations.java

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

scholzj · 2023-09-14T14:56:17Z

operator-common/src/main/java/io/strimzi/operator/common/Annotations.java

+    /**
+     * Annotation used to bypass the broker scale-down mechanism
+     */
+    public static final String ANNO_STRIMZI_IO_SKIP_BROKER_SCALEDOWN_CHECK = STRIMZI_DOMAIN + "skip-broker-scaledown-check";
+


Note for the future: Depending on what is merged first, this should reconcile with #9103 as this is a public annotation. (nothing to do right now, we just need to keep it in mind)

CHANGELOG.md

...src/main/java/io/strimzi/operator/cluster/operator/assembly/PreventBrokerScaleDownCheck.java

ShubhamRwt · 2023-09-15T15:29:55Z

Pushed the changes @scholzj. Just a doubt though -> Make sure brokers are empty before scaling them down Shouldn't we mention the annotation also?

scholzj · 2023-09-15T15:31:50Z

I don't think the CHANGELOG should supply docs. There should be docs covering the feature and that should list the annotation. If you want, you could also mention it in some log message if you want. But in CHANGELOG it seems unnecessary.

ShubhamRwt · 2023-09-15T15:32:56Z

I don't think the CHANGELOG should supply docs. There should be docs covering the feature and that should list the annotation. If you want, you could also mention it in some log message if you want. But in CHANGELOG it seems unnecessary.

Okay, Thanks Jakub

scholzj

Nice job 👍

scholzj · 2023-09-15T18:48:03Z

/azp run regression

azure-pipelines · 2023-09-15T18:48:14Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2023-09-16T23:29:10Z

@ShubhamRwt Can you please rebase it and move the new annotation to the api module? Thanks.

ShubhamRwt · 2023-09-17T06:05:51Z

Sure @scholzj. I will do that

ppatierno · 2023-09-17T12:02:22Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

@@ -367,6 +367,22 @@ public Set<NodeRef> nodes() {
        return nodes;
    }

+    /**
+     * Generates list of references to Kafka nodes going to be removed from the Kafka cluster. The references contain both the pod name and
+     * the ID of the Kafka node.


Is this description wrong? I mean it's returning just ID not pod name, right?

ppatierno · 2023-09-17T12:04:19Z

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

+                return Future.succeededFuture();
+            } else {
+                return brokerScaleDownOperations.canScaleDownBrokers(reconciliation, kafka.removedNodes(), secretOperator, adminClientProvider)
+                        .compose(s -> {


Can we use a more meaningful name for variable s? :)

ppatierno · 2023-09-17T12:10:33Z

...src/main/java/io/strimzi/operator/cluster/operator/assembly/PreventBrokerScaleDownCheck.java

+     * @param adminClientProvider  Used to create the Admin client instance
+     * @param idsToBeRemoved       Ids to be removed
+
+     * @return  returns a boolean future based on the outcome of the check


Wrong return description

ppatierno · 2023-09-17T12:11:29Z

...src/main/java/io/strimzi/operator/cluster/operator/assembly/PreventBrokerScaleDownCheck.java

+     * @param reconciliation       Reconciliation marker
+     * @param secretOperator       Secret operator for working with Secrets
+     * @param adminClientProvider  Used to create the Admin client instance
+     * @param idsToBeRemoved       Ids to be removed


Just a nit, can you list the parameters the same order as the method signature please?

ppatierno · 2023-09-17T12:14:50Z

...src/main/java/io/strimzi/operator/cluster/operator/assembly/PreventBrokerScaleDownCheck.java

+                    final Future<Collection<TopicDescription>> descriptions;
+                    try {
+                        String bootstrapHostname = KafkaResources.bootstrapServiceName(reconciliation.name()) + "." + reconciliation.namespace() + ".svc:" + KafkaCluster.REPLICATION_PORT;
+                        LOGGER.debugCr(reconciliation, "Creating AdminClient for cluster {}/{}", reconciliation.namespace());


The debug command is looking for two parameters/placeholders but you are passing one, just the namespace.

ppatierno · 2023-09-17T12:17:25Z

...src/main/java/io/strimzi/operator/cluster/operator/assembly/PreventBrokerScaleDownCheck.java

+                    } else {
+                        namesPromise.complete(names);
+                    }
+                });


Should we have some VertxUtils class method to go from a Kafka future to a Vertx one?

ppatierno · 2023-09-17T12:17:52Z

...src/main/java/io/strimzi/operator/cluster/operator/assembly/PreventBrokerScaleDownCheck.java

+                    } else {
+                        descPromise.complete(tds.values());
+                    }
+                });


Ditto as above?

...src/main/java/io/strimzi/operator/cluster/operator/assembly/PreventBrokerScaleDownCheck.java

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

scholzj · 2023-09-19T15:32:42Z

/azp run regression

azure-pipelines · 2023-09-19T15:32:53Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2023-09-19T20:34:06Z

@ShubhamRwt @henryZrncik It still seems to be failing ... is there one more place where it needs to be fixed? :-/

[ERROR] Errors: 
[ERROR] io.strimzi.systemtest.kafka.KafkaNodePoolST.testKafkaNodePoolBrokerIdsManagementUsingAnnotations(ExtensionContext)
[ERROR]   Run 1: KafkaNodePoolST.testKafkaNodePoolBrokerIdsManagementUsingAnnotations:280 » Wait Timeout after 180000 ms waiting for Pod: namespace-7/my-cluster-ea890678-kafka-pool-a18fff75-a to have 4 stable replicas
[ERROR]   Run 2: KafkaNodePoolST.testKafkaNodePoolBrokerIdsManagementUsingAnnotations:280 » Wait Timeout after 180000 ms waiting for Pod: namespace-15/my-cluster-33db8172-kafka-pool-a77b98c8-a to have 4 stable replicas
[ERROR]   Run 3: KafkaNodePoolST.testKafkaNodePoolBrokerIdsManagementUsingAnnotations:280 » Wait Timeout after 180000 ms waiting for Pod: namespace-16/my-cluster-6fe3a87f-kafka-pool-af5bc5a8-a to have 4 stable replicas

ShubhamRwt · 2023-09-20T09:26:19Z

I tested it a bit and yes looks like it passes when I disable the broker scale down check

ShubhamRwt · 2023-09-20T09:34:26Z

Would it be fine @scholzj @henryZrncik to modify this issue -> #9134 itself and add this failing test into the issue description instead of creating separate issue?

im-konge · 2023-09-20T09:37:17Z

@ShubhamRwt I guess it's not good practice to disable all tests (checks) that are failing. The one check which is now disabled makes sense, but this test - testKafkaNodePoolBrokerIdsManagementUsingAnnotations - should be fixed before merging the PR FMPOV.

henryZrncik · 2023-09-20T09:41:31Z

Given test does not contain something which should cause such error. (test itself is currently passing so it really is something caused by this change.

This is basically the only test which included node-pools and any kind of scaling and was part of regression which was run. So i think it is worth investigate how scaling up/down works with reggard to node-pools & this PR. I am writing this mainly because this failure occurres in scaling up (there is nothing that should prevent it).

As @im-konge mentioned, Disabling first test was good choice as we mentioned here that it need to be addressed after adding this new funcitonality, but this test is something which should work even after change.

im-konge · 2023-09-20T09:44:20Z

Let's see if there are more tests failing with NodePools.

im-konge · 2023-09-20T09:44:31Z

/azp run feature-gates-regression

azure-pipelines · 2023-09-20T09:44:42Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2023-09-20T13:07:13Z

I guess it's not good practice to disable all tests (checks) that are failing. The one check which is now disabled makes sense, but this test - testKafkaNodePoolBrokerIdsManagementUsingAnnotations - should be fixed before merging the PR FMPOV.

@ShubhamRwt is not disabling these tests, or? He is disabling his check.

Given test does not contain something which should cause such error. (test itself is currently passing so it really is something caused by this change.

@henryZrncik Are you really sure the test is correct today? I suspect it is not it - it is likely passing mostly by luck. You scale down broker 6 which might contain partition replicas without rescheduling them to other brokers. So I think the test is at fault, not Shubham's code.

im-konge · 2023-09-20T14:16:55Z

@ShubhamRwt is not disabling these tests, or? He is disabling his check.

I miss-read it, also I thought that the test is correct, but you mentioned that it works mostly out of luck. Anyway, if it's a test issue and we should update/fix the test, @ShubhamRwt can disable the check for now.

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

ShubhamRwt · 2023-09-20T14:56:28Z

I have now disabled the broker scale down check for the failing test.

scholzj · 2023-09-20T16:18:05Z

/azp run regression

azure-pipelines · 2023-09-20T16:18:18Z

Azure Pipelines successfully started running 1 pipeline(s).

im-konge · 2023-09-20T21:29:03Z

systemtest/src/test/java/io/strimzi/systemtest/kafka/KafkaNodePoolST.java

@@ -229,7 +229,7 @@ void testKafkaNodePoolBrokerIdsManagementUsingAnnotations(ExtensionContext exten

        Kafka kafka = KafkaTemplates.kafkaPersistent(testStorage.getClusterName(), 1, 1)
            .editOrNewMetadata()
-                .addToAnnotations(Annotations.ANNO_STRIMZI_IO_NODE_POOLS, "enabled")
+                .addToAnnotations(Map.of(Annotations.ANNO_STRIMZI_IO_NODE_POOLS, "enabled", Annotations.ANNO_STRIMZI_IO_SKIP_BROKER_SCALEDOWN_CHECK, "true"))


Maybe adding comment here would be helpful for the one who will fix those two tests, thanks! :)

I was wondering if I should mention about this failing test in this issue #9134 itself? I have left a comment about this issue already in the other failing test.

I think it is a good idea, We can change these test at the same time anyway.

Okay, I will update the issue then, Thanks @henryZrncik

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

scholzj · 2023-09-21T12:47:58Z

/azp run regression

azure-pipelines · 2023-09-21T12:48:10Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2023-09-21T13:45:28Z

/azp run feature-gates-regression

azure-pipelines · 2023-09-21T13:45:41Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2023-09-21T18:27:42Z

/azp run kraft-regression

azure-pipelines · 2023-09-21T18:27:53Z

Azure Pipelines successfully started running 1 pipeline(s).

ShubhamRwt requested a review from scholzj August 29, 2023 09:48

ShubhamRwt changed the title ~~Prevent broker scaledown if broker contains partition replicas~~ Prevent broker scaledown if broker contains partition replicas - 1 Aug 29, 2023

ShubhamRwt force-pushed the preventBrokerPrototype branch from 70e41de to 00abbce Compare August 29, 2023 10:19

ShubhamRwt changed the title ~~Prevent broker scaledown if broker contains partition replicas - 1~~ Prevent broker scaledown if broker contains partition replicas - Part 1 Aug 29, 2023

scholzj reviewed Aug 29, 2023

View reviewed changes

scholzj added this to the 0.38.0 milestone Sep 1, 2023

ShubhamRwt requested a review from scholzj September 4, 2023 20:33

scholzj reviewed Sep 5, 2023

View reviewed changes

ShubhamRwt requested a review from scholzj September 14, 2023 12:24

scholzj reviewed Sep 14, 2023

View reviewed changes

ShubhamRwt requested a review from scholzj September 15, 2023 07:50

ShubhamRwt force-pushed the preventBrokerPrototype branch from cc6a059 to 93de3af Compare September 15, 2023 08:26

scholzj reviewed Sep 15, 2023

View reviewed changes

ShubhamRwt requested a review from scholzj September 15, 2023 15:28

scholzj approved these changes Sep 15, 2023

View reviewed changes

scholzj requested a review from ppatierno September 15, 2023 18:48

ppatierno requested changes Sep 17, 2023

View reviewed changes

ShubhamRwt added 2 commits September 18, 2023 13:48

Added broker scale down Mechanism

f940d1f

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

Added suggestions by Jakub

27f2241

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

ShubhamRwt force-pushed the preventBrokerPrototype branch from 9560c1a to 27f2241 Compare September 18, 2023 08:23

ShubhamRwt requested a review from ppatierno September 18, 2023 08:23

Added comment for failing test

3101a8d

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

Disabled check for failing test

27230ac

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

im-konge reviewed Sep 20, 2023

View reviewed changes

Added the comment

5678617

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

scholzj merged commit 614399e into strimzi:main Sep 22, 2023
37 checks passed

This was referenced Oct 11, 2023

Revert blocked scale-down and continue reconciliation #9232

Closed

Prevent Kafka broker scale down if it still contains replicas #6736

Closed

[Enhancement] Integrate Cruise Control into the Kafka broker scale up/down process #4112

Open

Prevent broker scaledown if broker contains partition replicas - Part 1 #9042

Prevent broker scaledown if broker contains partition replicas - Part 1 #9042

Conversation

ShubhamRwt commented Aug 29, 2023 • edited

Type of change

Description

Checklist

scholzj left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

ShubhamRwt commented Sep 14, 2023

Choose a reason for hiding this comment

ShubhamRwt commented Sep 15, 2023

scholzj commented Sep 15, 2023

ShubhamRwt commented Sep 15, 2023

scholzj left a comment

Choose a reason for hiding this comment

scholzj commented Sep 15, 2023

azure-pipelines bot commented Sep 15, 2023

scholzj commented Sep 16, 2023

ShubhamRwt commented Sep 17, 2023

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

scholzj commented Sep 19, 2023

azure-pipelines bot commented Sep 19, 2023

scholzj commented Sep 19, 2023

ShubhamRwt commented Sep 20, 2023

ShubhamRwt commented Sep 20, 2023

im-konge commented Sep 20, 2023

henryZrncik commented Sep 20, 2023

im-konge commented Sep 20, 2023

im-konge commented Sep 20, 2023

azure-pipelines bot commented Sep 20, 2023

scholzj commented Sep 20, 2023

im-konge commented Sep 20, 2023

ShubhamRwt commented Sep 20, 2023

scholzj commented Sep 20, 2023

azure-pipelines bot commented Sep 20, 2023

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

scholzj commented Sep 21, 2023

azure-pipelines bot commented Sep 21, 2023

scholzj commented Sep 21, 2023

azure-pipelines bot commented Sep 21, 2023

scholzj commented Sep 21, 2023

azure-pipelines bot commented Sep 21, 2023

ShubhamRwt commented Aug 29, 2023 •

edited