Skip to content
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

Promote UnidirectionalTopicOperator feature gate to GA #9840

Merged
merged 12 commits into from
Mar 21, 2024

Conversation

fvaleri
Copy link
Contributor

@fvaleri fvaleri commented Mar 15, 2024

This change promotes the UnidirectionalTopicOperator feature gate to GA. It removes the Bidirectional Topic Operator code and the UnidirectionalTopicOperator feature gate. It also updates all related tests and documentation.

Additional notes:

  • Azure pipeline changes needs to be verified, as I don't have access to that platform.
  • The KRaftKafkaUpgradeDowngradeST fails because STRIMZI_USE_NODE_POOLS_IN_TESTS is set to false by default (fixed separately).
  • There is a backlog of STs marked with UTONotSupported that should be re-enabled (fixed separately).

@fvaleri fvaleri requested review from PaulRMellor, scholzj and tombentley and removed request for PaulRMellor and scholzj March 15, 2024 12:49
@fvaleri fvaleri added this to the 0.41.0 milestone Mar 15, 2024
Copy link
Member

@im-konge im-konge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left few comments. Thanks a lot for the changes :)

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the Pr, this looks good. I left some comments ... also some other things I think are missing:

  • In docker-images, I think we do not need to install the stunnel anymore and the scripts for running / configuring it.
  • In the docs, I left some comments. But in general, the docs are for Strimzi 0.41 and all mentiones of bidirectional TO can be removed apart form the feautre gates section that lists the (now unused) UTO feature gate and what it was doing. There is no need to explain that before 0.41 there was some other topic operator.

@fvaleri
Copy link
Contributor Author

fvaleri commented Mar 15, 2024

@scholzj I guess these warnings should not appear when the deprecated properties are not used right?

status:
  clusterId: vPnoXMxwTyay53EMBJ3chQ
  conditions:
  - lastTransitionTime: "2024-03-15T17:11:41.886032728Z"
    status: "True"
    type: Ready
  - lastTransitionTime: "2024-03-15T17:10:09.681532310Z"
    message: In resource Kafka(test/my-cluster) in API version kafka.strimzi.io/v1beta2
      the zookeeperSessionTimeoutSeconds property at path spec.entityOperator.topicOperator.zookeeperSessionTimeoutSeconds
      has been deprecated. This property has been deprecated because ZooKeeper is
      not used anymore by the Topic Operator.
    reason: DeprecatedFields
    status: "True"
    type: Warning
  - lastTransitionTime: "2024-03-15T17:10:09.681532310Z"
    message: In resource Kafka(test/my-cluster) in API version kafka.strimzi.io/v1beta2
      the topicMetadataMaxAttempts property at path spec.entityOperator.topicOperator.topicMetadataMaxAttempts
      has been deprecated. This property has been deprecated because ZooKeeper is
      not used anymore by the Topic Operator.
    reason: DeprecatedFields
    status: "True"
    type: Warning
  kafkaMetadataState: ZooKeeper
  kafkaVersion: 3.7.0
...

@scholzj
Copy link
Member

scholzj commented Mar 15, 2024

@fvaleri Are those int fields? MAybe you need to change them to Integer so that they default to null?

@fvaleri
Copy link
Contributor Author

fvaleri commented Mar 16, 2024

@fvaleri Are those int fields? MAybe you need to change them to Integer so that they default to null?

Not just that, we were also setting default values for these fields. Now it looks good. Please, run regression suites when you have time. Thanks.

@see-quick
Copy link
Member

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Mar 16, 2024

/azp run feature-gates-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more nits. You should also:

  • Update the Helm Chart in packaging to temove the TLS sidecar image
  • Render the installation YAML from it

@scholzj
Copy link
Member

scholzj commented Mar 18, 2024

/azp run kraft-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Mar 18, 2024

/azp run upgrade

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall lgtm. I left a few minor comments.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Federico Valeri <[email protected]>
@scholzj
Copy link
Member

scholzj commented Mar 19, 2024

/azp run kraft-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Mar 19, 2024

/azp run upgrade

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Mar 20, 2024

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Mar 20, 2024

/azp run feature-gates-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Mar 20, 2024

The KafkaST#testRemoveComponentsFromEntityOperator in kraft-regression pipeline is working fine on my end, so I guess it is a bit flaky.

@fvaleri I restarted it. But KafkaST.testRemoveComponentsFromEntityOperator keeps failing and seems to be failing only on your PR.

Copy link
Member

@see-quick see-quick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming tests pass. 👍

Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Copy link
Contributor Author

fvaleri commented Mar 20, 2024

@fvaleri I restarted it. But KafkaST.testRemoveComponentsFromEntityOperator keeps failing and seems to be failing only on your PR.

Ok, found the issue. With the last commit KafkaST.testRemoveComponentsFromEntityOperator should work with both STRIMZI_USE_KRAFT_IN_TESTS equal true (test currently failing) and false.

@scholzj
Copy link
Member

scholzj commented Mar 20, 2024

/azp run kraft-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Mar 20, 2024

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Mar 20, 2024

/azp run feature-gates-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fvaleri fvaleri requested a review from scholzj March 21, 2024 11:51
@scholzj scholzj merged commit 05d0c94 into strimzi:main Mar 21, 2024
37 checks passed
@fvaleri fvaleri deleted the promote-uto-ga branch March 21, 2024 13:32
steffen-karlsson pushed a commit to steffen-karlsson/strimzi-kafka-operator that referenced this pull request Mar 21, 2024
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Lukas Kral <[email protected]>
Co-authored-by: Lukas Kral <[email protected]>
Signed-off-by: Steffen Karlsson <[email protected]>
steffen-karlsson pushed a commit to steffen-karlsson/strimzi-kafka-operator that referenced this pull request Mar 26, 2024
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Lukas Kral <[email protected]>
Co-authored-by: Lukas Kral <[email protected]>
Signed-off-by: Steffen Karlsson <[email protected]>
steffen-karlsson pushed a commit to steffen-karlsson/strimzi-kafka-operator that referenced this pull request Apr 2, 2024
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Lukas Kral <[email protected]>
Co-authored-by: Lukas Kral <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants