Cruise Control deployment #2773

kyguy · 2020-04-01T16:44:39Z

Type of change

Enhancement / new feature

Description

Gives the ability to bring up a Cruise Control instance to monitor a Strimzi Kafka cluster. Cruise Control and its configurations can be declared in a Kafka resource like this:

apiVersion: kafka.strimzi.io/v1beta1
kind: Kafka
metadata:
...
spec:
  kafka:
    ...
  cruiseControl:
    image: <image>
    jvmOptions: {}
    logging: {}
    template: {}
    resources: {}
    tlsSideCar: {}
    readinessProbe: {}
    livenessProbe: {}
    brokerCapacity: [1]
      disk: 100G
      cpu: 100     # as a percentage (0-100)
      inboundNetwork: 10000KB/s
      outboundNetwork: 10000KB/s
    config: [2]
      default.goals:<list_of_goals>
      goals: <list_of_goals>
      intra.broker.goals: <list_of_goals>
      hard.goals: <list_of_goals>
      cpu.balance.threshold: 1.1
      metadata.max.age.ms: 300000
      send.buffer.bytes: 131072
      ...

After declaring a cruiseControl in the resource's spec, all Kafka brokers will be rerolled with a Cruise Control metric reporter which will gather metrics about the cluster and store them in a Kafka topic. The Cruise Control application will then process these metrics and use them to learn about the cluster.

Note: This is PR exposes the full Cruise Control API, but later PRs will lock this API down, making only a subset of the operations available through another resource.

