Fix topic operator loop for unmanaged topics #10451

padilo · 2024-08-14T13:52:45Z

Type of change

Bugfix

Description

Fixes a bug in the topic operator with unmanaged topics, that keeps updating the custom resources even isn't going to update the Kafka Topic

Closes #10380

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

I do not think this is the right fix for this issue. Also, this issue would likely manifest also in other situations with managed topics.

It seems like the Topic operator keeps updating the .status withotu changing it whihc is wrong. It needs to use the StatusDiff class to detect if something other than the timestamp changed in the status and only if it did, it should update the custom resource status. That should be done regardless whether it is managed or not.

Aside from that, I would expect the topic to not be in the Ready state when it is not managed but be in some special state such as Unmanaged to indicate that the state has been reflected by the Topic Operator. Likely it should not have the topic ID set either as it will not know if it changed or not? But that is not the core of the issue here and can be discussed separately.

scholzj

This looks like the good direction. I left one comment. But the tests also seemed to have failed in the Topic Operator which seems to be related I guess?

scholzj · 2024-08-19T12:58:45Z

topic-operator/src/main/java/io/strimzi/operator/topic/BatchingTopicController.java

-        if (statusChanged(reconcilableTopic.kt(), oldStatus)) {
+
+        StatusDiff statusDiff = new StatusDiff(oldStatus, reconcilableTopic.kt().getStatus());
+        if (!statusDiff.isEmpty() || differentConditions(oldStatus.getConditions(), reconcilableTopic.kt().getStatus().getConditions())) {


The StatusDiff should include the conditions. So why do we need the second part of the condition?

You are right, we can remove it, I misslooked how StatusDiff evaluates the conditions path.

I'll remove it and have a look at the tests as well.

fvaleri

Hi, thanks for working on this. I like the StatusDiff logic and the new Unmanaged condition type, but they require some additional changes.

The setObservedGeneration and setTopicName should be computed before the StatusDiff when reconciliation is enabled. This is why the following integration test is failing:

shouldUpdateTopicInKafkaWhenListConfigChangedInKube

You should also handle the corner case of "new AND unmanaged" KTs been created (the expected status should be "Unmanaged" instead of "Ready"). This is why the following integration tests are failing:

shouldNotCreateTopicInKafkaWhenUnmanagedTopicCreatedInKube
shouldRestoreFinalizerIfRemoved (unmanaged KTs only)

Likely it should not have the topic ID set either as it will not know if it changed or not?

I agree that we should reset the topicId when the KT is paused/unmanaged, and set the topicId when switching back to unpaused/managed. This is actually a separate bug, so I logged #10467 that we should address with a dedicated PR.

fvaleri · 2024-08-19T13:34:29Z

pom.xml

@@ -162,6 +162,7 @@
        <jupiter.version>5.8.2</jupiter.version>
        <strimzi-test-container.version>0.107.0</strimzi-test-container.version>
        <mockserver.version>5.13.2</mockserver.version>
+        <awaitility.version>4.2.2</awaitility.version>


Do we really need an extra dependency when we have TopicOperatroTestUtils.waitUntilCondition, and a number of wait methods in TopicControllerIT that you can use or adapt?

fvaleri · 2024-08-21T10:36:28Z

@padilo I opened #10481 to address the issue with unmanaged topics that are partially reconciled to add finalizers. This should make it easier to complete this bugfix.

padilo · 2024-08-22T17:09:03Z

Tests should be fixed, but I still need to have a look at the test and remove awaitability lib :)

Sorry for the delay on this.

fvaleri · 2024-08-23T10:52:43Z

Sorry for the delay on this.

No problem.

Can you please rebase and fix checkstyle errors?

padilo · 2024-08-25T14:28:39Z

I think this is already done in a different PR, so maybe this should be closed already :)

fvaleri · 2024-08-26T07:44:29Z

I think this is already done in a different PR, so maybe this should be closed already :)

If you are referring to #10481, that addresses a different issue. There is some overlap (introduction of the "Unmanaged" status condition) but there is no fix for the tight reconciliation loop reported by you. The important bit to deliver here is the introduction of StatusDiff.

scholzj

I agree with Federico. This still fixes the main issue.

scholzj · 2024-08-26T08:55:22Z

topic-operator/src/main/java/io/strimzi/operator/topic/BatchingTopicController.java

-        if (statusChanged(reconcilableTopic.kt(), oldStatus)) {
+
+        StatusDiff statusDiff = new StatusDiff(oldStatus, reconcilableTopic.kt().getStatus());
+        if (!statusDiff.isEmpty() || synchronizableAndDifferentGenerations(reconcilableTopic.kt(), oldStatus)) {


I still don't understand why the synchronizableAndDifferentGenerations. What exactly does it check that isn't covered by the StatusDiff? synchronizableAndDifferentGenerations seems to to return true for manage unpaused topics with different generations. But even StatusDiff should be able to handle that.

fvaleri

@padilo I tested your changes after removing synchronizableAndDifferentGenerations and they look good. If you remove that code, then we are good to go. Thanks.

padilo · 2024-08-30T14:32:24Z

done :)

fvaleri · 2024-08-30T14:44:46Z

done :)

