Move cruise control server, capacity and server logging metrics config to ConfigMap #8977

Thealisyed · 2023-08-11T14:47:37Z

Type of change

Enhancement / new feature

Description

Moves cruise control capacity config to ConfigMap
Resolves part of this #8337

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

kyguy

Great start @Thealisyed, left some comments, be careful with the IDE auto indenting code in files, it adds noise to the diff. Please go through an undo all of the unnecessary indentation changes, I have marked some of them but not all

kyguy · 2023-08-24T15:44:18Z

api/src/main/java/io/strimzi/api/kafka/model/CruiseControlResources.java

}
+

Remove this newline change, it adds noise to the diff

kyguy · 2023-08-24T15:46:52Z

api/src/main/java/io/strimzi/api/kafka/model/CruiseControlResources.java

@@ -93,4 +93,14 @@ public static String logAndMetricsConfigMapName(String clusterName) {
    public static String networkPolicyName(String clusterName) {
        return clusterName + "-network-policy-cruise-control";
    }
+
+    /**
+     * Returns the name of the Cruise Control Broker Capacity ConfigMap Name {@code ConfigMap} for a {@code Kafka} cluster of the given name.


Suggested change

* Returns the name of the Cruise Control Broker Capacity ConfigMap Name {@code ConfigMap} for a {@code Kafka} cluster of the given name.

* Returns the name of the Cruise Control Broker capacity {@code ConfigMap} for a {@code Kafka} cluster of the given name.

kyguy · 2023-08-24T15:47:16Z

api/src/main/java/io/strimzi/api/kafka/model/CruiseControlResources.java

+    /**
+     * Returns the name of the Cruise Control Broker Capacity ConfigMap Name {@code ConfigMap} for a {@code Kafka} cluster of the given name.
+     * @param clusterName  The {@code metadata.name} of the {@code Kafka} resource.
+     * @return The name of the corresponding Cruise Control log {@code ConfigMap}.


Suggested change

* @return The name of the corresponding Cruise Control log {@code ConfigMap}.

* @return The name of the corresponding Cruise Control capacity {@code ConfigMap}.

kyguy · 2023-08-24T15:47:56Z

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

+    protected static final String CRUISE_CONTROL_CAPACITY_CONFIGURATION_VOLUME_MOUNT = "/tmp1";
+    protected static final String DEFAULT_CAPACITY_CONFIG_FILE_CONFIG =
+            CRUISE_CONTROL_CAPACITY_CONFIGURATION_VOLUME_MOUNT + "/" +
+                    CRUISE_CONTROL_CAPACITY_CONFIGURATION_VOLUME_NAME;


Can 103 and 104 be aligned?

kyguy · 2023-08-24T15:48:10Z

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

@@ -145,6 +149,7 @@ public class CruiseControl extends AbstractModel implements SupportsMetrics, Sup
    private InternalServiceTemplate templateService;

    private static final Map<String, String> DEFAULT_POD_LABELS = new HashMap<>();
+


Undo this newline change, it adds noise to the diff

kyguy · 2023-08-24T16:25:30Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

+                                .addToLimits("memory", new Quantity("300Mi"))
+                                .addToRequests("memory", new Quantity("300Mi"))
+                                .build())
+                        .withNewJvmOptions()


undo indentation

kyguy · 2023-08-24T16:25:43Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

+                        .endJvmOptions()
+                        .endCruiseControl()
+                        .endSpec()
+                        .build());


undo intentation

kyguy · 2023-08-24T16:25:54Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

+                        .build());
+
+        String ccPodName = kubeClient().listPodsByPrefixInName(clusterOperator.getDeploymentNamespace(),
+                CruiseControlResources.deploymentName(clusterName)).get(0).getMetadata().getName();


fix indentation

kyguy · 2023-08-24T16:26:29Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

+                        .endBrokerCapacity()
+                        .endCruiseControl()
+                        .endSpec()
+                        .build());


Fix indentation

kyguy · 2023-08-24T16:28:58Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

+                JsonNode cpuEntry = capacity.get("CPU");
+                System.out.println("Expected: " + cpu + " Actual: " + cpuEntry.get("num.cores") + " TEST PASSED " + cpu.equals(cpuEntry.get("num.cores")));
+                JsonNode numCores = cpuEntry.get("num.cores");
+                System.out.println("Num Cores: " + numCores);


Remove the print statements; add assert statements

kyguy · 2023-08-28T14:25:59Z

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

@@ -97,7 +97,11 @@ public class CruiseControl extends AbstractModel implements SupportsMetrics, Sup
    protected static final String LOG_AND_METRICS_CONFIG_VOLUME_MOUNT = "/opt/cruise-control/custom-config/";
    protected static final String API_AUTH_CONFIG_VOLUME_NAME = "api-auth-config";
    protected static final String API_AUTH_CONFIG_VOLUME_MOUNT = "/opt/cruise-control/api-auth-config/";
-
+    protected static final String CRUISE_CONTROL_CAPACITY_CONFIG_VOLUME_NAME = "capacity-json";


Let's match the name of the original file, capacity.json, if possible

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

kyguy · 2023-08-28T14:38:17Z

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

+    /**
+     * Generates data for broker capacity ConfigMap
+     *
+     * @param capacity assigned capacity parameter for broker capacity configmap data


Suggested change

* @param capacity assigned capacity parameter for broker capacity configmap data

* @param capacity data used for broker capacity configuration

kyguy · 2023-08-28T14:39:06Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/cruisecontrol/Capacity.java

- *   "brokerCapacities":[
- *     {
+ * "brokerCapacities":[
+ *      {


kyguy · 2023-08-28T14:49:54Z

...i/operator/cluster/operator/resource/cruisecontrol/CruiseControlConfigurationParameters.java

@@ -83,6 +83,12 @@ public enum CruiseControlConfigurationParameters {
     */
    METRIC_REPORTER_TOPIC_NAME("metric.reporter.topic"),

+    /**
+     * Name of the Cruise Control metrics topic


Suggested change

* Name of the Cruise Control metrics topic

* Name of the Cruise Control capacity config file path

kyguy · 2023-08-28T14:53:15Z

...i/operator/cluster/operator/resource/cruisecontrol/CruiseControlConfigurationParameters.java

+    /**
+     * Name of the Cruise Control capacity file
+     */
+    public static final String DEFAULT_CRUISE_CONTROL_CAPACITY_CONFIG_NAME = "/tmp1/capacity-json";


I think we define this path name in the CruiseControl.java class, create this value from the variables defined in the CruiseControl.java class? or vice versa? Just want to make sure this path name is defined in one place

kyguy · 2023-08-28T14:53:57Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

+                .endMetadata()
+                .editOrNewSpec()
+                    .editKafka()
+                        .addToConfig("auto.create.topics.enable", "false")


undo indentation

kyguy · 2023-08-28T14:54:09Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

+                .endJvmOptions()
+            .endCruiseControl()
+        .endSpec()
+        .build());


