feat: Add annotations to perform connector/task restart operations #4114

ajborley · 2020-12-15T21:59:55Z

Type of change

Enhancement / new feature

Description

This PR implements the first part of the 'Restarting Kafka Connect connectors and tasks' proposal (https://github.com/strimzi/proposals/blob/master/007-restarting-kafka-connect-connectors-and-tasks.md),
adding new annotations that cause the operator to restart connectors or tasks. The annotations can be applied to the KafkaConnector, and the KafkaMirrorMaker2 custom resources. The annotation acts as a trigger for a single restart call
by the operator, and is removed from the CR when the restart REST API call is successfully called.

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

strimzi-ci · 2020-12-15T22:01:03Z

Can one of the admins verify this patch?
Run tests: @strimzi-ci run tests

scholzj

Thanks for the PR. This looks good to me. I left one question. You should also add it to the CHANGELOG.md under 0.21.0 release. And it would be also great to have at least some very simple doc to know how should this be used.

scholzj · 2020-12-16T00:22:25Z

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

+                            log.warn("{}: Failed to restart connector {}. {}", reconciliation, connectorName, throwable.getMessage());
+                            return Future.succeededFuture();
+                        })
+                    .compose(ignored -> apiClient.statusWithBackOff(new BackOff(200L, 2, 10), host, port,


Not sure I understand why do we call this at the end. Could you please explain?

I'm guessing because ultimately maybeCreateOrUpdateConnector needs to return the status. But it would probably be better to make pauseResume, maybeRestartConnector and maybeRestartConnectorTask all return Future<Void> and make the call the apiClient.statusWithBackOff directly from maybeCreateOrUpdateConnector.

Yeah, that is why I wondered about this. If it is about the status for the KafkaConnector status field, it should be probably called only at the end.

Thanks for the review - Tom is correct here & I have updated to move the apiClient.statusWithBackOff call to the end of the compose chains.

tombentley · 2020-12-16T10:01:29Z

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

+     * @return the ID of the task to be restarted if the provided KafkaConnector resource instance has the strimzio.io/restart-task annotation or -1 otherwise.
+     */
+    protected int getRestartTaskAnnotationTaskID(CustomResource resource, String connectorName) {
+        return Annotations.intAnnotation(resource, ANNO_STRIMZI_IO_RESTART_TASK, -1);


I wonder if we should support a list of ids. It would make it simpler to restart several failed tasks.

Thanks for the review. I have started looking into this - it should be possible to restart tasks on a KafkaConnector resource using an annotation like: strimzi.io/restart-tasks: "0 1 2", although it does raise some questions:

what should happen if one of the restart calls fails (e.g. maybe there is no task 2)? Should the annotation be altered to only include the task IDs that were not restarted, so that they can be attempted on the next reconcile?

what should the annotation look like for KafkaMirrorMaker2 resources? The code currently supports annotations that address a single MM2 connector task like: strimzi.io/restart-connector-tasks: "<MM2-connector-name>:<task-id>". Do you think it should allow multiple MM2 connectors to be specified? Or just multiple tasks for a single connector? For example strimzi.io/restart-connector-tasks: "<MM2-connector-name>:0 1 2"

These are good points. I guess it might be a good idea to leave this idea for now and see how this stuff is used in practice. It seems like it would be easy enough to add at a later date.

tombentley · 2020-12-16T10:04:05Z

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

+                            log.warn("{}: Failed to restart connector {}. {}", reconciliation, connectorName, throwable.getMessage());
+                            return Future.succeededFuture();
+                        })
+                    .compose(ignored -> apiClient.statusWithBackOff(new BackOff(200L, 2, 10), host, port,


I'm guessing because ultimately maybeCreateOrUpdateConnector needs to return the status. But it would probably be better to make pauseResume, maybeRestartConnector and maybeRestartConnectorTask all return Future<Void> and make the call the apiClient.statusWithBackOff directly from maybeCreateOrUpdateConnector.

ajborley · 2020-12-16T22:32:24Z

Latest commit adds the CHANGELOG update & some initial documentation. It also updates the code to move the apiClient.statusWithBackOff call to the end of the compose chains.

tombentley · 2020-12-17T08:35:27Z

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

+                    .compose(ignored -> removeRestartTaskAnnotation(reconciliation, resource),
+                        throwable -> {
+                            // Ignore restart failures - just try again on the next reconcile
+                            log.warn("{}: Failed to restart connector task {}:{}. {}", reconciliation, connectorName, taskID, throwable.getMessage());


I have my doubts about this, particularly in the case that the connector task doesn't exist (i.e. bad id, presumably an http 404). If it's only in the logs then the user might not realise why the annotation wasn't removed. For transient errors it will likely succeed, so leaving it in the annotation for another try is not a bad idea. But for permanent errors like non-existence its more of a problem, especially if the user doesn't have access to the logs. I think we should propagate the error as a warning in the status.

The error case doesn't seem to be covered by the tests.

The latest commit adds warning status conditions to the CR when the restart or restartTask REST calls fail. Have also added failure test cases.

scholzj

One nit. But LGTM otherwise.

documentation/assemblies/deploying/assembly-deploy-kafka-connect.adoc

scholzj

Thanks for the PR.

scholzj · 2020-12-24T16:28:47Z

/azp run regression

azure-pipelines · 2020-12-24T16:28:57Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2020-12-24T22:21:33Z

(Just FYI: The failing regression test is not related to this PR)

tombentley · 2021-01-04T09:05:12Z

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

@@ -476,28 +480,34 @@ protected KafkaConnectApi getKafkaConnectApi() {
     * @param apiClient The client instance.
     * @param connectorName The connector name.
     * @param connectorSpec The desired connector spec.
+     * @param resource The resource that defines the connector.
     * @return A Future whose result, when successfully completed, is a map of the current connector state.


Needs amending

tombentley · 2021-01-04T09:08:03Z

documentation/modules/proc-manual-restart-connector.adoc

+. To restart the connector, annotate the `KafkaConnector` resource in Kubernetes.
+For example, using `kubectl annotate`:
+[source,shell,subs=+quotes]
+kubectl annotate KafkaConnector _KafkaConnector-name_ strimzi.io/restart=true


I wonder if this should be restart-connector, so it's consistent with restart-task?

Similar questions around this were raised in the proposal discussion:

Add connector and connector task restart proposal proposals#8 (comment)

Add connector and connector task restart proposal proposals#8 (comment)

Add connector and connector task restart proposal proposals#8 (comment)

For KafkaConnnector this PR has the 2 annotations which apply to the resource they are being applied to:

strimzi.io/restart=true

strimzi.io/restart-task=<TASK-ID>

And for KafkaMirrorMaker2 this PR supports annotations that refer to a single connector or task amongst potentially many that are managed by the resource:

strimzi.io/restart-connector=<MM2-CONNECTOR-NAME>

strimzi.io/restart-connector-task=<MM2-CONNECTOR-NAME>:<TASK-ID>

With this style it's clear which annotations apply to which resource (and which connector/task is being restarted).

If people think it would be clearer, I am fine to change this to use the same annotations for both KafkaConnnector and KafkaMirrorMaker2, - e.g:

For KafkaConnnector:

strimzi.io/restart-connector=true

strimzi.io/restart-connector-task=<TASK-ID>

For KafkaMirrorMaker2:

strimzi.io/restart-connector=<MM2-CONNECTOR-NAME>

strimzi.io/restart-connector-task=<MM2-CONNECTOR-NAME>:<TASK-ID>

What do you think?

I think I prefer the second option, even though it means the values are different between KafkaConnector and KMM2 while the keys are the same. WDYT @ppatierno ?

Not sure, I think I prefer the first option but I have not a strong opinion tbh. Both of them are valid. The doubt on the second option is around the fact that it could be confusing for users to see the same annotation names but getting different values (bool vs string) even if in two different resources. Anyway, I am fine with both, in the end it's a matter of documenting the difference.

tombentley

A nit and a question, but otherwise lgtm.

scholzj · 2021-01-09T15:53:59Z

A nit and a question, but otherwise lgtm.

@tombentley @ajborley Is there still something to do on this PR? Or can it be merged now? From the last comment it is not completely clear to me if there is still something to do here or not.

ajborley · 2021-01-09T20:11:38Z

A nit and a question, but otherwise lgtm.

@tombentley @ajborley Is there still something to do on this PR? Or can it be merged now? From the last comment it is not completely clear to me if there is still something to do here or not.

There may be another commit to use the same annotation keys for both KafkaConnectors and KafkaMirrorMaker2s - I was waiting for @ppatierno to respond to Tom’s latest comment (#4114 (comment)) as similar thoughts were discussed in the proposal PR:

scholzj · 2021-01-10T00:21:30Z

@ppatierno Please have a look ^^^

ppatierno · 2021-01-10T14:30:20Z

@ajborley @scholzj sorry I didn't notice the question. Just left a comment.

ppatierno

Other than the comment I left, the PR looks good to me anyway.

ajborley · 2021-01-12T07:41:53Z

We have one vote for using the same annotation keys and one vote for leaving the PR as-is with different annotation keys for KafkaConnector and KafkaMirrorMaker2 resources. I don’t have strong feelings either way!
@scholzj do you want to have the casting vote here? 🙂

- This commit implements the first part of the 'Restarting Kafka Connect connectors and tasks' proposal (https://github.com/strimzi/proposals), adding new annotations that cause the operator to restart connectors or tasks. The annotations can be applied to the KafkaConnector, and the KafkaMirrorMaker2 custom resources. The annotation acts as a trigger for a single restart call by the operator, and is removed from the CR when the restart REST API call is successfully called. Signed-off-by: Andrew Borley <borley@uk.ibm.com>

- Add doc, CHANGELOG - Reduce calls to connector status. Signed-off-by: Andrew Borley <borley@uk.ibm.com>

Signed-off-by: Andrew Borley <borley@uk.ibm.com>

- Also fix accidental doc deletion. Signed-off-by: Andrew Borley <borley@uk.ibm.com>

Signed-off-by: Andrew Borley <borley@uk.ibm.com>

scholzj · 2021-01-12T09:45:58Z

I talked about it with Tom and Paolo and I guess it is fine as it is. I rebased it to solve the conflict and if the tests pass I will merge it. Great PR ... thanks.

scholzj · 2021-01-12T11:39:29Z

/azp run regression

azure-pipelines · 2021-01-12T11:39:39Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2021-01-12T16:00:48Z

Thanks for the PR.

tomasz-zylka-fluke · 2021-01-13T09:30:22Z

@scholzj was it included in 0.21 release? thanks!

scholzj · 2021-01-13T09:31:45Z

I will include it if we find some bugs to do RC2. But if the RC1 is released as GA it will be in 0.22.

tomasz-zylka-fluke · 2021-02-10T13:21:16Z

hi @scholzj,
our understanding is that #4114 was not part of 0.21 release, therefore the logical question ;) would be when do you plan to release 0.22 - will it be this month (February) ?
thanks!

scholzj · 2021-02-10T13:46:52Z

I think end-of-February is still the current plan. We need to finish the preparation for the CRDv1 ... I think once that is ready we will start with the release..

scholzj added this to the 0.21.0 milestone Dec 16, 2020

scholzj requested review from tombentley and scholzj December 16, 2020 00:15

scholzj reviewed Dec 16, 2020

View reviewed changes

tombentley reviewed Dec 16, 2020

View reviewed changes

ajborley force-pushed the restart-connectors-and-tasks branch from eefc53e to 40ab8ed Compare December 16, 2020 22:28

tombentley reviewed Dec 17, 2020

View reviewed changes

scholzj reviewed Dec 17, 2020

View reviewed changes

documentation/assemblies/deploying/assembly-deploy-kafka-connect.adoc Show resolved Hide resolved

ajborley force-pushed the restart-connectors-and-tasks branch from e3717e2 to 456fe3e Compare December 23, 2020 15:38

scholzj approved these changes Dec 24, 2020

View reviewed changes

tombentley reviewed Jan 4, 2021

View reviewed changes

tombentley approved these changes Jan 4, 2021

View reviewed changes

ajborley force-pushed the restart-connectors-and-tasks branch from 456fe3e to 274a8cc Compare January 4, 2021 09:59

ppatierno approved these changes Jan 10, 2021

View reviewed changes

scholzj modified the milestones: 0.21.0, 0.22.0 Jan 12, 2021

ajborley added 3 commits January 12, 2021 10:43

Address review comments

42cb464

- Add doc, CHANGELOG - Reduce calls to connector status. Signed-off-by: Andrew Borley <borley@uk.ibm.com>

Fix doc references

c0cc4a7

Signed-off-by: Andrew Borley <borley@uk.ibm.com>

ajborley added 2 commits January 12, 2021 10:44

Add warning condition to status on restart failure

c0c99b1

- Also fix accidental doc deletion. Signed-off-by: Andrew Borley <borley@uk.ibm.com>

Update javadoc

d81b208

Signed-off-by: Andrew Borley <borley@uk.ibm.com>

scholzj force-pushed the restart-connectors-and-tasks branch from 274a8cc to d81b208 Compare January 12, 2021 09:44

scholzj mentioned this pull request Jan 12, 2021

allow configuration of health check for livenessProbe #1479

Closed

scholzj merged commit c292f1e into strimzi:master Jan 12, 2021

ajborley deleted the restart-connectors-and-tasks branch January 12, 2021 16:06

This was referenced Feb 16, 2021

[DOC] Edits to connector/task restart operations doc #4427

Merged

Automatic restarts of Kafka Connectors by the operator #2621

Closed

alok87 mentioned this pull request Feb 12, 2022

Kafkaconnect does not auto recover on failures practo/tipoca-stream#121

Open

feat: Add annotations to perform connector/task restart operations #4114

feat: Add annotations to perform connector/task restart operations #4114

Conversation

ajborley commented Dec 15, 2020 • edited

Type of change

Description

Checklist

strimzi-ci commented Dec 15, 2020

scholzj 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

ajborley Dec 16, 2020 • 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

ajborley commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

scholzj commented Dec 24, 2020

azure-pipelines bot commented Dec 24, 2020

scholzj commented Dec 24, 2020

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

tombentley left a comment

Choose a reason for hiding this comment

scholzj commented Jan 9, 2021

ajborley commented Jan 9, 2021

scholzj commented Jan 10, 2021

ppatierno commented Jan 10, 2021

ppatierno left a comment

Choose a reason for hiding this comment

ajborley commented Jan 12, 2021

scholzj commented Jan 12, 2021

scholzj commented Jan 12, 2021

azure-pipelines bot commented Jan 12, 2021

scholzj commented Jan 12, 2021

tomasz-zylka-fluke commented Jan 13, 2021

scholzj commented Jan 13, 2021

tomasz-zylka-fluke commented Feb 10, 2021

scholzj commented Feb 10, 2021

ajborley commented Dec 15, 2020 •

edited

ajborley Dec 16, 2020 •

edited