-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Allow configuring additional options when restarting connectors #11797
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
Changes from 6 commits
66318c9
5deae11
85c3896
09837fa
b66a848
df9f746
ae3bc01
dce944f
26f6ac2
98ac310
a382a96
44d9ea4
0eccf8b
c0a4804
87aa2e2
8bafcf5
90a7725
fd2bdca
ccdb38e
5f7e4ac
827a81a
7ffac47
6b6c85d
0fd307c
62ffbc6
36bd179
2451683
7f94597
1f3c544
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 |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import io.fabric8.kubernetes.api.model.ConfigMap; | ||
| import io.fabric8.kubernetes.api.model.DefaultKubernetesResourceList; | ||
| import io.fabric8.kubernetes.api.model.HasMetadata; | ||
| import io.fabric8.kubernetes.api.model.LocalObjectReference; | ||
| import io.fabric8.kubernetes.api.model.ObjectMeta; | ||
| import io.fabric8.kubernetes.api.model.ServiceAccount; | ||
|
|
@@ -131,6 +132,17 @@ public abstract class AbstractConnectOperator<C extends KubernetesClient, T exte | |
| protected final SharedEnvironmentProvider sharedEnvironmentProvider; | ||
| protected final int port; | ||
|
|
||
| /** | ||
| * This optional argument can be used to include tasks in the restart connector operation. | ||
| * */ | ||
| protected static final String STRIMZI_IO_RESTART_INCLUDE_TASKS_ARG = "includeTasks"; | ||
|
|
||
| /** | ||
| * This optional argument can be used to restart connector only failed tasks. | ||
| * */ | ||
|
rlanhellas marked this conversation as resolved.
Outdated
|
||
| protected static final String STRIMZI_IO_RESTART_ONLY_FAILED_ARG = "onlyFailed"; | ||
|
|
||
|
|
||
|
rlanhellas marked this conversation as resolved.
Outdated
|
||
| /** | ||
| * Constructor | ||
| * | ||
|
|
@@ -746,8 +758,18 @@ private static int nextAutoRestartBackOffIntervalInMinutes(int restartCount) | |
| @SuppressWarnings({ "rawtypes" }) | ||
| private Future<List<Condition>> maybeRestartConnector(Reconciliation reconciliation, String host, KafkaConnectApi apiClient, String connectorName, CustomResource resource, List<Condition> conditions) { | ||
| if (hasRestartAnnotation(resource, connectorName)) { | ||
| LOGGER.debugCr(reconciliation, "Restarting connector {}", connectorName); | ||
| return VertxUtil.completableFutureToVertxFuture(apiClient.restart(host, port, connectorName, false, false)) | ||
|
|
||
| if (!restartAnnotationIsValid(resource)) { | ||
| LOGGER.warnCr(reconciliation, "Invalid annotation format"); | ||
| conditions.add(StatusUtils.buildWarningCondition("RestartConnector", "Invalid annotation format")); | ||
| return Future.succeededFuture(conditions); | ||
| } | ||
|
|
||
| boolean restartIncludeTasks = restartAnnotationHasIncludeTasksArg(resource); | ||
| boolean restartOnlyFailedTasks = restartAnnotationHasOnlyFailedTasksArg(resource); | ||
| LOGGER.infoCr(reconciliation, "Restarting connector {}, IncludeTasks {}, OnlyFailedTasks {}", connectorName, restartIncludeTasks, restartOnlyFailedTasks); | ||
|
|
||
| return VertxUtil.completableFutureToVertxFuture(apiClient.restart(host, port, connectorName, restartIncludeTasks, restartOnlyFailedTasks)) | ||
| .compose(ignored -> removeRestartAnnotation(reconciliation, resource) | ||
| .compose(v -> Future.succeededFuture(conditions)), | ||
| throwable -> { | ||
|
|
@@ -1059,6 +1081,36 @@ private Future<ConnectorStatusAndConditions> updateConnectorTopics(Reconciliatio | |
| @SuppressWarnings({ "rawtypes" }) | ||
| abstract boolean hasRestartAnnotation(CustomResource resource, String connectorName); | ||
|
|
||
| /** | ||
| * Checks if restart annotation value is valid | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has valid restart annotation. False otherwise. | ||
| * */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| abstract boolean restartAnnotationIsValid(CustomResource resource); | ||
|
|
||
| /** | ||
| * Checks whether the provided resource instance (a KafkaConnector or KafkaMirrorMaker2) has argument includeTasks in restart annotation. | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has argument includeTasks in restart annotation. False otherwise. | ||
| */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| abstract boolean restartAnnotationHasIncludeTasksArg(CustomResource resource); | ||
|
|
||
| /** | ||
| * Checks whether the provided resource instance (a KafkaConnector or KafkaMirrorMaker2) has argument onlyFailedTasks in restart annotation. | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has argument onlyFailedTasks in restart annotation. False otherwise. | ||
| */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| abstract boolean restartAnnotationHasOnlyFailedTasksArg(HasMetadata resource); | ||
|
Member
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. Did you check here and with the implementations if you can use
Contributor
Author
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 forgot to change the implementations, just did it |
||
|
|
||
| /** | ||
| * Returns the ID of the connector task to be restarted from the (a KafkaConnector or KafkaMirrorMaker2) custom resource. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| */ | ||
| package io.strimzi.operator.cluster.operator.assembly; | ||
|
|
||
| import io.fabric8.kubernetes.api.model.HasMetadata; | ||
| import io.fabric8.kubernetes.api.model.LabelSelector; | ||
| import io.fabric8.kubernetes.api.model.LabelSelectorBuilder; | ||
| import io.fabric8.kubernetes.api.model.LabelSelectorRequirement; | ||
|
|
@@ -62,6 +63,7 @@ | |
| import java.util.Set; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.function.Function; | ||
| import java.util.regex.Pattern; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import static io.strimzi.api.ResourceAnnotations.ANNO_STRIMZI_IO_CONNECTOR_OFFSETS; | ||
|
|
@@ -80,6 +82,13 @@ public class KafkaConnectAssemblyOperator extends AbstractConnectOperator<Kubern | |
| private final CrdOperator<KubernetesClient, KafkaConnector, KafkaConnectorList> connectorOperator; | ||
| private final ConnectBuildOperator connectBuildOperator; | ||
|
|
||
| /** | ||
| * Pattern for validation of restart connector annotation value. | ||
| * */ | ||
| private static final Pattern STRIMZI_IO_RESTART_ARGS_PATTERN = Pattern.compile("^includeTasks," + | ||
| "onlyFailed$|^onlyFailed,includeTasks$|^includeTasks$|^onlyFailed$|^true$"); | ||
|
Member
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 seems pretty hacky. Maybe you can split it by
Contributor
Author
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 changed the validation to if-else statement and removed the regex pattern |
||
|
|
||
|
|
||
| /** | ||
| * Constructor | ||
| * | ||
|
|
@@ -613,7 +622,56 @@ private Future<Void> maybeUpdateConnectorStatus(Reconciliation reconciliation, K | |
| @SuppressWarnings({ "rawtypes" }) | ||
| @Override | ||
| protected boolean hasRestartAnnotation(CustomResource resource, String connectorName) { | ||
| return Annotations.booleanAnnotation(resource, ANNO_STRIMZI_IO_RESTART, false); | ||
| return Annotations.stringAnnotation(resource, ANNO_STRIMZI_IO_RESTART, null) != null; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if restart annotation value has valid combination of values. These are valid and not valid combinations: | ||
| * strimzi.io/restart=includeTasks,onlyFailed # restart with args: includeTasks=true and onlyFailed=true | ||
| * strimzi.io/restart=includeTasks # restart with args: includeTasks=true and onlyFailed=false | ||
| * strimzi.io/restart=onlyFailed # restart with args: includeTasks=false and onlyFailed=true | ||
| * strimzi.io/restart=true # restart with args: includeTasks=false and onlyFailed=false | ||
| * strimzi.io/restart=false,includeTasks,onlyFailed # do not restart, fail and log error because you can't set args and boolean value together | ||
| * strimzi.io/restart=true,includeTasks,onlyFailed # do not restart, fail and log error because you can't set args and boolean value together | ||
| * strimzi.io/restart=includeTasks,wrongArg # do not restart, fail and log error because wrongArg is not supported | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has valid restart annotation. False otherwise. | ||
| * */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| protected boolean restartAnnotationIsValid(CustomResource resource) { | ||
| String restartValue = Annotations.stringAnnotation(resource, ANNO_STRIMZI_IO_RESTART, ""); | ||
| return STRIMZI_IO_RESTART_ARGS_PATTERN.matcher(restartValue).matches(); | ||
| } | ||
|
|
||
| /** | ||
| * Checks whether the provided resource instance (a KafkaConnector or KafkaMirrorMaker2) has argument includeTasks in restart annotation. | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has argument includeTasks in restart annotation. False otherwise. | ||
| */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| @Override | ||
| protected boolean restartAnnotationHasIncludeTasksArg(CustomResource resource) { | ||
| return Annotations.stringAnnotation(resource, ANNO_STRIMZI_IO_RESTART, "") | ||
| .contains(STRIMZI_IO_RESTART_INCLUDE_TASKS_ARG); | ||
| } | ||
|
|
||
|
|
||
|
rlanhellas marked this conversation as resolved.
Outdated
|
||
| /** | ||
|
rlanhellas marked this conversation as resolved.
|
||
| * Checks whether the provided resource instance (a KafkaConnector or KafkaMirrorMaker2) has argument onlyFailedTasks in restart annotation. | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has argument onlyFailedTasks in restart annotation. False otherwise. | ||
| */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| @Override | ||
| protected boolean restartAnnotationHasOnlyFailedTasksArg(HasMetadata resource) { | ||
| return Annotations.stringAnnotation(resource, ANNO_STRIMZI_IO_RESTART, "") | ||
| .contains(STRIMZI_IO_RESTART_ONLY_FAILED_ARG); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| */ | ||
| package io.strimzi.operator.cluster.operator.assembly; | ||
|
|
||
| import io.fabric8.kubernetes.api.model.HasMetadata; | ||
| import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBinding; | ||
| import io.fabric8.kubernetes.client.CustomResource; | ||
| import io.fabric8.kubernetes.client.KubernetesClient; | ||
|
|
@@ -42,6 +43,7 @@ | |
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.function.Function; | ||
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import static io.strimzi.api.ResourceAnnotations.ANNO_STRIMZI_IO_CONNECTOR_OFFSETS; | ||
|
|
@@ -63,6 +65,14 @@ | |
| public class KafkaMirrorMaker2AssemblyOperator extends AbstractConnectOperator<KubernetesClient, KafkaMirrorMaker2, KafkaMirrorMaker2List, KafkaMirrorMaker2Spec, KafkaMirrorMaker2Status> { | ||
| private static final ReconciliationLogger LOGGER = ReconciliationLogger.create(KafkaMirrorMaker2AssemblyOperator.class.getName()); | ||
|
|
||
| /** | ||
| * Pattern for validation of MirrorMaker2 restart connector annotation value. | ||
| * */ | ||
| private static final Pattern STRIMZI_IO_RESTART_CONNECTOR_MM2_ARGS_PATTERN = Pattern | ||
| .compile("^([a-zA-Z0-9-_]+):includeTasks,onlyFailed$|^([a-zA-Z0-9-_]+):onlyFailed,includeTasks$" + | ||
| "|^([a-zA-Z0-9-_]+):includeTasks$|^([a-zA-Z0-9-_]+):onlyFailed$|^([a-zA-Z0-9-_]+)$"); | ||
|
Member
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. Same as in the other expression. Maybe we can try to simplify it a bit? Parse our the comnand part using the Regexp and than split it rather than including all various combiantions? This does not scale very well. In addition, whatever the way to parse it is, it might be good to have some unit tests for how it parses the various annotations (both here as well as for connect). With some valid and invalid values etc.
Contributor
Author
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 changed the validation to if-else statement and removed the regex pattern
Contributor
Author
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. Btw, regarding testing invalid args, you can check the tests, I added a test to invalid args
Member
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 see. It is probably good to test it with invalid value once in the way you have it. But why don't you just test the methods for validating the annotations / checking the presence of the arguments? You can easily test them with 10 different valid and invalid values without the need for any mocks or anything. That lets you very quickly test all the possibly annotation forms with typos, spaces, etc.
Contributor
Author
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 created a unit test for both kafka connect and MM2 annotation values with many cases to test the maximum of validations we can. Please take a look again. |
||
|
|
||
|
|
||
|
rlanhellas marked this conversation as resolved.
Outdated
|
||
| /** | ||
| * Constructor | ||
| * | ||
|
|
@@ -311,7 +321,54 @@ private Future<Void> maybeUpdateMirrorMaker2Status(Reconciliation reconciliation | |
| @Override | ||
| protected boolean hasRestartAnnotation(CustomResource resource, String connectorName) { | ||
| String restartAnnotationConnectorName = Annotations.stringAnnotation(resource, ANNO_STRIMZI_IO_RESTART_CONNECTOR, null); | ||
| return connectorName.equals(restartAnnotationConnectorName); | ||
| return restartAnnotationConnectorName != null && restartAnnotationConnectorName.contains(connectorName); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if restart annotation value has valid combination of values. These are valid and not valid combinations: | ||
| * strimzi.io/restart-connector=mirrormaker_connector_name:includeTasks,onlyFailed # restart with args: includeTasks=true and onlyFailed=true | ||
| * strimzi.io/restart-connector=mirrormaker_connector_name:includeTasks # restart with args: includeTasks=true and onlyFailed=false | ||
| * strimzi.io/restart-connector=mirrormaker_connector_name:onlyFailed # restart with args: includeTasks=false and onlyFailed=true | ||
| * strimzi.io/restart-connector=mirrormaker_connector_name # restart with args: includeTasks=false and onlyFailed=false | ||
| * strimzi.io/restart-connector=includeTasks,onlyFailed # do not restart, fail and log error because connector name is required | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has valid restart annotation. False otherwise. | ||
| * */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| protected boolean restartAnnotationIsValid(CustomResource resource) { | ||
| String restartValue = Annotations.stringAnnotation(resource, ANNO_STRIMZI_IO_RESTART_CONNECTOR, ""); | ||
| return STRIMZI_IO_RESTART_CONNECTOR_MM2_ARGS_PATTERN.matcher(restartValue).matches(); | ||
| } | ||
|
|
||
| /** | ||
| * Checks whether the provided resource instance (a KafkaConnector or KafkaMirrorMaker2) has argument includeTasks in restart annotation. | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has argument includeTasks in restart annotation. False otherwise. | ||
| */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| @Override | ||
| protected boolean restartAnnotationHasIncludeTasksArg(CustomResource resource) { | ||
| return Annotations.stringAnnotation(resource, ANNO_STRIMZI_IO_RESTART_CONNECTOR, "") | ||
| .contains(STRIMZI_IO_RESTART_INCLUDE_TASKS_ARG); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Checks whether the provided resource instance (a KafkaConnector or KafkaMirrorMaker2) has argument onlyFailedTasks in restart annotation. | ||
| * | ||
| * @param resource Resource instance to check | ||
| * | ||
| * @return True if the provided resource has argument onlyFailedTasks in restart annotation. False otherwise. | ||
| */ | ||
| @SuppressWarnings({ "rawtypes" }) | ||
| @Override | ||
| protected boolean restartAnnotationHasOnlyFailedTasksArg(HasMetadata resource) { | ||
| return Annotations.stringAnnotation(resource, ANNO_STRIMZI_IO_RESTART_CONNECTOR, "") | ||
| .contains(STRIMZI_IO_RESTART_ONLY_FAILED_ARG); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.