There is an unused import to fix.

Signed-off-by: Pablo Díaz <[email protected]>

scholzj · 2024-08-30T17:21:08Z

/azp run regression

azure-pipelines · 2024-08-30T17:21:20Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj

LGTM assuming the system tests pass. Thanks for the PR.

fvaleri

Thanks.

padilo force-pushed the fix-unmanaged-infinite-loop branch 2 times, most recently from a4abdbf to 41330db Compare August 14, 2024 14:38

ppatierno requested review from fvaleri and a team August 14, 2024 14:42

scholzj requested a review from tombentley August 14, 2024 14:53

padilo force-pushed the fix-unmanaged-infinite-loop branch from 41330db to 48f1cfe Compare August 14, 2024 14:54

scholzj requested changes Aug 14, 2024

View reviewed changes

scholzj reviewed Aug 19, 2024

View reviewed changes

fvaleri reviewed Aug 19, 2024

View reviewed changes

fvaleri added this to the 0.43.0 milestone Aug 19, 2024

scholzj modified the milestones: 0.43.0, 0.44.0 Aug 19, 2024

padilo force-pushed the fix-unmanaged-infinite-loop branch 2 times, most recently from 00c30b6 to 6a3e244 Compare August 22, 2024 17:13

padilo force-pushed the fix-unmanaged-infinite-loop branch 5 times, most recently from 23fd983 to 75963bd Compare August 25, 2024 14:20

scholzj reviewed Aug 26, 2024

View reviewed changes

fvaleri mentioned this pull request Aug 27, 2024

Fix topicId status update #10491

Merged

4 tasks

fvaleri reviewed Aug 30, 2024

View reviewed changes

padilo force-pushed the fix-unmanaged-infinite-loop branch from 75963bd to ea813f6 Compare August 30, 2024 14:31

Fix topic operator loop for unmanaged topics

3bcb22a

Signed-off-by: Pablo Díaz <[email protected]>

padilo force-pushed the fix-unmanaged-infinite-loop branch from ea813f6 to 3bcb22a Compare August 30, 2024 15:48

scholzj approved these changes Aug 30, 2024

View reviewed changes

fvaleri approved these changes Aug 31, 2024

View reviewed changes

scholzj merged commit cb45f8d into strimzi:main Aug 31, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix topic operator loop for unmanaged topics #10451

Fix topic operator loop for unmanaged topics #10451

padilo commented Aug 14, 2024 •

edited

Loading

scholzj left a comment

scholzj left a comment

scholzj Aug 19, 2024

padilo Aug 19, 2024

fvaleri left a comment •

edited

Loading

fvaleri Aug 19, 2024

fvaleri commented Aug 21, 2024 •

edited

Loading

padilo commented Aug 22, 2024

fvaleri commented Aug 23, 2024

padilo commented Aug 25, 2024

fvaleri commented Aug 26, 2024

scholzj left a comment

scholzj Aug 26, 2024

fvaleri left a comment

padilo commented Aug 30, 2024

fvaleri commented Aug 30, 2024

scholzj commented Aug 30, 2024

azure-pipelines bot commented Aug 30, 2024

scholzj left a comment

fvaleri left a comment

Fix topic operator loop for unmanaged topics #10451

Fix topic operator loop for unmanaged topics #10451

Conversation

padilo commented Aug 14, 2024 • edited Loading

Type of change

Description

Checklist

scholzj left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

scholzj Aug 19, 2024

Choose a reason for hiding this comment

padilo Aug 19, 2024

Choose a reason for hiding this comment

fvaleri left a comment • edited Loading

Choose a reason for hiding this comment

fvaleri Aug 19, 2024

Choose a reason for hiding this comment

fvaleri commented Aug 21, 2024 • edited Loading

padilo commented Aug 22, 2024

fvaleri commented Aug 23, 2024

padilo commented Aug 25, 2024

fvaleri commented Aug 26, 2024

scholzj left a comment

Choose a reason for hiding this comment

scholzj Aug 26, 2024

Choose a reason for hiding this comment

fvaleri left a comment

Choose a reason for hiding this comment

padilo commented Aug 30, 2024

fvaleri commented Aug 30, 2024

scholzj commented Aug 30, 2024

azure-pipelines bot commented Aug 30, 2024

scholzj left a comment

Choose a reason for hiding this comment

fvaleri left a comment

Choose a reason for hiding this comment

padilo commented Aug 14, 2024 •

edited

Loading

fvaleri left a comment •

edited

Loading

fvaleri commented Aug 21, 2024 •

edited

Loading