Add allocateLoadBalancerNodePorts configuration to listeners configuration #10792

venkatesh2090 · 2024-11-03T14:41:35Z

Type of change

Enhancement / new feature

Description

Enabling users to configure spec.allocateLoadBalancerNodePorts in an external LoadBalancer type Service by providing an option in GenericKafkaListenerConfiguration.

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

Signed-off-by: Venkatesh Kannan <[email protected]>

@pillai-ashwin

Credit: @pillai-ashwin Signed-off-by: Venkatesh Kannan <[email protected]>

Signed-off-by: Venkatesh Kannan <[email protected]>

scholzj · 2024-11-03T20:47:35Z

...c/main/java/io/strimzi/api/kafka/model/kafka/listener/GenericKafkaListenerConfiguration.java

@@ -58,6 +58,7 @@ public class GenericKafkaListenerConfiguration implements UnknownPropertyPreserv
    private String hostTemplate;
    private String advertisedHostTemplate;
    private Map<String, Object> additionalProperties;
+    private Boolean allocateLoadBalancerNodePorts = true;


If we don't leave this as null, should it be boolean instead? Or maybe you should keep it as Boolean, have it default to null and then in the code check if it is set which might be better approach.

Thanks for the review. Yes I will keep it null and change the generate functions to check for null.

scholzj

Thanks for the PR. You will also need to write some unit tests for this before we can merge it. The ListenerUtils and ListenersValidators have their own, we should probably have it also in the KafkaCluster tests.

Signed-off-by: Venkatesh Kannan <[email protected]>

venkatesh2090 · 2024-11-05T00:12:09Z

I have pushed some tests, but the tests in KafkaClusterTest are seemingly incomplete. The testGenerateExternalBootstrapService and testGeneratePerPodService is supposed to assert other fields but I don't have enough context to add those. Moreover I am not sure if it even belongs in this PR. I have only added asserts for the allocateLoadBalancerNodePorts field.

scholzj · 2024-11-05T04:11:16Z

cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaClusterTest.java

+                            new GenericKafkaListenerBuilder()
+                                    .withName("lb")
+                                    .withPort(9094)
+                                    .withType(KafkaListenerType.LOADBALANCER)
+                                    .withNewConfiguration()
+                                        .withAllocateLoadBalancerNodePorts(false)
+                                    .endConfiguration()
                                    .build())


I don't think you should add it here and have it in all tests. Add it only to the Kafka CR used in the unit test focusing on this.

scholzj · 2024-11-05T04:12:50Z

cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaClusterTest.java

+    @ParallelTest
+    public void testGenerateExternalBootstrapServices() {
+        List<Service> services = KC.generateExternalBootstrapServices();
+
+        assertThat(services, hasSize(1));
+        Service service = services.get(0);
+
+        assertThat(service.getSpec().getAllocateLoadBalancerNodePorts(), is(false));
+    }
+
+    @ParallelTest
+    public void testGeneratePerPodServices() {
+        List<Service> services = KC.generatePerPodServices();
+
+        assertThat(services, hasSize(5));
+        assertThat(services.stream()
+                        .filter(s -> !s.getSpec().getAllocateLoadBalancerNodePorts())
+                        .collect(Collectors.toList()),
+                hasSize(5));
+    }


I think this works. Maybe you can have a single unit test to cover both and you should have the listener configured for this in this method and not in the static resource. Then it would not be needed to include it in all the other tests.

And in one of the generic tests for the services you should also check that the value is not set in other cases.

I have moved this test to KafkaClusterListenerTest like you suggested. It creates another KafkaCluster instance instead of using the static one

scholzj · 2024-11-05T04:14:36Z

cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaClusterTest.java

I'm not sure if I suggested to add it to this class ... but it would make more sense in KafkaClusterListenersTest.

scholzj · 2024-11-05T04:16:50Z

cluster-operator/src/test/java/io/strimzi/operator/cluster/model/ListenersUtilsTest.java

+        assertThat(internalListeners.stream()
+                        .filter(l -> ListenersUtils.allocateLoadBalancerNodePorts(l) == null ||
+                                     ListenersUtils.allocateLoadBalancerNodePorts(l))
+                        .collect(Collectors.toList()),
+                hasSize(4));
+        assertThat(allListeners.stream()
+                        .filter(l -> ListenersUtils.allocateLoadBalancerNodePorts(l) == null ||
+                                ListenersUtils.allocateLoadBalancerNodePorts(l))
+                        .collect(Collectors.toList()),
+                hasSize(13));


