Don't fail reconciliation when Manual Rolling Update fails

[Staniel]

Type of change

Select the type of your PR

• Enhancement / new feature

Description

Implements https://github.com/strimzi/proposals/blob/main/070-dont-fail-reconciliation-in-manual-rolling-update.md

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

[scholzj]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[scholzj]

Thanks for the PR. The code looks good. I left only some smaller comments. Would it be worth to also add some unit tests for Connect / Mirror Maker 2? They have already some maunal rolling update tests in KafkaMirrorMaker2AssemblyOperatorPodSetTest.java and KafkaConnectAssemblyOperatorPodSetTest.java

[scholzj]

One more thing ... could you also enable the feature gate in these two files:

• https://github.com/strimzi/strimzi-kafka-operator/blob/main/.azure/templates/jobs/system-tests/feature_gates_regression_jobs.yaml
• https://github.com/strimzi/strimzi-kafka-operator/blob/main/.azure/templates/jobs/system-tests/feature_gates_regression_namespace_rbac_jobs.yaml

in the strimzi_feature_gates parameter => that would allow us to run the tests with the feature gate enabled and see f that works as well. Thanks.

[Staniel]

Thanks for the PR. The code looks good. I left only some smaller comments. Would it be worth to also add some unit tests for Connect / Mirror Maker 2? They have already some maunal rolling update tests in KafkaMirrorMaker2AssemblyOperatorPodSetTest.java and KafkaConnectAssemblyOperatorPodSetTest.java

I looked at it but didn't find an easy way to simulate a manual roll failure, since it was not using mock operator. Any suggestion?

[scholzj]

I looked at it but didn't find an easy way to simulate a manual roll failure, since it was not using mock operator. Any suggestion?

Maybe you have to write one. You could also try to trigger the error through some mocked method. Maybe if you throw here an error instead of success it would cause an error you can use?

when(mockPodOps.deleteAsync(any(), eq(NAMESPACE), startsWith(COMPONENT_NAME), eq(false))).thenReturn(Future.succeededFuture());

Let's look sta it next week. Otherwise it looks good.

[scholzj]

/azp run feature-gates-regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[scholzj]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[katheris]

LGTM, just had one comment about the tests.

[katheris]

These are effectively parametrized tests, so it might be worth using the actual annotation, it would save having lots of tests with the same name but different numbers at the end. https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests-sources-MethodSource

[scholzj]

@Staniel I added the unit tests so that we can move forward with this and unblock some other work that would conflict with this.

@ppatierno Could you please review this?

[scholzj]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[scholzj]

/azp run feature-gates-regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ppatierno]

LGTM. Thanks!