[1] Cruise Control uses these capacity limits to determine if goals related to broker resources are being violated. (e.g. DiskCapacityGoal: violated when the disk capacity set for a broker is exceeded by the disk being used by that broker). The settings specified here will populate the capacity configuration file fed to Cruise Control (https://github.com/linkedin/cruise-control/blob/2.0.97/config/capacity.json)

[2] Represents the Cruise Control properties file used for configuring the Cruise Control application (https://github.com/linkedin/cruise-control/wiki/Configurations) All configurations are allowed except for the FORBIDDEN CONFIG OPTIONS = bootstrap.servers, client.id, zookeeper., network., security., failed.brokers.zk.path, webserver.http., webserver.api.urlprefix, webserver.session.path, webserver.accesslog., two.step., request.reason.required, metric.reporter.topic, partition.metric.sample.store.topic, broker.metric.sample.store.topic, capacity.config.file, self.healing.

Checklist

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

Update/write design documentation in ./design
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

samuel-hawker · 2020-04-01T17:55:28Z

api/src/main/java/io/strimzi/api/kafka/model/template/CruiseControlTemplate.java

+)
+@JsonInclude(JsonInclude.Include.NON_NULL)
+@JsonPropertyOrder({
+        "deployment", "pod", "apiService", "podDisruptionBudget", "cruiseControlcontainer", "tlsSidecarContainer"})


*Container needs capitalization in cruiseControlContainer

I've opened #2779 for resolving this sort of thing once and for all.

samuel-hawker · 2020-04-01T18:18:11Z

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

+
+        // Check Service
+        Service svc = cc.generateService();
+        assertThat(svc.getMetadata().getLabels().entrySet().containsAll(svcLabels.entrySet()), is(true));


Minor niggle, but from a failing test point of view might the check

assertThat(svc.getMetadata().getLabels(), is(svcLabels));

give a more informative error message on failure? This applys to all of the assertions in this file with containsAll calls

samuel-hawker · 2020-04-01T18:19:58Z

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

+        try {
+            assertThat(cc.generateService(), is(nullValue()));
+        } catch (Throwable expected) {
+            assertEquals(NullPointerException.class, expected.getClass());


Just to be a little more consistent with the rest of the tests in Strimzi would you opposed to using

assertThrows(NullPointerException.class, () -> cc.generateService());

And let's be clear that the code above it wrong anyway. If you're expecting cc.generateService() to throw there's no point in having the assertThat and the cc.generateService() statement should be followed by a fail(), otherwise in the case of no NPE the test will erroneously pass.

samuel-hawker · 2020-04-01T18:22:40Z

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

+        CruiseControl cc = CruiseControl.fromCrd(resource, VERSIONS);
+
+        List<EnvVar> kafkaEnvVars = cc.getEnvVars();
+        assertThat(kafkaEnvVars.stream().filter(var -> testEnvOneKey.equals(var.getName())).map(EnvVar::getValue).findFirst().get(), is(testEnvOneValue));


assertThat(kafkaEnvVars, hasEntry(testEnvOneKey, testEnvOneValue) suggested improvement
And maybe have same for envar2?

Since kafkaEnvVars is a list instead of a map I used hasItem() instead. How does this look:

List<EnvVar> envVarList = cc.getEnvVars(); assertThat(envVarList, hasItems(new EnvVar(testEnvOneKey, testEnvOneValue, null))); assertThat(envVarList, hasItems(new EnvVar(testEnvTwoKey, testEnvTwoValue, null)));

Let me know what you think!

Looks good, though since hasItems takes ...items you could achieve the same with

assertThat(envVarList, hasItems(new EnvVar(testEnvOneKey, testEnvOneValue, null), new EnvVar(testEnvTwoKey, testEnvTwoValue, null)));

But this is only a suggestion , not a required change at all! :P

samuel-hawker · 2020-04-01T18:22:54Z

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

+        CruiseControl cc = CruiseControl.fromCrd(resource, VERSIONS);
+
+        List<EnvVar> kafkaEnvVars = cc.getEnvVars();
+        assertThat(kafkaEnvVars.stream().filter(var -> testEnvOneKey.equals(var.getName())).map(EnvVar::getValue).findFirst().get(), is(testEnvOneValue));


assertThat(kafkaEnvVars, hasEntry(testEnvOneKey, testEnvOneValue) suggested improvement
And maybe have same for envar2?

samuel-hawker · 2020-04-01T18:24:44Z

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

+
+    private final CruiseControl cc = CruiseControl.fromCrd(resource, VERSIONS);
+
+    public void checkOwnerReference(OwnerReference ownerRef, HasMetadata resource)  {


Just as an aside, this might be a good function to contribute to test utils as I can see all of the tests needing to call this method!

Yes please! Good spot @samuel-hawker!

I'll save this for another PR if that is alright with you two, the PR already changes a lot of files!

@kyguy for sure updating ALL the other files should be done in a different PR but here you could start to move the method in the test utils and using it. Then, again, reusing the method for ALL the other tests has to be done in a different PR. Wdyt?

tombentley · 2020-04-02T08:08:21Z

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

+public class CruiseControlSpec implements UnknownPropertyPreserving, Serializable {
+    private static final long serialVersionUID = 1L;
+
+    public static final String FORBIDDEN_PREFIXES = "bootstrap.servers, zookeeper., ssl., security., failed.brokers.zk.path,"


@kyguy is there a link to the documentation about all these configs?

Just added this link[1] above the field
[1] https://github.com/linkedin/cruise-control/wiki/Configurations

tombentley · 2020-04-02T08:09:02Z

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

+        this.tlsSidecar = tlsSidecar;
+    }
+
+    @Description("The Cruise Control broker capacity config.")


Suggested change

@Description("The Cruise Control broker capacity config.")

@Description("The Cruise Control broker capacity configuration.")

tombentley · 2020-04-02T08:09:39Z

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

+
+    @Description("The Cruise Control broker capacity config.")
+    @JsonInclude(JsonInclude.Include.NON_NULL)
+    public CruiseControlBrokerCapacity getCapacity() {


The class is called ...BokerCapacity, so should the property be called that too?

tombentley · 2020-04-02T08:10:14Z

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

+        this.capacity = capacity;
+    }
+
+    @Description("The Cruise Control config. Properties with the following prefixes cannot be set: " + FORBIDDEN_PREFIXES)


We should include a link here to the place where the user can learn about what the valid configs are.

Added this link [1] to the description

[1] https://github.com/linkedin/cruise-control/wiki/Configurations

tombentley · 2020-04-02T08:11:30Z

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

+    }
+
+    @JsonInclude(JsonInclude.Include.NON_EMPTY)
+    @Description("JVM Options for pods")


For the cruise control pod (singular)? If so, please say so in the Description to the reader of the docs is left in no doubt about what they're configuring.

documentation/modules/cruise-control/con-cruise-control-overview.adoc

tombentley · 2020-04-02T09:04:54Z

documentation/modules/cruise-control/proc-deploying-cruise-control.adoc

+<1> Number of Cruise Control application instances; set to `1` to run or `0` to pause the application.
+<2> Specifies the Cruise Control capacity configuration. For more information, see xref:capacity_configuration[capacity configuration].
+<3> Specifies the Cruise Control configuration. You can provide any xref:ref-cruise-control-configuration-{context}[standard configuration option] apart from those managed directly by {ProductName}.
+<4> CPU and memory resources reserved for Cruise Control. For more information, see xref:assembly-resource-limits-and-requests-deployment-configuration-kafka[].
+<5> Logging configuration, to log messages with a given severity (debug, info, warn, error, fatal) or above.
+<6> xref:assembly-customizing-deployments-str[Customization of deployment templates and pods].
+<7> xref:assembly-healthchecks-deployment-configuration-kafka[Healthcheck readiness probes].
+<8> xref:assembly-healthchecks-deployment-configuration-kafka[Healthcheck liveness probes].


Does this belong in a procedure, rather than a concept?

I think it could be separated into a reference but have been shuffling these around. Let me get the final word from @laidan6000

On balance, I think it should stay here because the example configuration is helpful within the context of the task. In other words, it doesn't seem to stand alone as a concept or reference module.

tombentley · 2020-04-02T09:06:46Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+The `config` property in `Kafka.spec.cruiseControl` contains configuration options as keys with values as one of the following JSON types:
+
+* String
+* Number
+* Boolean


This applies more generally to the rest of the docs too, but here would be a good place for a NOTE to say that if your String happens to look like YAML or JSON it will need to be explicitly quoted.

tombentley · 2020-04-02T09:07:45Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+----
+== Capacity configuration
+
+Cruise Control uses capacity limits to determine if goals related to broker resources are being violated.


Which begs the question: What happens if the limits are violated?

Added some text:

A violation of a goal can cause a optimization to fail, preventing that optimization from being used to generate a partition rebalance proposal.
For example, an optimization that would cause a broker to exceed its CPU capacity would not be used for when the CpuCapacityGoal is set as a hard goal

tombentley · 2020-04-02T09:08:15Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+* `networkIn`  - Network inbound throughput in KB/s
+* `networkOut` - Newtork outbound throughput in KB/s
+
+Since {ProductName} brokers are homogenous, Cruise Control will apply the same capacity limits to every broker it is monitoring.


Suggested change

Since {ProductName} brokers are homogenous, Cruise Control will apply the same capacity limits to every broker it is monitoring.

Since {ProductName} brokers are homogeneous, Cruise Control will apply the same capacity limits to every broker it is monitoring.

samuel-hawker · 2020-04-02T12:19:45Z

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

+        this.capacity = capacity;
+    }
+
+    @Description("The Cruise Control config. Properties with the following prefixes cannot be set: " + FORBIDDEN_PREFIXES)


as Tom suggested above, config should also be configuration here

tombentley · 2020-04-07T07:31:45Z

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

+    public static final String FORBIDDEN_PREFIXES = "bootstrap.servers, zookeeper., ssl., security., failed.brokers.zk.path,"
+        + "webserver.http.port, webserver.http.address, webserver.api.urlprefix, metric.reporter.sampler.bootstrap.servers, metric.reporter.topic, metric.reporter.topic.pattern,"
+        + "partition.metric.sample.store.topic, broker.metric.sample.store.topic, brokerCapacity.config.file, skip.sample.store.topic.rack.awareness.check, cruise.control.metrics.topic"
+        + "cruise.control.metrics.reporter.bootstrap.servers, sasl. interceptor.classes";


Suggested change

+ "cruise.control.metrics.reporter.bootstrap.servers, sasl. interceptor.classes";

+ "cruise.control.metrics.reporter.bootstrap.servers, sasl.interceptor.classes";

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

tombentley · 2020-04-07T07:32:19Z

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

+    // For the full configuration list refer to https://github.com/linkedin/cruise-control/wiki/Configurations
+    public static final String FORBIDDEN_PREFIXES = "bootstrap.servers, zookeeper., ssl., security., failed.brokers.zk.path,"
+        + "webserver.http.port, webserver.http.address, webserver.api.urlprefix, metric.reporter.sampler.bootstrap.servers, metric.reporter.topic, metric.reporter.topic.pattern,"
+        + "partition.metric.sample.store.topic, broker.metric.sample.store.topic, brokerCapacity.config.file, skip.sample.store.topic.rack.awareness.check, cruise.control.metrics.topic"


Suggested change

+ "partition.metric.sample.store.topic, broker.metric.sample.store.topic, brokerCapacity.config.file, skip.sample.store.topic.rack.awareness.check, cruise.control.metrics.topic"

+ "partition.metric.sample.store.topic, broker.metric.sample.store.topic, brokerCapacity.config.file, skip.sample.store.topic.rack.awareness.check, cruise.control.metrics.topic,"

tombentley · 2020-04-07T07:33:14Z

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

+        this.tlsSidecar = tlsSidecar;
+    }
+
+    @Description("The Cruise Control brokerCapacity configuration.")


Suggested change

@Description("The Cruise Control brokerCapacity configuration.")

@Description("The Cruise Control `brokerCapacity` configuration.")

tombentley · 2020-04-07T07:33:41Z

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

+    }
+
+    @Description("The Cruise Control configuration. For a full list of configuration options refer to" +
+            " https://github.com/linkedin/cruise-control/wiki/Configurations Note that properties " +


Suggested change

" https://github.com/linkedin/cruise-control/wiki/Configurations Note that properties " +

" https://github.com/linkedin/cruise-control/wiki/Configurations. Note that properties " +

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

tombentley · 2020-04-07T07:55:36Z

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

+        if (cruiseControlSpec != null) {
+            metricReporterList.add(CRUISE_CONTROL_METRIC_REPORTER);
+        } else {
+            metricReporterList.remove(CRUISE_CONTROL_METRIC_REPORTER);


So it they added CRUISE_CONTROL_METRIC_REPORTER explicitly themselves we remove it unless they also specified a CruiseControlSpec? I think this is more or less a moot point at the moment, since there's no mechanism for people to add plugin classes to our Kafka images (apart from them building their own images, which perhaps some people do). But I would none the less argue that this is not the right thing to do.

So it they added CRUISE_CONTROL_METRIC_REPORTER explicitly themselves we remove it unless they also specified a CruiseControlSpec?

Yes, but the main intention of this code is to stop the Cruise Control metric reporters after the Cruise Control application has been deleted. Otherwise, the Cruise Control metric reporters will continue to run and cost resources even if they are not being used.

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

tombentley · 2020-04-07T07:58:55Z

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

+            List<SingleVolumeStorage> volumeList = ((JbodStorage) storage).getVolumes();
+            long size = 0;
+            for (SingleVolumeStorage volume : volumeList) {
+                size += generateDiskCapacity(volume);
+            }
+            return size;


So does this mean the CC doesn't really distinguish the disks as having individual sizes?

Cruise Control does distinguish the disks as having individual sizes using the adminClient.

However, for balancing partitions between disks of the same broker, Cruise Control needs us to explicitly state the capacity of each disk here, which I have not done yet. This will require some extra logic.

So will that extra logic be in this PR, or a future one?

A future one

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

d-laing

Peer reviewed. This is looking great!

d-laing · 2020-04-07T13:17:06Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+----
+
+.Additional resources
+For information, refer to the xref:type-CruiseControlBrokerCapacity-reference[Capacity schema reference].


FYI - this cross-reference is currently broken.

d-laing · 2020-04-07T13:47:01Z

documentation/modules/cruise-control/con-cruise-control-overview.adoc

+[id='con-cruise-control-overview-{context}']
+= Cruise Control overview
+
+Brokers in a Kafka cluster can become unevenly loaded due to things like an uneven distribution of busy partitions.


Suggested change

Brokers in a Kafka cluster can become unevenly loaded due to things like an uneven distribution of busy partitions.

Brokers in a Kafka cluster can become unevenly loaded for many reasons. For example, partitions that handle large amounts of message traffic can be unevenly distributed across the available brokers.

Next, I would explain that partition reassignment is usually a manual task. Perhaps you could add a sentence like the following:

"Monitoring broker load and reassigning busy partitions to brokers with spare capacity is time consuming and adds to the overhead of managing a Kafka cluster"

d-laing · 2020-04-07T14:16:28Z

documentation/modules/cruise-control/con-cruise-control-overview.adoc

+= Cruise Control overview
+
+Brokers in a Kafka cluster can become unevenly loaded due to things like an uneven distribution of busy partitions.
+Cruise Control is a tool for simplifying the monitoring and rebalancing of partitions across a Kafka cluster.


Suggested change

Cruise Control is a tool for simplifying the monitoring and rebalancing of partitions across a Kafka cluster.

Cruise Control is a tool for automating the monitoring and rebalancing of partitions across a Kafka cluster.

d-laing · 2020-04-07T14:33:12Z

documentation/modules/cruise-control/con-cruise-control-overview.adoc

+
+Brokers in a Kafka cluster can become unevenly loaded due to things like an uneven distribution of busy partitions.
+Cruise Control is a tool for simplifying the monitoring and rebalancing of partitions across a Kafka cluster.
+You deploy Cruise Control alongside a Kafka cluster to:


I think we should make CC the subject of this sentence and use different terminology. Please rephrase to something like the following:

When deployed alongside a Kafka cluster, Cruise Control can perform the following cluster balancing tasks:

Monitoring...

Making optimization proposals: balanced partition assignments based on configurable optimization goals.

Executing partition reassignments based on optimization proposals.

d-laing · 2020-04-07T14:50:15Z

documentation/modules/cruise-control/proc-deploying-cruise-control.adoc

+[id='proc-deploying-cruise-control-{context}']
+= Deploying Cruise Control
+
+Deploy Cruise Control to your {ProductName} cluster through the `cruiseControl` property in the `Kafka` resource.


Suggested change

Deploy Cruise Control to your {ProductName} cluster through the `cruiseControl` property in the `Kafka` resource.

Cruise Control is configured using the `cruiseControl` property in the `Kafka` resource. Once configured, you can deploy a Cruise Control instance to your {ProductName} cluster by creating or updating the `Kafka` resource.

d-laing · 2020-04-08T15:41:02Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+
+Cruise Control uses capacity limits to determine if goals related to broker resources are being violated.
+A violation of a goal can cause a optimization to fail, preventing that optimization from being used to generate a partition rebalance proposal.
+For example, an optimization that would cause a broker to exceed its CPU capacity would not be used for when the CpuCapacityGoal is set as a hard goal.


Suggested change

For example, an optimization that would cause a broker to exceed its CPU capacity would not be used for when the CpuCapacityGoal is set as a hard goal.

For example, an optimization that would cause a broker to exceed its CPU capacity would not be used if the `CpuCapacityGoal` is set as a hard goal.

d-laing · 2020-04-08T15:42:37Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+A violation of a goal can cause a optimization to fail, preventing that optimization from being used to generate a partition rebalance proposal.
+For example, an optimization that would cause a broker to exceed its CPU capacity would not be used for when the CpuCapacityGoal is set as a hard goal.
+
+Capacity limits for broker resources can be specified in the `brokerCapacity` property in `Kafka.spec.cruiseControl`.


Suggested change

Capacity limits for broker resources can be specified in the `brokerCapacity` property in `Kafka.spec.cruiseControl`.

You specify capacity limits for broker resources in the `brokerCapacity` property in `Kafka.spec.cruiseControl` .

d-laing · 2020-04-08T15:45:04Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+* `diskMiB`  - Disk storage in MiB
+* `cpuUtilization`   - CPU utilization as a percent (0-100)
+* `inboundNetworkKiBPerSecond`  - Network inbound throughput in KiB/s
+* `outboundNetworkKiBPerSecond` - Newtork outbound throughput in KiB/s


Suggested change

* `outboundNetworkKiBPerSecond` - Newtork outbound throughput in KiB/s

* `outboundNetworkKiBPerSecond` - Network outbound throughput in KiB/s

d-laing · 2020-04-08T15:46:08Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+* `inboundNetworkKiBPerSecond`  - Network inbound throughput in KiB/s
+* `outboundNetworkKiBPerSecond` - Newtork outbound throughput in KiB/s
+
+Since {ProductName} brokers are homogeneous, Cruise Control will apply the same capacity limits to every broker it is monitoring.


Suggested change

Since {ProductName} brokers are homogeneous, Cruise Control will apply the same capacity limits to every broker it is monitoring.

Because Kafka brokers are homogeneous, Cruise Control applies the same capacity limits to every broker it is monitoring.

Updated to {ProductName} Kafka brokers since Kafka brokers are not necessarily homogeneous but Kafka brokers managed by Strimzi are homogeneous

d-laing · 2020-04-08T15:46:31Z

documentation/modules/cruise-control/ref-cruise-control-configuration.adoc

+----
+
+.Additional resources
+For information, refer to the xref:type-CruiseControlBrokerCapacity-reference[Capacity schema reference].


Suggested change

For information, refer to the xref:type-CruiseControlBrokerCapacity-reference[Capacity schema reference].

For more information, refer to the xref:type-CruiseControlBrokerCapacity-reference[].

tombentley

A few nits, but I'm happy for this to be merged.

tombentley · 2020-04-20T09:13:12Z

api/src/main/java/io/strimzi/api/kafka/model/balancing/BrokerCapacity.java

+    private Map<String, Object> additionalProperties = new HashMap<>(0);
+
+    @JsonInclude(JsonInclude.Include.NON_DEFAULT)
+    @Pattern("^[0-9]+([KMGTPE]i?)?$")


I think we should allow stuff like 1.5T and exponential notation like 2e6 (which Kube does for memory, for example).

Suggested change

@Pattern("^[0-9]+([KMGTPE]i?)?$")

@Pattern("^[0-9]+([.][0-9]*)?([KMGTPE]i?|e[0-9]+)?$")

And I guess to be consistent with the network settings we should allow the B suffix in there too. It's difficult because we're trying to be consistent with Kube, but what Kube does (omitting the actual unit) doesn't really work for the network rates.

Updated to include . and exponential notation.

I think we should leave the B suffix out of the disk capacity values so that it is consistent with the storage values we use for Strimzi (which follow the K8s memory format). I do still think the B suffix is very important for the network rates though! What do you think?

tombentley · 2020-04-20T09:18:17Z

api/src/main/java/io/strimzi/api/kafka/model/balancing/BrokerCapacity.java

+
+    @JsonInclude(JsonInclude.Include.NON_NULL)
+    @Pattern("[0-9]+([KMG]i?)?B/s")
+    @Description("Broker capacity for network inbound throughput, for example, 10000KB/s")


Can we explicitly say that this is in bytes per second, just to avoid people mistakenly thinking it might be in bits per second? Same for the outbound.

tombentley · 2020-04-20T09:19:25Z

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

+    private Double brokerInboundNetworkKiBPerSecondCapacity;
+    private Double brokerOuboundNetworkKiBPerSecondCapacity;
+
+


I don't think we need these double empty lines (here and the following two).

tombentley · 2020-04-20T09:19:53Z

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

+        List<ServicePort> ports = new ArrayList<>(1);
+        ports.add(createServicePort(REST_API_PORT_NAME, REST_API_PORT, REST_API_PORT, "TCP"));


Collections.singletonList()

tombentley · 2020-04-20T09:20:21Z

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

+    }
+
+    protected List<Volume> getVolumes(boolean isOpenShift) {
+        List<Volume> volumeList = new ArrayList<>(1);


If you insist on doing it like this then get the right number of items:

Suggested change

List<Volume> volumeList = new ArrayList<>(1);

List<Volume> volumeList = new ArrayList<>(3);

But even better would be to use

return Arrays.asList(createSecretVolume(...), createSecretVolume(...), createConfigMapVolume(...));

tombentley · 2020-04-20T09:20:31Z

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

+    }
+
+    protected List<VolumeMount> getVolumeMounts() {
+        List<VolumeMount> volumeMountList = new ArrayList<>(1);


Same comment.

tombentley · 2020-04-20T09:24:51Z

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

+        CC_DEFAULT_PROPERTIES_MAP.put("default.goals", DEFAULT_GOALS);
+        CC_DEFAULT_PROPERTIES_MAP.put("goals", DEFAULT_GOALS);
+
+        FORBIDDEN_OPTIONS = asList(CruiseControlSpec.FORBIDDEN_PREFIXES.split(", "));


Using ", *" (split takes a regex) would protect you a little from people being careless with whitespace when making changes in the future.

tombentley · 2020-04-20T09:27:36Z

...rator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperator.java

+                Map<String, String> annotations = new HashMap<>();
+                annotations.put(CruiseControl.ANNO_STRIMZI_IO_LOGGING, logAndMetricsConfigMap.getData().get(ANCILLARY_CM_KEY_LOG_CONFIG));


Collections.singletonMap()

d-laing

Doc looks good so far.

ppatierno · 2020-04-21T06:15:13Z

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

@@ -172,7 +174,7 @@ public void setImage(String image) {
    }

    @JsonInclude(JsonInclude.Include.NON_NULL)
-    @Description("CPU and memory resources to reserve.")
+    @Description("CPU and memory resources to reserve for the Cruise Control container.")


we are in the KafkaClusterSpec, why this is about Cruise Control container resources?

ppatierno · 2020-04-21T06:19:02Z

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

+    protected static final String LOG_AND_METRICS_CONFIG_VOLUME_NAME = "cruise-control-logging";
+    protected static final String LOG_AND_METRICS_CONFIG_VOLUME_MOUNT = "/opt/cruise-control/custom-config/";
+
+    public static final String ANNO_STRIMZI_IO_LOGGING = Annotations.STRIMZI_DOMAIN + "/logging";


Have you rebased against the latest master? Just asking because lately the Annotations.STRIMZI_DOMAIN was changed from strimzi.io to strimzi.io/. In this case, we are going to have strimzi.io//logging so you should remove the / from logging. I had the same problem with one of my latest PR :-)

d-laing · 2020-04-21T09:38:13Z

@kyguy - I think you should comment out the two modules that are "TBC".

kyguy · 2020-04-23T00:18:28Z

Can someone kick the Azure Pipelines again? The build failure appears to be related to the pipeline

ppatierno · 2020-04-23T06:11:14Z

@strimzi-ci run tests

ppatierno · 2020-04-23T06:12:27Z

@kyguy I think there is a problem with Java 8 on Azure. @Frawless can you confirm this?

Frawless · 2020-04-23T06:17:19Z

Yes. Opened #2876 to fix it

strimzi-ci · 2020-04-23T06:33:28Z

❗ Systemtests Failed (no tests results are present) ❗

ppatierno · 2020-04-23T07:24:26Z

@strimzi-ci run tests

strimzi-ci · 2020-04-23T09:47:52Z

✔️ Test Summary ✔️

TEST_PROFILE: acceptance
TEST_CASE: *ST
TOTAL: 30
PASS: 30
FAIL: 0
SKIP: 0
BUILD_NUMBER:

scholzj

Looks mostly good. Just some minor comments.

scholzj · 2020-04-23T09:53:05Z

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

+)
+@JsonInclude(JsonInclude.Include.NON_NULL)
+@JsonPropertyOrder({
+        "replicas", "image", "config",


I think you could remove the replicas from here.

scholzj · 2020-04-23T09:56:09Z

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

+                .withReadinessProbe(ModelUtils.createHttpProbe(readinessPath, REST_API_PORT_NAME, readinessProbeOptions))
+                .withResources(getResources())
+                .withVolumeMounts(getVolumeMounts())
+                .withImagePullPolicy(determineImagePullPolicy(imagePullPolicy, getImage()))


I think we need to add here the securityontext form the container templates.

scholzj · 2020-04-23T09:56:34Z

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

+                        .withCommand("/opt/stunnel/cruise_control_stunnel_pre_stop.sh",
+                                String.valueOf(templateTerminationGracePeriodSeconds))
+                        .endExec().endPreStop().build())
+                .withImagePullPolicy(determineImagePullPolicy(imagePullPolicy, tlsSidecarImage))


We need to add here the security context from the tlsSidecar container template.

scholzj · 2020-04-23T09:59:45Z

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

+import static io.strimzi.operator.cluster.model.VolumeUtils.createSecretVolume;
+import static io.strimzi.operator.cluster.model.VolumeUtils.createVolumeMount;
+
+public class CruiseControl extends AbstractModel {


Do we need to set some network policies on the CruiseControl ports?

scholzj · 2020-04-23T10:06:19Z

examples/kafka/kafka-cruise-control.yaml

+  name: my-cluster
+spec:
+  kafka:
+    version: 2.4.0


This should be 2.4.1 now.

d-laing · 2020-04-23T10:51:25Z

examples/kafka/kafka-cruise-control.yaml

+  entityOperator:
+    topicOperator: {}
+    userOperator: {}
+  cruiseControl: {}


I think we should provide a default CC configuration in the example file. We could provide the full list of goals and default goals so the user can remove any they don't need and/or fine-tune the constraints.

Otherwise, we're making the user work harder. They'll need to refer to the documentation to figure out how to set goals and capacity limits.

I don't mind if you merge this now and we continue the discussion in later PRs.

* Added security context and tests Signed-off-by: Paolo Patierno <ppatierno@live.com> * Added network policy for the cruise control REST API port Signed-off-by: Paolo Patierno <ppatierno@live.com>

ppatierno · 2020-04-23T16:29:14Z

@strimzi-ci run tests

strimzi-ci · 2020-04-23T16:58:13Z

❗ Systemtests Failed (no tests results are present) ❗

Signed-off-by: Paolo Patierno <ppatierno@live.com>

ppatierno · 2020-04-23T17:08:00Z

@strimzi-ci run profile=regression testcase=CruiseControlST

ppatierno · 2020-04-23T17:09:15Z

@strimzi-ci run tests

ppatierno · 2020-04-23T17:31:00Z

@strimzi-ci run tests profile=regression testcase=CruiseControlST

strimzi-ci · 2020-04-23T17:43:09Z

❗ Systemtests Failed (no tests results are present) ❗

strimzi-ci · 2020-04-23T18:03:29Z

❗ Systemtests Failed (no tests results are present) ❗

Signed-off-by: Paolo Patierno <ppatierno@live.com>

ppatierno · 2020-04-23T19:11:59Z

@strimzi-ci run tests

ppatierno · 2020-04-23T19:12:10Z

@strimzi-ci run tests profile=regression testcase=CruiseControlST

strimzi-ci · 2020-04-23T20:00:09Z

✔️ Test Summary ✔️

TEST_PROFILE: regression
TEST_CASE: CruiseControlST
TOTAL: 1
PASS: 1
FAIL: 0
SKIP: 0
BUILD_NUMBER:

ppatierno · 2020-04-23T20:09:17Z

@strimzi-ci run tests

strimzi-ci · 2020-04-23T22:50:29Z

✔️ Test Summary ✔️

TEST_PROFILE: acceptance
TEST_CASE: *ST
TOTAL: 30
PASS: 30
FAIL: 0
SKIP: 0
BUILD_NUMBER:

* Added Cruise Control System Tests - Added deployment with CC to Kafka ST resources - Added pod name and deployment resources - Added CC deployment system test Signed-off-by: Thomas Cooper <tcooper@redhat.com> * Cruise Control Deployment Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Add broker capacity estimation and configuration Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Add k8s memory parsing to return different byte multiples Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Refactor Capacity constructor Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Regenerate helm charts Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Addressing some comments Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Addressing more comments Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Addressing more comments (refactoring capacity API, checkOwnerReferences) Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Addressing comments ( capacity properties as strings/validate units in schema) Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Tightening capacity regex; fixing docs Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Another doc fix Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Add missing goal; bump CC version to 2.0.100 Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Fix logging and typos Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Addressing comments ( Update allowed disk capacity notation + refactoring) Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Fixing more typos Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Fixing another doc issue Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Securing communication between Cruise Control and Kafka (strimzi#37) * Adding TLS communication between CC and Kafka Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Adding TLS communication between metric reporter and Kafka Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Fixed metrics report to use TLS hostname verification (strimzi#38) Moved metrics topic to Cruise Control configuration Hiding truststore and keystore passwords in the log Signed-off-by: Paolo Patierno <ppatierno@live.com> Co-authored-by: Paolo Patierno <ppatierno@live.com> Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Removing CC replicas, refactor Dockerfile + CC dependencies Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Added security context, network policy and tests (strimzi#39) * Added security context and tests Signed-off-by: Paolo Patierno <ppatierno@live.com> * Added network policy for the cruise control REST API port Signed-off-by: Paolo Patierno <ppatierno@live.com> * Added CruiseControl ST to regression tests Signed-off-by: Paolo Patierno <ppatierno@live.com> * Fixed NPE on Cruise Control network policy Signed-off-by: Paolo Patierno <ppatierno@live.com> Co-authored-by: Thomas Cooper <tcooper@redhat.com> Co-authored-by: Paolo Patierno <ppatierno@live.com>

kyguy added this to the 0.18.0 milestone Apr 1, 2020

kyguy requested review from scholzj, tombentley, ppatierno and tomncooper April 1, 2020 16:46

samuel-hawker reviewed Apr 1, 2020

View reviewed changes

tombentley reviewed Apr 2, 2020

View reviewed changes

samuel-hawker reviewed Apr 2, 2020

View reviewed changes

kyguy requested a review from d-laing April 3, 2020 12:32

kyguy force-pushed the cruise-control-deployment branch 3 times, most recently from 25ae97a to dfeccf9 Compare April 6, 2020 23:22

kyguy requested a review from tombentley April 6, 2020 23:23

tombentley reviewed Apr 7, 2020

View reviewed changes

d-laing suggested changes Apr 8, 2020

View reviewed changes

d-laing added the documentation label Apr 8, 2020

kyguy requested review from tombentley, d-laing and samuel-hawker April 9, 2020 22:20

kyguy force-pushed the cruise-control-deployment branch from be06b74 to cf09d68 Compare April 15, 2020 15:33

tombentley approved these changes Apr 20, 2020

View reviewed changes

d-laing approved these changes Apr 20, 2020

View reviewed changes

kyguy force-pushed the cruise-control-deployment branch from cf09d68 to 0f5eb11 Compare April 20, 2020 19:57

ppatierno reviewed Apr 21, 2020

View reviewed changes

ppatierno removed request for scholzj and samuel-hawker April 21, 2020 06:28

kyguy force-pushed the cruise-control-deployment branch 2 times, most recently from d52a0be to e9ac04b Compare April 21, 2020 13:40

kyguy force-pushed the cruise-control-deployment branch from b57762a to 91ac7f2 Compare April 22, 2020 23:17

scholzj reviewed Apr 23, 2020

View reviewed changes

d-laing reviewed Apr 23, 2020

View reviewed changes

Added security context, network policy and tests (#39)

ee9b1a2

* Added security context and tests Signed-off-by: Paolo Patierno <ppatierno@live.com> * Added network policy for the cruise control REST API port Signed-off-by: Paolo Patierno <ppatierno@live.com>

scholzj approved these changes Apr 23, 2020

View reviewed changes

Added CruiseControl ST to regression tests

2bc4a05

Signed-off-by: Paolo Patierno <ppatierno@live.com>

Fixed NPE on Cruise Control network policy

38fb0cf

Signed-off-by: Paolo Patierno <ppatierno@live.com>

ppatierno merged commit 1a94d2e into strimzi:master Apr 24, 2020

ppatierno removed the documentation label Apr 29, 2020


		private final CruiseControl cc = CruiseControl.fromCrd(resource, VERSIONS);

		public void checkOwnerReference(OwnerReference ownerRef, HasMetadata resource) {

	@Description("The Cruise Control broker capacity config.")
	@Description("The Cruise Control broker capacity configuration.")

	Since {ProductName} brokers are homogenous, Cruise Control will apply the same capacity limits to every broker it is monitoring.
	Since {ProductName} brokers are homogeneous, Cruise Control will apply the same capacity limits to every broker it is monitoring.

	+ "cruise.control.metrics.reporter.bootstrap.servers, sasl. interceptor.classes";
	+ "cruise.control.metrics.reporter.bootstrap.servers, sasl.interceptor.classes";

	+ "partition.metric.sample.store.topic, broker.metric.sample.store.topic, brokerCapacity.config.file, skip.sample.store.topic.rack.awareness.check, cruise.control.metrics.topic"
	+ "partition.metric.sample.store.topic, broker.metric.sample.store.topic, brokerCapacity.config.file, skip.sample.store.topic.rack.awareness.check, cruise.control.metrics.topic,"

	@Description("The Cruise Control brokerCapacity configuration.")
	@Description("The Cruise Control `brokerCapacity` configuration.")

	" https://github.com/linkedin/cruise-control/wiki/Configurations Note that properties " +
	" https://github.com/linkedin/cruise-control/wiki/Configurations. Note that properties " +

	Brokers in a Kafka cluster can become unevenly loaded due to things like an uneven distribution of busy partitions.
	Brokers in a Kafka cluster can become unevenly loaded for many reasons. For example, partitions that handle large amounts of message traffic can be unevenly distributed across the available brokers.

	Cruise Control is a tool for simplifying the monitoring and rebalancing of partitions across a Kafka cluster.
	Cruise Control is a tool for automating the monitoring and rebalancing of partitions across a Kafka cluster.

	Deploy Cruise Control to your {ProductName} cluster through the `cruiseControl` property in the `Kafka` resource.
	Cruise Control is configured using the `cruiseControl` property in the `Kafka` resource. Once configured, you can deploy a Cruise Control instance to your {ProductName} cluster by creating or updating the `Kafka` resource.

	For example, an optimization that would cause a broker to exceed its CPU capacity would not be used for when the CpuCapacityGoal is set as a hard goal.
	For example, an optimization that would cause a broker to exceed its CPU capacity would not be used if the `CpuCapacityGoal` is set as a hard goal.

	Capacity limits for broker resources can be specified in the `brokerCapacity` property in `Kafka.spec.cruiseControl`.
	You specify capacity limits for broker resources in the `brokerCapacity` property in `Kafka.spec.cruiseControl` .

	* `outboundNetworkKiBPerSecond` - Newtork outbound throughput in KiB/s
	* `outboundNetworkKiBPerSecond` - Network outbound throughput in KiB/s

	Since {ProductName} brokers are homogeneous, Cruise Control will apply the same capacity limits to every broker it is monitoring.
	Because Kafka brokers are homogeneous, Cruise Control applies the same capacity limits to every broker it is monitoring.

	For information, refer to the xref:type-CruiseControlBrokerCapacity-reference[Capacity schema reference].
	For more information, refer to the xref:type-CruiseControlBrokerCapacity-reference[].

	@Pattern("^[0-9]+([KMGTPE]i?)?$")
	@Pattern("^[0-9]+([.][0-9]*)?([KMGTPE]i?\|e[0-9]+)?$")

		private Double brokerInboundNetworkKiBPerSecondCapacity;
		private Double brokerOuboundNetworkKiBPerSecondCapacity;

		List<ServicePort> ports = new ArrayList<>(1);
		ports.add(createServicePort(REST_API_PORT_NAME, REST_API_PORT, REST_API_PORT, "TCP"));

	List<Volume> volumeList = new ArrayList<>(1);
	List<Volume> volumeList = new ArrayList<>(3);

		Map<String, String> annotations = new HashMap<>();
		annotations.put(CruiseControl.ANNO_STRIMZI_IO_LOGGING, logAndMetricsConfigMap.getData().get(ANCILLARY_CM_KEY_LOG_CONFIG));

Cruise Control deployment #2773

Cruise Control deployment #2773

Conversation

kyguy commented Apr 1, 2020 • edited Loading

Type of change

Description

Checklist

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

kyguy Apr 2, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-laing 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

kyguy Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombentley 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

d-laing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-laing commented Apr 21, 2020

kyguy commented Apr 23, 2020

kyguy commented Apr 1, 2020 •

edited

Loading

kyguy Apr 2, 2020 •

edited

Loading

kyguy Apr 9, 2020 •

edited

Loading