undo indentation changes

kyguy · 2023-08-28T14:56:16Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

        }

-        RollingUpdateUtils.waitForComponentScaleUpOrDown(testStorage.getNamespaceName(), testStorage.getKafkaSelector(), initialReplicas);
+        RollingUpdateUtils.waitForComponentScaleUpOrDown(testStorage.getNamespaceName(), testStorage.getKafkaSelector(),
+                initialReplicas);
    }

    @BeforeAll


Undo all the changes here that are no related to the testAutoCreationOfCruiseControlCapacityConfigMap test method i.e. undo changes to all other test methods

kyguy · 2023-08-29T15:14:51Z

...i/operator/cluster/operator/resource/cruisecontrol/CruiseControlConfigurationParameters.java

+    /**
+     * Name of the Cruise Control capacity file
+     */
+    public static final String DEFAULT_CRUISE_CONTROL_CAPACITY_CONFIG_NAME = "/tmp/capacity-json";


Watch the value here, we should define this value once, either here of in the CruiseControl.java class

kyguy · 2023-08-29T15:15:47Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java


    @IsolatedTest
    @KRaftWithoutUTONotSupported
    @UTONotSupported("https://github.com/strimzi/strimzi-kafka-operator/issues/8864")
    void testAutoCreationOfCruiseControlTopicsWithResources(ExtensionContext extensionContext) {
        final String clusterName = mapWithClusterNames.get(extensionContext.getDisplayName());

+


extra line?

kyguy · 2023-08-29T15:16:23Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

@@ -121,11 +127,11 @@ void testAutoCreationOfCruiseControlTopicsWithResources(ExtensionContext extensi
            .withName(CRUISE_CONTROL_METRICS_TOPIC).get().getSpec();

        KafkaTopicUtils.waitForKafkaTopicReady(Constants.TEST_SUITE_NAMESPACE, CRUISE_CONTROL_MODEL_TRAINING_SAMPLES_TOPIC);
-        KafkaTopicSpec modelTrainingTopic = KafkaTopicResource.kafkaTopicClient().inNamespace(Constants.TEST_SUITE_NAMESPACE)
+        KafkaTopicSpec modelTrainingTopic = KafkaTopicResource.kafkaTopicClient().inNamespace(clusterOperator.getDeploymentNamespace())


Are these changes necessary?

kyguy · 2023-08-29T15:16:30Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

-        KafkaTopicUtils.waitForKafkaTopicReady(Constants.TEST_SUITE_NAMESPACE, CRUISE_CONTROL_PARTITION_METRICS_SAMPLES_TOPIC);
-        KafkaTopicSpec partitionMetricsTopic = KafkaTopicResource.kafkaTopicClient().inNamespace(Constants.TEST_SUITE_NAMESPACE)
+        KafkaTopicUtils.waitForKafkaTopicReady(clusterOperator.getDeploymentNamespace(), CRUISE_CONTROL_PARTITION_METRICS_SAMPLES_TOPIC);
+        KafkaTopicSpec partitionMetricsTopic = KafkaTopicResource.kafkaTopicClient().inNamespace(clusterOperator.getDeploymentNamespace())


kyguy · 2023-08-29T15:16:35Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

            .withName(CRUISE_CONTROL_MODEL_TRAINING_SAMPLES_TOPIC).get().getSpec();

-        KafkaTopicUtils.waitForKafkaTopicReady(Constants.TEST_SUITE_NAMESPACE, CRUISE_CONTROL_PARTITION_METRICS_SAMPLES_TOPIC);
-        KafkaTopicSpec partitionMetricsTopic = KafkaTopicResource.kafkaTopicClient().inNamespace(Constants.TEST_SUITE_NAMESPACE)
+        KafkaTopicUtils.waitForKafkaTopicReady(clusterOperator.getDeploymentNamespace(), CRUISE_CONTROL_PARTITION_METRICS_SAMPLES_TOPIC);


kyguy · 2023-08-29T15:16:56Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

@@ -162,7 +220,7 @@ void testCruiseControlWithApiSecurityDisabled(ExtensionContext extensionContext)
            .endMetadata()
            .build());

-        KafkaRebalanceUtils.waitForKafkaRebalanceCustomResourceState(Constants.TEST_SUITE_NAMESPACE, clusterName, KafkaRebalanceState.ProposalReady);
+        KafkaRebalanceUtils.waitForKafkaRebalanceCustomResourceState(clusterOperator.getDeploymentNamespace(), clusterName, KafkaRebalanceState.ProposalReady);


undo changes

kyguy · 2023-08-29T15:17:28Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

-        if (Environment.isKafkaNodePoolsEnabled()) {
-            KafkaNodePoolResource.replaceKafkaNodePoolResourceInSpecificNamespace(testStorage.getKafkaNodePoolName(), knp ->
-                knp.getSpec().setReplicas(initialReplicas), testStorage.getNamespaceName());
+        if (Environment.isKafkaNodePoolsEnabled()) {KafkaNodePoolResource.replaceKafkaNodePoolResourceInSpecificNamespace(testStorage.getKafkaNodePoolName(), knp -> knp.getSpec().setReplicas(initialReplicas), testStorage.getNamespaceName());
        } else {
            KafkaResource.replaceKafkaResourceInSpecificNamespace(testStorage.getClusterName(), kafka -> kafka.getSpec().getKafka().setReplicas(initialReplicas), testStorage.getNamespaceName());
        }


Undo changes here that are unrelated to the testAutoCreationOfCruiseControlCapacityConfigMap class

kyguy · 2023-08-29T15:35:39Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

+                .endBrokerCapacity()
+                .endCruiseControl()
+                .endSpec()
+                .build());


indentation

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

kyguy · 2023-08-30T14:39:57Z

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

+     * @param versions                  Supported Kafka versions
+     * @param kafkaBrokerNodes          List of the nodes which are part of the Kafka cluster
+     * @param kafkaStorage              A map with storage configuration used by the Kafka cluster and its node pools
+     * @param kafkaBrokerResources      A map with resource configuration used by the Kafka cluster and its node pools


Why this changes

kyguy · 2023-08-30T14:40:43Z

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

@@ -942,7 +942,7 @@ public void testCruiseControlContainerSecurityContext() {
                .withAllowPrivilegeEscalation(false)
                .withRunAsNonRoot(true)
                .withNewCapabilities()
-                    .addToDrop("ALL")
+                .addToDrop("ALL")


indentation

kyguy · 2023-08-30T14:41:08Z

...src/test/java/io/strimzi/operator/cluster/operator/assembly/CruiseControlReconcilerTest.java

@@ -102,7 +103,7 @@ public void reconcileEnabledCruiseControl(VertxTestContext context) {

        Kafka kafka = new KafkaBuilder(ResourceUtils.createKafka(NAMESPACE, NAME, 3, "foo", 120, 30))
                .editSpec()
-                    .withCruiseControl(cruiseControlSpec)
+                .withCruiseControl(cruiseControlSpec)


indentation

scholzj · 2023-08-11T14:56:48Z

client-examples

Wrong commit?

scholzj · 2023-08-30T15:34:18Z

api/src/main/java/io/strimzi/api/kafka/model/CruiseControlResources.java

+     * @return The name of the corresponding Cruise Control capacity {@code ConfigMap}.
+     */
+    public static String brokerCapacityConfigMapName(String clusterName) {
+        return clusterName + "-cruise-control-capacity-config";


I would use the same name as the Cruise Control deployment has. Or actually, why not use the existing Config Map we already have?

I think a single config map does not scale well in this case, also considering the 1MB limit, so I like this approach.

It was until now in an environment variable and nobody complained. So I do not think 1MB limit would be an issue.

That's true, but I still think it's a good idea to use a dedicated config map, as these configuration files can be quite big. Additionally, logAndMetricsConfigMapName would need to be renamed if we decide to go with one config map for all kind of configurations.

The config map name is <cluster-name>-cruise-control-config. So I do not think you need to rename it. I think it has like 1kB of size today in typical environment.

I was talking about the method name.

scholzj · 2023-08-30T15:35:47Z

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

@@ -95,7 +95,15 @@ public class CruiseControl extends AbstractModel implements SupportsMetrics, Sup
    protected static final String LOG_AND_METRICS_CONFIG_VOLUME_MOUNT = "/opt/cruise-control/custom-config/";
    protected static final String API_AUTH_CONFIG_VOLUME_NAME = "api-auth-config";
    protected static final String API_AUTH_CONFIG_VOLUME_MOUNT = "/opt/cruise-control/api-auth-config/";
-
+    protected static final String CRUISE_CONTROL_CAPACITY_CONFIG_VOLUME_NAME = "capacity.json";
+    protected static final String CRUISE_CONTROL_CAPACITY_CONFIG_VOLUME_MOUNT = "/tmp";


Assuming we keep separate config map with separate mount, it should not mount to /tmp but to something like the other config maps.

scholzj · 2023-08-30T15:36:45Z

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

@@ -158,8 +165,7 @@ public class CruiseControl extends AbstractModel implements SupportsMetrics, Sup
     * Constructor
     *
     * @param reconciliation The reconciliation
-     * @param resource  Kubernetes resource with metadata containing the namespace and cluster name
-     * @param sharedEnvironmentProvider Shared environment provider
+     * @param resource       Kubernetes resource with metadata containing the namespace and cluster name


Why was the sharedEnvironmentProvider removed from the Javadoc? That does not seem right.

scholzj · 2023-08-30T15:37:18Z

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

+     * @param kafkaBrokerNodes          List of the broker nodes which are part of the Kafka cluster
+     * @param kafkaStorage              A map with storage configuration used by the Kafka cluster and its node pools
+     * @param kafkaBrokerResources      A map with resource configuration used by the Kafka cluster and its broker pools
+     * @param sharedEnvironmentProvider A Interface to be implemented for returning an instance of the environment variables shared by all containers.


Why not keep the original Javadoc?

I think the indentation part might have happened automatically by the IDE. @Thealisyed I guees you can revert the indentation and keep the things same as they were earlier. Not sure why you edited the javadoc for sharedEnvironmentProvider

scholzj · 2023-08-30T15:37:56Z

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

+     * @param isOpenShift      Flag indicating if we are on OpenShift or not
+     * @param imagePullPolicy  Image pull policy
+     * @param imagePullSecrets Image pull secrets
     *
-     * @return  Cruise Control Kubernetes Deployment
+     * @return Cruise Control Kubernetes Deployment


No need to change the indentation.

scholzj · 2023-08-30T15:38:21Z

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

@@ -389,12 +395,12 @@ protected List<EnvVar> getEnvVars() {
        varList.add(ContainerUtils.createEnvVar(ENV_VAR_STRIMZI_KAFKA_GC_LOG_ENABLED, String.valueOf(gcLoggingEnabled)));
        varList.add(ContainerUtils.createEnvVar(ENV_VAR_MIN_INSYNC_REPLICAS, String.valueOf(minInsyncReplicas)));

-        varList.add(ContainerUtils.createEnvVar(ENV_VAR_CRUISE_CONTROL_CAPACITY_CONFIGURATION, capacity.toString()));
+        //varList.add(ContainerUtils.createEnvVar(ENV_VAR_CRUISE_CONTROL_CAPACITY_CONFIGURATION, capacity.toString()));


Should this be deleted? And should the ENV_VAR_CRUISE_CONTROL_CAPACITY_CONFIGURATION field be removed as well?

scholzj · 2023-08-30T15:39:36Z

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

+     * @param namespace                         Namespace in which the Cruise Control cluster runs
+     * @param kafkaName                         Name of the Kafka cluster (it is used for the SANs in the certificate)
+     * @param clusterCa                         The cluster CA.
     * @param isMaintenanceTimeWindowsSatisfied Indicates whether we are in the maintenance window or not.
-     *                                          This is used for certificate renewals
-     *
+     * This is used for certificate renewals


No need to change the indentation here or in the other places. I will stop repeating it, but it applies below as well.

scholzj · 2023-08-30T15:43:05Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

@@ -142,6 +146,52 @@ void testAutoCreationOfCruiseControlTopicsWithResources(ExtensionContext extensi
        assertThat(partitionMetricsTopic.getReplicas(), is(2));
    }

+    @IsolatedTest
+    @KRaftNotSupported("Topic Operator is not supported by KRaft mode and is used in this test class")


Why does this use Topic Operator? And maybe a bit more general, why does this need a separate System test? I would think that:

Unit tests should be sufficient to cover this

If we want a system test, it can be probably added to one of the existing tests?

scholzj · 2023-08-30T15:43:24Z

systemtest/src/test/java/io/strimzi/systemtest/cruisecontrol/CruiseControlST.java

+                assertThat(nwThroughputOutbound, is(capacity.get("NW_OUT")));
+            }
+        } catch (Exception e) {
+            e.printStackTrace();


Should this fail the test instead of printing the stack trace?

fvaleri

Thanks for this PR. I left a couple of comments, but first you need to fix the build error.

fvaleri · 2023-08-31T09:20:26Z

api/src/main/java/io/strimzi/api/kafka/model/CruiseControlResources.java

+     * @return The name of the corresponding Cruise Control capacity {@code ConfigMap}.
+     */
+    public static String brokerCapacityConfigMapName(String clusterName) {
+        return clusterName + "-cruise-control-capacity-config";


I think a single config map does not scale well in this case, also considering the 1MB limit, so I like this approach.

fvaleri · 2023-08-31T12:52:39Z

...tor/src/main/java/io/strimzi/operator/cluster/operator/assembly/CruiseControlReconciler.java

@@ -188,6 +189,27 @@ protected Future<Void> metricsAndLoggingConfigMap() {
                    .map((Void) null);
        }
    }
+    /**
+     * Manages the Cruise Control certificates Secret.


I guess this comment should be updated.

ppatierno · 2023-09-04T07:25:06Z

Nothing to add right now. I will wait for all the other comments to be fixed before having another pass.

ShubhamRwt

@Thealisyed I think you should first get rid of all the unnecessory indentation since it makes the changes look big when they are not.

ShubhamRwt · 2023-09-07T07:10:31Z

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

+     * @param kafkaBrokerNodes          List of the broker nodes which are part of the Kafka cluster
+     * @param kafkaStorage              A map with storage configuration used by the Kafka cluster and its node pools
+     * @param kafkaBrokerResources      A map with resource configuration used by the Kafka cluster and its broker pools
+     * @param sharedEnvironmentProvider A Interface to be implemented for returning an instance of the environment variables shared by all containers.


I think the indentation part might have happened automatically by the IDE. @Thealisyed I guees you can revert the indentation and keep the things same as they were earlier. Not sure why you edited the javadoc for sharedEnvironmentProvider

kyguy · 2023-10-05T03:52:15Z

Ali is back at university so I'll take this over and finish it up over the next couple days

scholzj · 2024-01-05T19:41:25Z

@kyguy In the regression tests I started earlier, several CC related STs seemed to have failed: https://dev.azure.com/cncf/strimzi/_build/results?buildId=159143&view=results ... I restarted it, maybe it was just flaky ... but might be also some issue. That said, I realized the PR does not seem to have any logic to roll CC when the config changes. Is that missing? Or did I missed it in the PR review? Typically, you would for example hash the configs from the CM and add them as annotation to the pods to trigger the restart when the configs change (unless CC reloads them dynamically in which case we would need to fix the STs I guess).

Update: The restarted test failed as well :-/

kyguy · 2024-01-08T19:49:22Z

That said, I realized the PR does not seem to have any logic to roll CC when the config changes. Is that missing? Or did I missed it in the PR review?

You are right, this is missing! Going to fix this and re-request review once it is ready

Signed-off-by: alsyed <thealisyed@gmail.com> Signed-off-by: Kyle Liberti <kliberti@redhat.com>

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

...tor/src/main/java/io/strimzi/operator/cluster/operator/assembly/CruiseControlReconciler.java

...src/test/java/io/strimzi/operator/cluster/operator/assembly/CruiseControlReconcilerTest.java

...r/src/test/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperatorTest.java

scholzj · 2024-01-14T00:26:31Z

...src/test/java/io/strimzi/operator/cluster/operator/assembly/CruiseControlReconcilerTest.java

You should probably test the annotation here that they are set to an expected values? Might not need a new test, just enhancing the existing checks.

Done, let me know what you think!

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

scholzj · 2024-01-16T10:02:19Z

...src/test/java/io/strimzi/operator/cluster/operator/assembly/CruiseControlReconcilerTest.java

+                    // Verify annotations
+                    CruiseControl expectCruiseControl = CruiseControl.fromCrd(Reconciliation.DUMMY_RECONCILIATION, kafka, VERSIONS, NODES, Map.of("kafka", kafka.getSpec().getKafka().getStorage()), Map.of(), SHARED_ENV_PROVIDER);
+                    ConfigMap configMap = expectCruiseControl.generateConfigMap(new MetricsAndLogging(null, null));
+
+                    Map<String, String> expectedAnnotations = new HashMap<>();
+                    expectedAnnotations.put(Ca.ANNO_STRIMZI_IO_CLUSTER_CA_CERT_GENERATION, String.valueOf(clusterCa.caCertGeneration()));
+                    expectedAnnotations.put(Ca.ANNO_STRIMZI_IO_CLUSTER_CA_KEY_GENERATION, String.valueOf(clusterCa.caKeyGeneration()));
+                    expectedAnnotations.put(CruiseControl.ANNO_STRIMZI_SERVER_CONFIGURATION_HASH, Util.hashStub(configMap.getData().get(CruiseControl.SERVER_CONFIG_FILENAME)));
+                    expectedAnnotations.put(CruiseControl.ANNO_STRIMZI_CAPACITY_CONFIGURATION_HASH,  Util.hashStub(configMap.getData().get(CruiseControl.CAPACITY_CONFIG_FILENAME)));
+
+                    Map<String, String> actualAnnotations = depCaptor.getAllValues().get(0).getSpec().getTemplate().getMetadata().getAnnotations();
+                    assertThat("Annotations are not equal", actualAnnotations, is(expectedAnnotations));


This is not the right way to do it. You should use the deployment captor to check what annotations you have in the Deployment that the operator created. You already seem to have the captor below, you just need to assert it.

You should use the deployment captor to check what annotations you have in the Deployment that the operator created

I am not sure I understand, isn't that what the following line does?

Map<String, String> actualAnnotations = depCaptor.getAllValues().get(0).getSpec().getTemplate().getMetadata().getAnnotations();

uses the deployment captor, depCaptor to get a map of the annotations the operator created? Then compares against the annotation values we expect for the given configuration?

Or should I just use the deployment captor to check that required annotation keys exist in the deployment created by the operator?

I guess it does -> but all the lines above it do not seem to make any sense. You should just check the annotation value is as expected. The hash should be stable. Also, it should likely be with the other deployment captor checks?

I have refactored the code a little bit here, hopefully it is more clear. Let me know what you think!

scholzj · 2024-01-16T10:03:31Z

/azp run regression

azure-pipelines · 2024-01-16T10:03:43Z

Azure Pipelines successfully started running 1 pipeline(s).

fvaleri

Thanks @kyguy for adding the annotation part, I also missed that in my previous review. Just few nits.

Running CruiseControlConfigurationST, I have two failed tests due to NPE.

[ERROR] io.strimzi.systemtest.cruisecontrol.CruiseControlConfigurationST.testConfigurationPerformanceOptions(ExtensionContext) -- Time elapsed: 230.3 s <<< ERROR!
java.lang.NullPointerException
	at java.base/java.util.Objects.requireNonNull(Objects.java:209)
	at io.strimzi.systemtest.cruisecontrol.CruiseControlConfigurationST.testConfigurationPerformanceOptions(CruiseControlConfigurationST.java:277)


[ERROR] io.strimzi.systemtest.cruisecontrol.CruiseControlConfigurationST.testConfigurationReflection(ExtensionContext) -- Time elapsed: 144.4 s <<< ERROR!
java.lang.NullPointerException
	at java.base/java.util.Objects.requireNonNull(Objects.java:209)
	at io.strimzi.systemtest.cruisecontrol.CruiseControlConfigurationST.testConfigurationReflection(CruiseControlConfigurationST.java:194)

fvaleri · 2024-01-16T09:33:53Z

api/src/main/java/io/strimzi/api/kafka/model/kafka/cruisecontrol/CruiseControlResources.java

@@ -77,7 +77,7 @@ public static String secretName(String clusterName) {
     * @param clusterName  The {@code metadata.name} of the {@code Kafka} resource.
     * @return The name of the corresponding Cruise Control log {@code ConfigMap}.
     */
-    public static String logAndMetricsConfigMapName(String clusterName) {
+    public static String configMapName(String clusterName) {


We should also update the javadoc.

/** * Returns the name of the Cruise Control {@code ConfigMap} for a {@code Kafka} cluster of the given name. * @param clusterName The {@code metadata.name} of the {@code Kafka} resource. * @return The name of the corresponding Cruise Control {@code ConfigMap}. */

fvaleri · 2024-01-16T09:44:26Z

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

    protected static final String API_AUTH_CREDENTIALS_FILE = API_AUTH_CONFIG_VOLUME_MOUNT + API_AUTH_FILE_KEY;

    protected static final String ENV_VAR_CRUISE_CONTROL_METRICS_ENABLED = "CRUISE_CONTROL_METRICS_ENABLED";

+    /**
+     * Annotation for rolling a cluster whenever the server configuration has changed.
+     * By changing the annotation we force a restart since the pod will be out of date compared to the Pod.


Suggested change

* By changing the annotation we force a restart since the pod will be out of date compared to the Pod.

* When the configuration hash annotation change is detected, we force a pod restart.

fvaleri · 2024-01-16T09:44:40Z

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

+
+    /**
+     * Annotation for rolling a cluster whenever the capacity configuration has changed.
+     * By changing the annotation we force a restart since the pod will be out of date compared to the Pod.


Suggested change

* By changing the annotation we force a restart since the pod will be out of date compared to the Pod.

* When the configuration hash annotation change is detected, we force a pod restart.

fvaleri · 2024-01-16T09:46:40Z

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

@@ -380,7 +398,6 @@ protected List<EnvVar> getEnvVars() {
        varList.add(ContainerUtils.createEnvVar(ENV_VAR_STRIMZI_KAFKA_BOOTSTRAP_SERVERS, KafkaResources.bootstrapServiceName(cluster) + ":" + KafkaCluster.REPLICATION_PORT));
        varList.add(ContainerUtils.createEnvVar(ENV_VAR_STRIMZI_KAFKA_GC_LOG_ENABLED, String.valueOf(gcLoggingEnabled)));



Unnecessary empty line.

fvaleri · 2024-01-16T09:52:34Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/cruisecontrol/CpuCapacity.java

+     *
+     * @return The CpuCapacity object as a JsonObject
+     */
+    public JsonObject getJson() {


With this, it looks like getCores() is not used anymore, so we can get rid of it.

fvaleri · 2024-01-16T10:05:47Z

...src/test/java/io/strimzi/operator/cluster/operator/assembly/CruiseControlReconcilerTest.java

@@ -93,7 +100,7 @@ public void reconcileEnabledCruiseControl(VertxTestContext context) {
        when(mockNetPolicyOps.reconcile(any(), eq(NAMESPACE), eq(CruiseControlResources.networkPolicyName(NAME)), netPolicyCaptor.capture())).thenReturn(Future.succeededFuture());

        ArgumentCaptor<ConfigMap> cmCaptor = ArgumentCaptor.forClass(ConfigMap.class);
-        when(mockCmOps.reconcile(any(), eq(NAMESPACE), eq(CruiseControlResources.logAndMetricsConfigMapName(NAME)), cmCaptor.capture())).thenReturn(Future.succeededFuture());
+        when(mockCmOps.reconcile(any(), eq(NAMESPACE), eq(CruiseControlResources.configMapName(NAME)), cmCaptor.capture())).thenReturn(Future.succeededFuture());

        ArgumentCaptor<Deployment> depCaptor = ArgumentCaptor.forClass(Deployment.class);


Maybe deployCaptor?

If it's alright with you, I would like to take care of this in a different PR where will refactor the CC package structure and tests

I think depCaptor is used in many places. deployCaptor is not used anywhere. So I think the name is fine.

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

scholzj

LGTM assuming the tests pass.

scholzj · 2024-01-17T00:38:11Z

/azp run regression

azure-pipelines · 2024-01-17T00:38:23Z

Azure Pipelines successfully started running 1 pipeline(s).

ppatierno

LGTM. Just a couple of nits.

ppatierno · 2024-01-17T13:21:47Z

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

+    /**
+     * Server config file name
+     */
+    public static final String SERVER_CONFIG_FILENAME = "cruisecontrol.properties";


why are we using the SERVER_ naming here? The same applies to the annotation below. Isn't this constant and the annotation related to Cruise Control only, so that something like CRUISE_CONTROL_ is clearer?

I wanted to draw the distinction between the configuration of the Cruise Control server application and the Cruise Control broker capacity, since both are Cruise Control configurations. I figured since this fields and annotations are in the Cruise Control class and Cruise Control pod respectively, it would be clear they were apart of Cruise Control!

Does that explanation make sense, does it sound reasonable to you?

Ok, makes sense. Thanks!

ppatierno · 2024-01-17T13:23:26Z

...ter-operator/src/main/java/io/strimzi/operator/cluster/model/CruiseControlConfiguration.java

@@ -16,6 +16,7 @@
 import java.util.TreeMap;
 import java.util.concurrent.TimeUnit;

+


non necessary blank line.

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

kyguy · 2024-01-18T13:57:08Z

All outstanding feedback has been addressed, this PR is now ready for merge!

kyguy added this to the 0.37.0 milestone Aug 11, 2023

Thealisyed force-pushed the cc-capacity-cm branch 3 times, most recently from 0dfdd11 to d417308 Compare August 16, 2023 15:45

kyguy reviewed Aug 24, 2023

View reviewed changes

kyguy reviewed Aug 28, 2023

View reviewed changes

Thealisyed force-pushed the cc-capacity-cm branch 2 times, most recently from 557ed6e to 6b25178 Compare August 29, 2023 15:05

kyguy reviewed Aug 29, 2023

View reviewed changes

Thealisyed force-pushed the cc-capacity-cm branch from 6b25178 to d628d8f Compare August 29, 2023 15:32

kyguy reviewed Aug 29, 2023

View reviewed changes

kyguy reviewed Aug 30, 2023

View reviewed changes

Thealisyed force-pushed the cc-capacity-cm branch from 5d96fb1 to 2322b7b Compare August 30, 2023 14:53

kyguy requested review from scholzj, ShubhamRwt and ppatierno August 30, 2023 14:55

scholzj reviewed Aug 30, 2023

View reviewed changes

kyguy requested a review from fvaleri August 30, 2023 16:08

scholzj modified the milestones: 0.37.0, 0.38.0 Aug 31, 2023

fvaleri reviewed Aug 31, 2023

View reviewed changes

ShubhamRwt suggested changes Sep 7, 2023

View reviewed changes

kyguy self-assigned this Oct 5, 2023

scholzj modified the milestones: 0.38.0, 0.39.0 Oct 11, 2023

kyguy force-pushed the cc-capacity-cm branch 3 times, most recently from f818fd6 to 095de6f Compare November 8, 2023 00:58

ShubhamRwt approved these changes Jan 5, 2024

View reviewed changes

kyguy added 6 commits January 12, 2024 16:00

Fixed CC system test for capacity config map

98a2e20

Signed-off-by: alsyed <thealisyed@gmail.com> Signed-off-by: Kyle Liberti <kliberti@redhat.com>

Addressing comments, adding CC server config to ConfigMap

6f20368

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

Addressing feedback - js

7b52835

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

Addressing feedback - JS, SR, FV

faa0a53

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

Addressing more feedback - FV, JS

8d7f676

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

Adding config hash annotations to ensure pod rolls

e154298

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

kyguy force-pushed the cc-capacity-cm branch from 1ac3121 to e154298 Compare January 12, 2024 23:26

scholzj reviewed Jan 14, 2024

View reviewed changes

Addressing more feedback - js

5991a20

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

kyguy force-pushed the cc-capacity-cm branch from 4f37e3d to 5991a20 Compare January 16, 2024 06:27

scholzj reviewed Jan 16, 2024

View reviewed changes

fvaleri reviewed Jan 16, 2024

View reviewed changes

kyguy force-pushed the cc-capacity-cm branch 2 times, most recently from d569835 to 89d79e8 Compare January 16, 2024 23:26

Addressing more feedback - js, fv

360d93c

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

kyguy force-pushed the cc-capacity-cm branch from 89d79e8 to 360d93c Compare January 16, 2024 23:27

scholzj approved these changes Jan 17, 2024

View reviewed changes

ppatierno approved these changes Jan 17, 2024

View reviewed changes

Addressing feedback - pp

4df82a7

Signed-off-by: Kyle Liberti <kliberti@redhat.com>

scholzj merged commit 3a8de17 into strimzi:main Jan 18, 2024
13 checks passed

scholzj mentioned this pull request Feb 11, 2024

Cruise Control configuration should be passed in a ConfigMap instead of environment variable #8337

Closed

	* Returns the name of the Cruise Control Broker Capacity ConfigMap Name {@code ConfigMap} for a {@code Kafka} cluster of the given name.
	* Returns the name of the Cruise Control Broker capacity {@code ConfigMap} for a {@code Kafka} cluster of the given name.

	* @return The name of the corresponding Cruise Control log {@code ConfigMap}.
	* @return The name of the corresponding Cruise Control capacity {@code ConfigMap}.

		@@ -145,6 +149,7 @@ public class CruiseControl extends AbstractModel implements SupportsMetrics, Sup
		private InternalServiceTemplate templateService;

		private static final Map<String, String> DEFAULT_POD_LABELS = new HashMap<>();

	* @param capacity assigned capacity parameter for broker capacity configmap data
	* @param capacity data used for broker capacity configuration

	* Name of the Cruise Control metrics topic
	* Name of the Cruise Control capacity config file path

	* By changing the annotation we force a restart since the pod will be out of date compared to the Pod.
	* When the configuration hash annotation change is detected, we force a pod restart.

		@@ -380,7 +398,6 @@ protected List<EnvVar> getEnvVars() {
		varList.add(ContainerUtils.createEnvVar(ENV_VAR_STRIMZI_KAFKA_BOOTSTRAP_SERVERS, KafkaResources.bootstrapServiceName(cluster) + ":" + KafkaCluster.REPLICATION_PORT));
		varList.add(ContainerUtils.createEnvVar(ENV_VAR_STRIMZI_KAFKA_GC_LOG_ENABLED, String.valueOf(gcLoggingEnabled)));

		@@ -16,6 +16,7 @@
		import java.util.TreeMap;
		import java.util.concurrent.TimeUnit;

Move cruise control server, capacity and server logging metrics config to ConfigMap #8977

Move cruise control server, capacity and server logging metrics config to ConfigMap #8977

Conversation

Thealisyed commented Aug 11, 2023 • edited

Type of change

Description

Checklist

kyguy 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

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

fvaleri Aug 31, 2023 • edited

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

fvaleri left a comment

Choose a reason for hiding this comment

fvaleri Aug 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppatierno commented Sep 4, 2023

ShubhamRwt left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyguy commented Oct 5, 2023

scholzj commented Jan 5, 2024 • edited

kyguy commented Jan 8, 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 Jan 16, 2024

azure-pipelines bot commented Jan 16, 2024

fvaleri 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

Thealisyed commented Aug 11, 2023 •

edited

fvaleri Aug 31, 2023 •

edited

fvaleri Aug 31, 2023 •

edited

ShubhamRwt left a comment •

edited

scholzj commented Jan 5, 2024 •

edited