To be honest, I have no idea what are these tests trying to do. I would just create 3 listeners here - one with the flag set to true, one with it set to false and one with it not set. And just call the method on them and assert the result.

addressed this as well. Thanks

Signed-off-by: Venkatesh Kannan <[email protected]>

scholzj · 2024-11-06T04:57:15Z

/azp run regression

azure-pipelines · 2024-11-06T04:57:27Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj

LGTM. Thanks.

ppatierno

LGTM. Just one nit.

ppatierno · 2024-11-06T08:15:40Z

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

+            return listener.getConfiguration().getAllocateLoadBalancerNodePorts();
+        } else {
+            return true;
+        }


why not just ....

return listener.getConfiguration() != null ? listener.getConfiguration().getAllocateLoadBalancerNodePorts() : true;

No particular reason. I just copied what the other methods were doing. I was going to return null, so the Utils method takes over with the null check. But one of the bug check CI pipelines caught it as a bug.

I think @ppatierno highlights what I missed ... it should probably return null (not configured) and not true. Whether it is in the longer code or shorter does not matter that much for me.

Just ignored it in spotbugs. Hope that's alright

Yeah, that is fine.

Signed-off-by: Venkatesh Kannan <[email protected]>

scholzj · 2024-11-08T04:24:18Z

/azp run regression

azure-pipelines · 2024-11-08T04:24:30Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-11-08T04:24:46Z

I started the regression tests and if they pass we can merge it.

venkatesh2090 · 2024-11-08T19:59:14Z

Is it possible for the regression tests to fail intermittently? I ran the failing test CruiseControlST locally with a Kind cluster and it didn't fail for me. Wondering if you could rerun those failing pipelines? I'm guessing I don't have permissions for obvious reasons.

This was the command I used, FYI

DOCKER_TAG=0.44.1 \
DOCKER_REGISTRY=localhost:5001 \
ST_KAFKA_VERSION=3.8.0 \
KAFKA_VERSION=3.8.0 \
STRIMZI_USE_KRAFT_IN_TESTS=true \
STRIMZI_USE_NODE_POOLS_IN_TESTS=true \
STRIMZI_RBAC_SCOPE=CLUSTER \
BRIDGE_IMAGE=latest-released \
mvn \
-DexcludeGroups=flaky,loadbalancer,networkpolicies \
-Dmaven.javadoc.skip=true \
-Dfailsafe.rerunFailingTestsCount=2 \
-Djunit.jupiter.execution.parallel.enabled=1 \
-Dskip.surefire.tests \
-f systemtest/pom.xml \
-DskipTests=false \
-Dgroups=regression,kafkasmoke \
-Dit.test='cruisecontrol/CruiseControlST' \
verify

scholzj · 2024-11-09T01:40:22Z

It could definitely be flaky. I restarted it and let's see how it goes now. I think it is unlikely that you really broke anything here.

scholzj · 2024-11-10T02:31:11Z

Thanks for the PR!

Add allocateLoadBalancerNodePorts configuration to listeners

12b80ac

Signed-off-by: Venkatesh Kannan <[email protected]>

venkatesh2090 force-pushed the feat/allocateLoadBalancerNodePorts branch from 858a79f to 12b80ac Compare November 3, 2024 14:42

venkatesh2090 marked this pull request as draft November 3, 2024 15:44

venkatesh2090 added 2 commits November 3, 2024 15:46

Add validator to allocateLoadBalancerNodePorts

4b4e598

Credit: @pillai-ashwin Signed-off-by: Venkatesh Kannan <[email protected]>

Fix CI

22a9685

Signed-off-by: Venkatesh Kannan <[email protected]>

venkatesh2090 force-pushed the feat/allocateLoadBalancerNodePorts branch from 7f56f0d to 22a9685 Compare November 3, 2024 15:47

venkatesh2090 mentioned this pull request Nov 3, 2024

[Enhancement]: support allocateLoadBalancerNodePorts parameter to enable/disable dynamic nodeport creation for type LoadBalancer in Strimzi Kafka #8782

Closed

venkatesh2090 added 2 commits November 3, 2024 16:05

Also update per pod services

9983354

