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

[system test] Auto-rebalance feature test case #10666

Merged
merged 10 commits into from
Oct 22, 2024

Conversation

see-quick
Copy link
Member

@see-quick see-quick commented Oct 2, 2024

Type of change

  • Enhancement / new feature
  • Documentation

Description

This PR adds a system test to cover the basic functionality of the auto-balance feature.

Checklist

  • Write tests
  • Make sure all tests pass

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.

Few comments, thanks :)

@see-quick see-quick added this to the 0.44.0 milestone Oct 3, 2024
@see-quick
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Step(value = "Check that topic replicas are moved to the new brokers.", expected = "Topic replicas are distributed onto the newly added brokers."),
@Step(value = "Scale Kafka down to the original number of brokers.", expected = "Kafka brokers are scaled down, and Cruise Control initiates rebalancing in REMOVE_BROKERS mode."),
@Step(value = "Verify that Kafka auto-rebalance status transitions to RebalanceOnScaleDown and then back to Idle.", expected = "Auto-rebalance status moves to RebalanceOnScaleDown during scaling down and returns to Idle after rebalancing completes."),
@Step(value = "Confirm that the cluster is stable after scaling operations.", expected = "Cluster returns to a stable state with initial number of brokers and Cruise Control completed the rebalancing.")
Copy link
Member

Choose a reason for hiding this comment

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

@see-quick see the comments I made on the description, they apply here as well.

@see-quick
Copy link
Member Author

see-quick commented Oct 4, 2024

Converting to the draft I have faced issue [1] multiple times (3/5 times race conditions happen).

[1] - #10631

@see-quick
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ppatierno
Copy link
Member

@see-quick build and regression are green, I guess we can move this back as ready to be reviewed.

@see-quick see-quick marked this pull request as ready for review October 17, 2024 18:50
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.

LGTM

@ppatierno
Copy link
Member

@im-konge can you have another pass on this please?

Signed-off-by: see-quick <[email protected]>
@see-quick
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

Just two small nits in the logging stuff. Thanks

Signed-off-by: see-quick <[email protected]>
@see-quick
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

resourceManager.createResourceWithoutWait(
KafkaRebalanceTemplates.kafkaRebalance(testStorage.getNamespaceName(), testStorage.getClusterName())
.editMetadata()
.addToAnnotations(Annotations.ANNO_STRIMZI_IO_REBALANCE, "template")
Copy link
Member

Choose a reason for hiding this comment

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

So, I guess this needs to be fixed to the new format?

Copy link
Member

@ppatierno ppatierno Oct 21, 2024

Choose a reason for hiding this comment

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

@see-quick it's weird it's still this way. We had a chat to update it to the new strimzi.io/rebalance-template: true and it was also done, or? I was pretty sure about that, for this reason approved at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but we speak offline with Paolo. He will rebase :)

.build(),
KafkaRebalanceTemplates.kafkaRebalance(testStorage.getNamespaceName(), testStorage.getClusterName())
.editMetadata()
.addToAnnotations(Annotations.ANNO_STRIMZI_IO_REBALANCE, "template")
Copy link
Member

Choose a reason for hiding this comment

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

So, I guess this needs to be fixed to the new format?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, ditto as above.

Comment on lines +701 to +705
.withMode(KafkaRebalanceMode.ADD_BROKERS)
.withNewTemplate(scaleUpKafkaRebalanceTemplateName)
.build(),
new KafkaAutoRebalanceConfigurationBuilder()
.withMode(KafkaRebalanceMode.REMOVE_BROKERS)
Copy link
Member

Choose a reason for hiding this comment

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

This will be likely broken by #10744?

Copy link
Member

@ppatierno ppatierno Oct 21, 2024

Choose a reason for hiding this comment

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

Yes it will be. At this point I think it's better waiting for #10744 to be merged (regression running) and then rebasing this one or I have to fix it in my PR when this is merged.

Signed-off-by: see-quick <[email protected]>
@see-quick see-quick merged commit ff8521a into strimzi:main Oct 22, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants