-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move cruise control server, capacity and server logging metrics config to ConfigMap #8977
Changes from 6 commits
98a2e20
6f20368
7b52835
faa0a53
8d7f676
e154298
5991a20
360d93c
4df82a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -40,6 +40,7 @@ | |||||
import io.strimzi.operator.cluster.model.metrics.SupportsMetrics; | ||||||
import io.strimzi.operator.cluster.model.securityprofiles.ContainerSecurityProviderContextImpl; | ||||||
import io.strimzi.operator.cluster.model.securityprofiles.PodSecurityProviderContextImpl; | ||||||
import io.strimzi.operator.common.Annotations; | ||||||
import io.strimzi.operator.common.Reconciliation; | ||||||
import io.strimzi.operator.common.Util; | ||||||
import io.strimzi.operator.common.model.Labels; | ||||||
|
@@ -91,15 +92,34 @@ public class CruiseControl extends AbstractModel implements SupportsMetrics, Sup | |||||
protected static final String TLS_CC_CERTS_VOLUME_MOUNT = "/etc/cruise-control/cc-certs/"; | ||||||
protected static final String TLS_CA_CERTS_VOLUME_NAME = "cluster-ca-certs"; | ||||||
protected static final String TLS_CA_CERTS_VOLUME_MOUNT = "/etc/cruise-control/cluster-ca-certs/"; | ||||||
protected static final String LOG_AND_METRICS_CONFIG_VOLUME_NAME = "cruise-control-metrics-and-logging"; | ||||||
protected static final String LOG_AND_METRICS_CONFIG_VOLUME_MOUNT = "/opt/cruise-control/custom-config/"; | ||||||
protected static final String CONFIG_VOLUME_NAME = "config"; | ||||||
/** | ||||||
* Server config file name | ||||||
*/ | ||||||
public static final String SERVER_CONFIG_FILENAME = "cruisecontrol.properties"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does that explanation make sense, does it sound reasonable to you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, makes sense. Thanks! |
||||||
/** | ||||||
* Capacity config file name | ||||||
*/ | ||||||
public static final String CAPACITY_CONFIG_FILENAME = "capacity.json"; | ||||||
protected static final String 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 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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
*/ | ||||||
public static final String ANNO_STRIMZI_SERVER_CONFIGURATION_HASH = Annotations.STRIMZI_DOMAIN + "server-configuration-hash"; | ||||||
|
||||||
/** | ||||||
* 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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
*/ | ||||||
public static final String ANNO_STRIMZI_CAPACITY_CONFIGURATION_HASH = Annotations.STRIMZI_DOMAIN + "capacity-configuration-hash"; | ||||||
|
||||||
// Configuration defaults | ||||||
protected static final boolean DEFAULT_CRUISE_CONTROL_METRICS_ENABLED = false; | ||||||
|
||||||
|
@@ -125,11 +145,8 @@ public class CruiseControl extends AbstractModel implements SupportsMetrics, Sup | |||||
} | ||||||
|
||||||
// Cruise Control configuration keys (EnvVariables) | ||||||
protected static final String ENV_VAR_CRUISE_CONTROL_CONFIGURATION = "CRUISE_CONTROL_CONFIGURATION"; | ||||||
protected static final String ENV_VAR_STRIMZI_KAFKA_BOOTSTRAP_SERVERS = "STRIMZI_KAFKA_BOOTSTRAP_SERVERS"; | ||||||
|
||||||
protected static final String ENV_VAR_CRUISE_CONTROL_CAPACITY_CONFIGURATION = "CRUISE_CONTROL_CAPACITY_CONFIGURATION"; | ||||||
|
||||||
protected static final String ENV_VAR_API_SSL_ENABLED = "STRIMZI_CC_API_SSL_ENABLED"; | ||||||
protected static final String ENV_VAR_API_AUTH_ENABLED = "STRIMZI_CC_API_AUTH_ENABLED"; | ||||||
protected static final String ENV_VAR_API_USER = "API_USER"; | ||||||
|
@@ -311,27 +328,28 @@ protected List<Volume> getVolumes(boolean isOpenShift) { | |||||
createSecretVolume(TLS_CC_CERTS_VOLUME_NAME, CruiseControlResources.secretName(cluster), isOpenShift), | ||||||
createSecretVolume(TLS_CA_CERTS_VOLUME_NAME, AbstractModel.clusterCaCertSecretName(cluster), isOpenShift), | ||||||
createSecretVolume(API_AUTH_CONFIG_VOLUME_NAME, CruiseControlResources.apiSecretName(cluster), isOpenShift), | ||||||
createConfigMapVolume(LOG_AND_METRICS_CONFIG_VOLUME_NAME, CruiseControlResources.logAndMetricsConfigMapName(cluster))); | ||||||
createConfigMapVolume(CONFIG_VOLUME_NAME, CruiseControlResources.configMapName(cluster))); | ||||||
} | ||||||
|
||||||
protected List<VolumeMount> getVolumeMounts() { | ||||||
return List.of(VolumeUtils.createTempDirVolumeMount(), | ||||||
createVolumeMount(CruiseControl.TLS_CC_CERTS_VOLUME_NAME, CruiseControl.TLS_CC_CERTS_VOLUME_MOUNT), | ||||||
createVolumeMount(CruiseControl.TLS_CA_CERTS_VOLUME_NAME, CruiseControl.TLS_CA_CERTS_VOLUME_MOUNT), | ||||||
createVolumeMount(CruiseControl.API_AUTH_CONFIG_VOLUME_NAME, CruiseControl.API_AUTH_CONFIG_VOLUME_MOUNT), | ||||||
createVolumeMount(LOG_AND_METRICS_CONFIG_VOLUME_NAME, LOG_AND_METRICS_CONFIG_VOLUME_MOUNT)); | ||||||
createVolumeMount(CONFIG_VOLUME_NAME, CONFIG_VOLUME_MOUNT)); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Generates Kubernetes Deployment for Cruise Control | ||||||
* | ||||||
* @param annotations Map with annotations | ||||||
* @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 | ||||||
*/ | ||||||
public Deployment generateDeployment(boolean isOpenShift, ImagePullPolicy imagePullPolicy, List<LocalObjectReference> imagePullSecrets) { | ||||||
public Deployment generateDeployment(Map<String, String> annotations, boolean isOpenShift, ImagePullPolicy imagePullPolicy, List<LocalObjectReference> imagePullSecrets) { | ||||||
return WorkloadUtils.createDeployment( | ||||||
componentName, | ||||||
namespace, | ||||||
|
@@ -346,7 +364,7 @@ public Deployment generateDeployment(boolean isOpenShift, ImagePullPolicy imageP | |||||
labels, | ||||||
templatePod, | ||||||
DEFAULT_POD_LABELS, | ||||||
Map.of(), | ||||||
annotations, | ||||||
templatePod != null ? templatePod.getAffinity() : null, | ||||||
null, | ||||||
List.of(createContainer(imagePullPolicy)), | ||||||
|
@@ -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))); | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary empty line. |
||||||
varList.add(ContainerUtils.createEnvVar(ENV_VAR_CRUISE_CONTROL_CAPACITY_CONFIGURATION, capacity.toString())); | ||||||
|
||||||
varList.add(ContainerUtils.createEnvVar(ENV_VAR_API_SSL_ENABLED, String.valueOf(this.sslEnabled))); | ||||||
varList.add(ContainerUtils.createEnvVar(ENV_VAR_API_AUTH_ENABLED, String.valueOf(this.authEnabled))); | ||||||
|
@@ -392,10 +409,6 @@ protected List<EnvVar> getEnvVars() { | |||||
JvmOptionUtils.jvmPerformanceOptions(varList, jvmOptions); | ||||||
JvmOptionUtils.jvmSystemProperties(varList, jvmOptions); | ||||||
|
||||||
if (configuration != null && !configuration.getConfiguration().isEmpty()) { | ||||||
varList.add(ContainerUtils.createEnvVar(ENV_VAR_CRUISE_CONTROL_CONFIGURATION, configuration.getConfiguration())); | ||||||
} | ||||||
|
||||||
// Add shared environment variables used for all containers | ||||||
varList.addAll(sharedEnvironmentProvider.variables()); | ||||||
|
||||||
|
@@ -498,25 +511,6 @@ public NetworkPolicy generateNetworkPolicy(String operatorNamespace, Labels oper | |||||
); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Generates a metrics and logging ConfigMap according to the configuration. If this operand doesn't support logging | ||||||
* or metrics, they will nto be set. | ||||||
* | ||||||
* @param metricsAndLogging The external CMs with logging and metrics configuration | ||||||
* | ||||||
* @return The generated ConfigMap | ||||||
*/ | ||||||
public ConfigMap generateMetricsAndLogConfigMap(MetricsAndLogging metricsAndLogging) { | ||||||
return ConfigMapUtils | ||||||
.createConfigMap( | ||||||
CruiseControlResources.logAndMetricsConfigMapName(cluster), | ||||||
namespace, | ||||||
labels, | ||||||
ownerReference, | ||||||
ConfigMapUtils.generateMetricsAndLogConfigMapData(reconciliation, this, metricsAndLogging) | ||||||
); | ||||||
} | ||||||
|
||||||
/** | ||||||
* @return Metrics Model instance for configuring Prometheus metrics | ||||||
*/ | ||||||
|
@@ -530,4 +524,31 @@ public MetricsModel metrics() { | |||||
public LoggingModel logging() { | ||||||
return logging; | ||||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* Generates a ConfigMap with the following: | ||||||
* | ||||||
* (1) Cruise Control server configuration | ||||||
* (2) Cruise Control broker capacity configuration | ||||||
* (3) Cruise Control server metrics and logging configuration | ||||||
* | ||||||
* @param metricsAndLogging The logging and metrics configuration | ||||||
* | ||||||
* @return The generated data | ||||||
*/ | ||||||
public ConfigMap generateConfigMap(MetricsAndLogging metricsAndLogging) { | ||||||
Map<String, String> configMapData = new HashMap<>(); | ||||||
configMapData.put(SERVER_CONFIG_FILENAME, configuration.asOrderedProperties().asPairs()); | ||||||
configMapData.put(CAPACITY_CONFIG_FILENAME, capacity.toString()); | ||||||
configMapData.putAll(ConfigMapUtils.generateMetricsAndLogConfigMapData(reconciliation, this, metricsAndLogging)); | ||||||
|
||||||
return ConfigMapUtils | ||||||
.createConfigMap( | ||||||
CruiseControlResources.configMapName(cluster), | ||||||
namespace, | ||||||
labels, | ||||||
ownerReference, | ||||||
configMapData | ||||||
); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
import java.util.TreeMap; | ||
import java.util.concurrent.TimeUnit; | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. non necessary blank line. |
||
/** | ||
* Class for handling Cruise Control configuration passed by the user | ||
*/ | ||
|
@@ -90,7 +91,8 @@ public class CruiseControlConfiguration extends AbstractConfiguration { | |
Map.entry(CruiseControlConfigurationParameters.WEBSERVER_SSL_ENABLE.getValue(), Boolean.toString(CruiseControlConfigurationParameters.DEFAULT_WEBSERVER_SSL_ENABLED)), | ||
Map.entry(CruiseControlConfigurationParameters.PARTITION_METRIC_TOPIC_NAME.getValue(), CruiseControlConfigurationParameters.DEFAULT_PARTITION_METRIC_TOPIC_NAME), | ||
Map.entry(CruiseControlConfigurationParameters.BROKER_METRIC_TOPIC_NAME.getValue(), CruiseControlConfigurationParameters.DEFAULT_BROKER_METRIC_TOPIC_NAME), | ||
Map.entry(CruiseControlConfigurationParameters.METRIC_REPORTER_TOPIC_NAME.getValue(), CruiseControlConfigurationParameters.DEFAULT_METRIC_REPORTER_TOPIC_NAME) | ||
Map.entry(CruiseControlConfigurationParameters.METRIC_REPORTER_TOPIC_NAME.getValue(), CruiseControlConfigurationParameters.DEFAULT_METRIC_REPORTER_TOPIC_NAME), | ||
Map.entry(CruiseControlConfigurationParameters.CAPACITY_CONFIG_FILE.getValue(), CruiseControl.CONFIG_VOLUME_MOUNT + CruiseControl.CAPACITY_CONFIG_FILENAME) | ||
))); | ||
|
||
private static final List<String> FORBIDDEN_PREFIXES = AbstractConfiguration.splitPrefixesToList(CruiseControlSpec.FORBIDDEN_PREFIXES); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,6 +147,16 @@ public class Capacity { | |
*/ | ||
public static final String CPU_KEY = "CPU"; | ||
|
||
/** | ||
* Inbound network key | ||
*/ | ||
public static final String INBOUND_NETWORK_KEY = "NW_IN"; | ||
|
||
/** | ||
* Outbound network key | ||
*/ | ||
public static final String OUTBOUND_NETWORK_KEY = "NW_OUT"; | ||
Comment on lines
+150
to
+158
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably does not have to be fixed in this PR right now. But I think the fact that you have to make this public because of tests of a different class instead of just marking it as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This. The class/package structure for CC files needs to be refactored, will do it in a follow up PR |
||
|
||
/** | ||
* Resource type | ||
*/ | ||
|
@@ -155,8 +165,6 @@ public class Capacity { | |
private static final String KAFKA_MOUNT_PATH = "/var/lib/kafka"; | ||
private static final String KAFKA_LOG_DIR = "kafka-log"; | ||
private static final String BROKER_ID_KEY = "brokerId"; | ||
private static final String INBOUND_NETWORK_KEY = "NW_IN"; | ||
private static final String OUTBOUND_NETWORK_KEY = "NW_OUT"; | ||
private static final String DOC_KEY = "doc"; | ||
|
||
private enum ResourceRequirementType { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,12 @@ protected static String milliCpuToCpu(int milliCPU) { | |
return String.valueOf(milliCPU / 1000.0); | ||
} | ||
|
||
protected JsonObject getJson() { | ||
/** | ||
* Returns CpuCapacity object as a JsonObject | ||
* | ||
* @return The CpuCapacity object as a JsonObject | ||
*/ | ||
public JsonObject getJson() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this, it looks like |
||
return new JsonObject().put(CORES_KEY, this.cores); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also update the javadoc.