Support Multiple controllers #167

SamBarker · 2023-08-23T03:31:05Z

Type of change

Bugfix
Enhancement

Description

Fixes the cluster configuration and start up so that multiple controller nodes work. Fixes: #165

Additional Context

I still see this as an intermediate step, there are kludges to detect if a node should be a controller or a broker or both. While they work it feels like we are missing a proper object model that knows what role a node has. We also lack the ability for users to control the cluster topology (i.e. they can't have nodes in controller only mode with while they they still have brokers to start)

Checklist

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

robobario · 2023-08-25T01:04:55Z

impl/src/test/java/io/kroxylicious/testing/kafka/common/KafkaClusterConfigTest.java

+
+    private void assertNodeIdHasRole(KafkaClusterConfig kafkaClusterConfig, int nodeId, String expectedRole) {
+        final var config = kafkaClusterConfig.generateConfigForSpecificNode(endpointConfig, nodeId);
+        assertThat(config.getProperties()).extracting(brokerConfig -> brokerConfig.get("process.roles")).as("nodeId: %s to have process.roles", nodeId).isEqualTo(


We should switch to using the kafka enums throughout in another PR.

Ohh that is a good thought

impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterFactory.java

impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterConfig.java

robobario · 2023-08-25T01:16:21Z

impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterFactory.java

@@ -93,6 +98,11 @@ public static KafkaCluster create(KafkaClusterConfig clusterConfig) {
        }
    }

+    private static KafkaClusterExecutionMode getExecutionMode(KafkaClusterConfig clusterConfig) {
+        return KafkaClusterExecutionMode.convertClusterExecutionMode(System.getenv().get(TEST_CLUSTER_EXECUTION_MODE),


not part of your changes but it looks a bit dangerous that it defaults it if the string is unknown, maybe better to explode with error if the user supplies value "CONTAINDERER"

robobario

LGTM, good to have all the language aligned on node/broker/controller thanks @SamBarker

Would be good to have another round on KafkaClusterConfig to try and move away from going from the configuration straight to server properties, passing those maps down to collect protocols/listeners/etc. Maybe there's some intermediate representation missing like: TestDefinition -> Nodes -> Properties where Nodes describes how we want the cluster to look.

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaCluster.java

tombentley · 2023-08-27T21:34:02Z

We should probably update the README and the javadocs on the @KRaftCluster annotation to explain how the numBrokers and numControllers relate to the cluster that gets created.

I also think that with this change you're not far from being able to add support for controlling whether we use combined or pure-controller nodes using some combinedMode attribute on @KRaftCluster. Maybe that's something we can do in a follow-up PR, WDYT?

SamBarker · 2023-08-27T21:49:53Z

We should probably update the README and the javadocs on the @KRaftCluster annotation to explain how the numBrokers and numControllers relate to the cluster that gets created.

Yeah thats a good point.

I also think that with this change you're not far from being able to add support for controlling whether we use combined or pure-controller nodes using some combinedMode attribute on @KRaftCluster. Maybe that's something we can do in a follow-up PR, WDYT?

Yeah I've questioned the use of combined mode throughout this work but I really don't want to try and tackle it as part of this PR. Partly because I wonder if at that point we want to move to a more sophisticated deployment model such as @KraftCluster(brokerIds = [1,2,4], controllerId=1,2,6]) but keep numBrokers/ numControllers as a fall back...

Signed-off-by: Sam Barker <[email protected]>

…ing comes from. Signed-off-by: Sam Barker <[email protected]>

Signed-off-by: Sam Barker <[email protected]>

When the build fails archiving the container logs also failed as the previous format contained `:`. Signed-off-by: Sam Barker <[email protected]>

Signed-off-by: Sam Barker <[email protected]>

Extracting methods in the process. Signed-off-by: Sam Barker <[email protected]>

Signed-off-by: Sam Barker <[email protected]>

…trollers in Kraft clusters. Signed-off-by: Sam Barker <[email protected]>

robobario · 2023-08-28T00:31:22Z

README.md

@@ -74,6 +74,17 @@ When multiple constraints are provided they will _all_ be satisfied.

 The cluster will be provisioned using the fastest available mechanism, because your development inner loop is a precious thing.

+## Provisioning topology
+
+In kraft cluster (the default) the extension will generate nodes using the `broker, controller` roles until it reaches the desired number of brokers or controllers (which ever is lowest) at which point it will continue deploying the remaining role.


In kraft cluster

Should we introduce some term like cluster mode [kraft, zookeeper]?

Hmm, my first draft locally I called it kraft mode but thought a kraft cluster aligned better with the use of the KraftCluster annotation.

The mode terminology would be extension specific as Apache Kafka talks about KRaft clusters IIRC

Signed-off-by: Sam Barker <[email protected]>

README.md

Co-authored-by: Robert Young <[email protected]> Signed-off-by: Sam Barker <[email protected]>

sonarcloud · 2023-08-28T01:30:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.0% Duplication

robobario

LGTM

SamBarker force-pushed the multipleControllers branch from 285431a to 90bdc01 Compare August 24, 2023 23:33

robobario reviewed Aug 25, 2023

View reviewed changes

impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterFactory.java Outdated Show resolved Hide resolved

robobario reviewed Aug 25, 2023

View reviewed changes

impl/src/main/java/io/kroxylicious/testing/kafka/common/KafkaClusterConfig.java Show resolved Hide resolved

robobario reviewed Aug 25, 2023

View reviewed changes

robobario approved these changes Aug 25, 2023

View reviewed changes

SamBarker force-pushed the multipleControllers branch from 9c78f66 to d3c045b Compare August 25, 2023 04:11

tombentley reviewed Aug 27, 2023

View reviewed changes

impl/src/main/java/io/kroxylicious/testing/kafka/testcontainers/TestcontainersKafkaCluster.java Outdated Show resolved Hide resolved

SamBarker added 20 commits August 28, 2023 10:29

Update workflow step name

7771636

Signed-off-by: Sam Barker <[email protected]>

Fix typo in pom description.

567a7f9

Signed-off-by: Sam Barker <[email protected]>

Remove un-necessary public modifiers

e6e2c0f

Signed-off-by: Sam Barker <[email protected]>

refactor generateConfigForSpecificNode

f6ac4cb

Signed-off-by: Sam Barker <[email protected]>

Ensure we can generate the required config for controllers

a8a0005

Signed-off-by: Sam Barker <[email protected]>

Include thread in default pattern as it helps identify where the logg…

ef0fb28

…ing comes from. Signed-off-by: Sam Barker <[email protected]>

Support multiple controllers in kraft mode.

3c7457d

Signed-off-by: Sam Barker <[email protected]>

Update the time format used for test names.

047c08b

When the build fails archiving the container logs also failed as the previous format contained `:`. Signed-off-by: Sam Barker <[email protected]>

Squash sonar warnings

9752845

Signed-off-by: Sam Barker <[email protected]>

Add role to configHolder

121299b

Signed-off-by: Sam Barker <[email protected]>

Try to detect configurations we cannot satisfy and fail fast

6e07b0d

Signed-off-by: Sam Barker <[email protected]>

Only execute the additional controllers test in IN_VM mode.

c2440e7

Signed-off-by: Sam Barker <[email protected]>

Include cluster mode in the exception.

e0ccdbf

Signed-off-by: Sam Barker <[email protected]>

Remove controller port from ConfigHolder.

1eb6769

Extracting methods in the process. Signed-off-by: Sam Barker <[email protected]>

Simplify determineRole.

63cf726

Signed-off-by: Sam Barker <[email protected]>

Extract constants for listener names

daa07fd

Signed-off-by: Sam Barker <[email protected]>

Support adding brokers to the topology.

cb8cbb0

Signed-off-by: Sam Barker <[email protected]>

revert change to QUARKUS_LOG_LEVEL in TestcontainersKafkaCluster

f16cc6d

Signed-off-by: Sam Barker <[email protected]>

drop log level for container logs location to debug

ec633bd

Signed-off-by: Sam Barker <[email protected]>

fight the formatter round 33989

bb44c07

Signed-off-by: Sam Barker <[email protected]>

SamBarker added 3 commits August 28, 2023 10:29

Swap package and copyright header

ba2cb59

Signed-off-by: Sam Barker <[email protected]>

Removed commented out logging.

8be2a6c

Signed-off-by: Sam Barker <[email protected]>

Update docs to describe the interaction between numBrokers and numCon…

05d6fb6

…trollers in Kraft clusters. Signed-off-by: Sam Barker <[email protected]>

SamBarker force-pushed the multipleControllers branch from 16bf56a to 05d6fb6 Compare August 27, 2023 23:42

robobario reviewed Aug 28, 2023

View reviewed changes

SamBarker added 2 commits August 28, 2023 12:35

Update javadoc tables

844e711

Signed-off-by: Sam Barker <[email protected]>

fix readme typo

09cde67

Signed-off-by: Sam Barker <[email protected]>

robobario reviewed Aug 28, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

Reword node toplogy section

d3ac926

Co-authored-by: Robert Young <[email protected]> Signed-off-by: Sam Barker <[email protected]>

robobario approved these changes Aug 28, 2023

View reviewed changes

SamBarker merged commit 1418b32 into kroxylicious:main Aug 28, 2023
3 checks passed

SamBarker deleted the multipleControllers branch August 28, 2023 01:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Multiple controllers #167

Support Multiple controllers #167

SamBarker commented Aug 23, 2023

robobario Aug 25, 2023

SamBarker Aug 25, 2023

robobario Aug 25, 2023

robobario left a comment

tombentley commented Aug 27, 2023

SamBarker commented Aug 27, 2023

robobario Aug 28, 2023

SamBarker Aug 28, 2023

sonarcloud bot commented Aug 28, 2023

robobario left a comment

Support Multiple controllers #167

Support Multiple controllers #167

Conversation

SamBarker commented Aug 23, 2023

Type of change

Description

Additional Context

Checklist

robobario Aug 25, 2023

Choose a reason for hiding this comment

SamBarker Aug 25, 2023

Choose a reason for hiding this comment

robobario Aug 25, 2023

Choose a reason for hiding this comment

robobario left a comment

Choose a reason for hiding this comment

tombentley commented Aug 27, 2023

SamBarker commented Aug 27, 2023

robobario Aug 28, 2023

Choose a reason for hiding this comment

SamBarker Aug 28, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Aug 28, 2023

robobario left a comment

Choose a reason for hiding this comment