Signed-off-by: Venkatesh Kannan <[email protected]>

Default allocateLoadBalancerNodePorts to true

1d501b8

Signed-off-by: Venkatesh Kannan <[email protected]>

venkatesh2090 force-pushed the feat/allocateLoadBalancerNodePorts branch from 9fd38e2 to 1d501b8 Compare November 3, 2024 17:19

scholzj reviewed Nov 3, 2024

View reviewed changes

scholzj added this to the 0.45.0 milestone Nov 3, 2024

venkatesh2090 added 2 commits November 3, 2024 21:13

use boxed boolean to make it nullable

7437071

Signed-off-by: Venkatesh Kannan <[email protected]>

Add tests for allocateLoadBalancerNodePorts

9d8b57e

Signed-off-by: Venkatesh Kannan <[email protected]>

scholzj reviewed Nov 5, 2024

View reviewed changes

addressed comments

e91629e

Signed-off-by: Venkatesh Kannan <[email protected]>

venkatesh2090 marked this pull request as ready for review November 5, 2024 20:18

scholzj linked an issue Nov 6, 2024 that may be closed by this pull request

[Enhancement]: support allocateLoadBalancerNodePorts parameter to enable/disable dynamic nodeport creation for type LoadBalancer in Strimzi Kafka #8782

Closed

scholzj approved these changes Nov 6, 2024

View reviewed changes

scholzj requested a review from ppatierno November 6, 2024 04:57

ppatierno approved these changes Nov 6, 2024

View reviewed changes

Ignore boolean null check in spotbugs

225bc90

Signed-off-by: Venkatesh Kannan <[email protected]>

venkatesh2090 force-pushed the feat/allocateLoadBalancerNodePorts branch from 869ba8b to 225bc90 Compare November 7, 2024 22:01

Update tests

e8f76c3

Signed-off-by: Venkatesh Kannan <[email protected]>

scholzj merged commit c0f4a0f into strimzi:main Nov 10, 2024
21 checks passed

venkatesh2090 deleted the feat/allocateLoadBalancerNodePorts branch November 10, 2024 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add allocateLoadBalancerNodePorts configuration to listeners configuration #10792

Add allocateLoadBalancerNodePorts configuration to listeners configuration #10792

venkatesh2090 commented Nov 3, 2024 •

edited

Loading

scholzj Nov 3, 2024

venkatesh2090 Nov 3, 2024

scholzj left a comment •

edited

Loading

venkatesh2090 commented Nov 5, 2024

scholzj Nov 5, 2024

scholzj Nov 5, 2024

venkatesh2090 Nov 5, 2024

scholzj Nov 5, 2024

scholzj Nov 5, 2024

venkatesh2090 Nov 5, 2024

scholzj commented Nov 6, 2024

azure-pipelines bot commented Nov 6, 2024

scholzj left a comment

ppatierno left a comment

ppatierno Nov 6, 2024

venkatesh2090 Nov 6, 2024

scholzj Nov 7, 2024

venkatesh2090 Nov 7, 2024

scholzj Nov 8, 2024

scholzj commented Nov 8, 2024

azure-pipelines bot commented Nov 8, 2024

scholzj commented Nov 8, 2024

venkatesh2090 commented Nov 8, 2024

scholzj commented Nov 9, 2024 •

edited

Loading

scholzj commented Nov 10, 2024

Add allocateLoadBalancerNodePorts configuration to listeners configuration #10792

Add allocateLoadBalancerNodePorts configuration to listeners configuration #10792

Conversation

venkatesh2090 commented Nov 3, 2024 • edited Loading

Type of change

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scholzj left a comment • edited Loading

Choose a reason for hiding this comment

venkatesh2090 commented Nov 5, 2024

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 Nov 6, 2024

azure-pipelines bot commented Nov 6, 2024

scholzj left a comment

Choose a reason for hiding this comment

ppatierno 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

scholzj commented Nov 8, 2024

azure-pipelines bot commented Nov 8, 2024

scholzj commented Nov 8, 2024

venkatesh2090 commented Nov 8, 2024

scholzj commented Nov 9, 2024 • edited Loading

scholzj commented Nov 10, 2024

venkatesh2090 commented Nov 3, 2024 •

edited

Loading

scholzj left a comment •

edited

Loading

scholzj commented Nov 9, 2024 •

edited

Loading