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.

Sign up for GitHub

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

Jump to bottom

Add Quotas API to Kafka CR and upgrade Strimzi Quotas Plugin to 0.3.1 #9895

Merged
merged 4 commits into from
Jun 22, 2024
Merged

Add Quotas API to Kafka CR and upgrade Strimzi Quotas Plugin to 0.3.1 #9895

merged 4 commits into from
Jun 22, 2024

Conversation

im-konge
Copy link
Member

@im-konge im-konge commented Mar 29, 2024 •
edited
Loading

Type of change

  • Enhancement

Description

This PR adds Quotas API to Kafka CR based on Quotas management proposal and updates Strimzi Quotas Plugin version to 0.3.1 (and also it reflects changes based on Cluster Wide Volume Usage Quota Management).

It also changes the QuotasST, which need to reflect changes done in Strimzi Quotas Plugin 0.3.1.

Closes #9097, #9859

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

@im-konge im-konge added this to the 0.41.0 milestone Mar 29, 2024
@im-konge im-konge linked an issue Mar 29, 2024 that may be closed by this pull request
[Enhancement] Integrate the Quotas 0.3.0 plugin into the Kafka CR #9859
Closed
scholzj
scholzj reviewed Mar 29, 2024
View reviewed 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.

You should also go through the PR checklist to see what to do as part of the Pr such as update the CHANGELOG. You should also at least remove the old invalid docs from the documentation.

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPlugin.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginKafka.java Outdated Show resolved Hide resolved
...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java Outdated Show resolved Hide resolved
...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java Outdated Show resolved Hide resolved
...erator/src/main/java/io/strimzi/operator/cluster/model/quotas/QuotasPluginConfiguration.java Outdated Show resolved Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/quotas/QuotasPluginUtils.java Outdated Show resolved Hide resolved
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java Outdated Show resolved Hide resolved
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java Outdated Show resolved Hide resolved
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java Outdated Show resolved Hide resolved
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java Outdated Show resolved Hide resolved
fvaleri
fvaleri reviewed Apr 2, 2024
View reviewed changes
Copy link
Contributor

@fvaleri fvaleri left a comment

Choose a reason for hiding this comment

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

Hi @im-konge, thanks for working on this. Left some comments.

Once this PR is merged, I think we need to also remove this warning.

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPlugin.java Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginKafka.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java Outdated Show resolved Hide resolved
...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java Show resolved Hide resolved
...tor/src/test/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilderTest.java Outdated Show resolved Hide resolved
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java Outdated Show resolved Hide resolved
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java Outdated Show resolved Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java Outdated Show resolved Hide resolved
@im-konge im-konge self-assigned this Apr 4, 2024
PaulRMellor
PaulRMellor approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@PaulRMellor PaulRMellor left a comment

Choose a reason for hiding this comment

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

Looking good! I left a couple of suggestions.
As discussed, there will be related doc that needs updating, but should probably be left to a separate pr.

CHANGELOG.md Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/KafkaClusterSpec.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginKafka.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java Outdated Show resolved Hide resolved
@im-konge
Copy link
Member Author

im-konge commented Apr 5, 2024 •
edited
Loading

@PaulRMellor thanks for reviewing the descriptions, I updated the section mentioning Kafka Static Quota plugin, to match the 0.3.0 version + the API changes. Could you please review that as well? Thanks!

(I can create a follow-up docs PR that will mention the default user quotas, I just wanted to update in this one the parts that will not be correct after this change)

fvaleri
fvaleri approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@fvaleri fvaleri left a comment

Choose a reason for hiding this comment

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

LGTM. Great work!

CHANGELOG.md Outdated Show resolved Hide resolved
PaulRMellor
PaulRMellor approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@PaulRMellor PaulRMellor left a comment

Choose a reason for hiding this comment

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

additional changes look good. Just a couple of suggestions

documentation/modules/managing/proc-setting-broker-limits.adoc Outdated Show resolved Hide resolved
documentation/modules/managing/proc-setting-broker-limits.adoc Outdated Show resolved Hide resolved
scholzj
scholzj requested changes Apr 8, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPlugin.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java Outdated Show resolved Hide resolved
...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java Outdated Show resolved Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java Outdated Show resolved Hide resolved
...-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/QuotasPluginUtils.java Outdated Show resolved Hide resolved
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java Outdated Show resolved Hide resolved
...-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/QuotasPluginUtils.java Outdated Show resolved Hide resolved
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java Outdated Show resolved Hide resolved
cluster-operator/src/test/java/io/strimzi/operator/cluster/ResourceUtils.java Outdated Show resolved Hide resolved
@im-konge im-konge marked this pull request as draft April 9, 2024 21:11
@scholzj scholzj modified the milestones: 0.41.0, 0.42.0 May 7, 2024
@im-konge im-konge requested a review from scholzj May 31, 2024 18:11
@im-konge im-konge marked this pull request as ready for review May 31, 2024 18:11
@im-konge
Copy link
Member Author

@scholzj @ppatierno the PR is updated with the new version of Strimzi Quotas plugin (0.3.1), the comments should be now resolved and I updated also the ST for the quotas plugin, to reflect the changes from this PR.

When you have time, could you please have another look? Thanks :)

@im-konge im-konge changed the title Add Quotas API to Kafka CR and upgrade Strimzi Quotas Plugin to 0.3.0 Add Quotas API to Kafka CR and upgrade Strimzi Quotas Plugin to 0.3.1 May 31, 2024
scholzj
scholzj reviewed Jun 2, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/KafkaClusterSpec.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
cluster-operator/src/test/java/io/strimzi/operator/cluster/ResourceUtils.java Outdated Show resolved Hide resolved
...tor/src/test/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilderTest.java Show resolved Hide resolved
scholzj
scholzj reviewed Jun 12, 2024
View reviewed changes
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
...c/test/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManagerTest.java Outdated Show resolved Hide resolved
...c/test/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManagerTest.java Outdated Show resolved Hide resolved
...c/test/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManagerTest.java Outdated Show resolved Hide resolved
...c/test/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManagerTest.java Show resolved Hide resolved
...va/io/strimzi/operator/cluster/operator/resource/events/KubernetesRestartEventsMockTest.java Outdated Show resolved Hide resolved
@im-konge im-konge requested a review from scholzj June 18, 2024 16:39
scholzj
scholzj reviewed Jun 19, 2024
View reviewed 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.

Some more nits.

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Show resolved Hide resolved
...c/test/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManagerTest.java Outdated Show resolved Hide resolved
...c/test/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManagerTest.java Outdated Show resolved Hide resolved
PaulRMellor
PaulRMellor approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@PaulRMellor PaulRMellor left a comment

Choose a reason for hiding this comment

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

Thanks @im-konge -- I re-reviewed the docs and left a few suggestions, including one about use of users and clients. We may want to be consistent there.

documentation/modules/appendix_crds.adoc Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java Outdated Show resolved Hide resolved
documentation/modules/appendix_crds.adoc Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java Outdated Show resolved Hide resolved
documentation/modules/managing/proc-setting-broker-limits.adoc Outdated Show resolved Hide resolved
@im-konge
quotas API in Kafka CR
4e69ada 
Signed-off-by: Lukas Kral <[email protected]>

fixups and tests

Signed-off-by: Lukas Kral <[email protected]>

few more changes, comments, changes to the QuotasST

Signed-off-by: Lukas Kral <[email protected]>

add one more check and OneOf to the Strimzi quota plugin fields

Signed-off-by: Lukas Kral <[email protected]>

changes after rebase

Signed-off-by: Lukas Kral <[email protected]>

comments

Signed-off-by: Lukas Kral <[email protected]>

crd install

Signed-off-by: Lukas Kral <[email protected]>

fix spotbugs

Signed-off-by: Lukas Kral <[email protected]>

fix tests

Signed-off-by: Lukas Kral <[email protected]>

update documentation, fix more tests, and apply comments from Paul

Signed-off-by: Lukas Kral <[email protected]>

comments

Signed-off-by: Lukas Kral <[email protected]>

excluded principals

Signed-off-by: Lukas Kral <[email protected]>

bump quotas plugin to 0.3.1 and do changes to code

Signed-off-by: Lukas Kral <[email protected]>

fix UT and update ST for the quotas plugin

Signed-off-by: Lukas Kral <[email protected]>

Jakub's and Paolo's comments

Signed-off-by: Lukas Kral <[email protected]>

remove blank lines in the config builder

Signed-off-by: Lukas Kral <[email protected]>

handle situation when we fail to create admin client

Signed-off-by: Lukas Kral <[email protected]>

comments from Jakub

Signed-off-by: Lukas Kral <[email protected]>

Jakub's comments

Signed-off-by: Lukas Kral <[email protected]>

Jakub's and Paul's comments

Signed-off-by: Lukas Kral <[email protected]>
@im-konge im-konge requested a review from scholzj June 20, 2024 10:39
@im-konge im-konge requested a review from scholzj June 20, 2024 23:08
@scholzj
Copy link
Member

scholzj commented Jun 21, 2024

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scholzj
scholzj approved these changes Jun 21, 2024
View reviewed 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.

LGTM. Thanks.

Frawless
Frawless approved these changes Jun 21, 2024
View reviewed changes
Copy link
Member

@Frawless Frawless left a comment

Choose a reason for hiding this comment

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

I double checked ST part and LGTM.

@im-konge
Copy link
Member Author

@strimzi-ci run tests --cluster-type=ocp --cluster-version=4.15 --install-type=bundle --profile=all --groups=quotasplugin --build-images

@strimzi-ci
Copy link

▶️ Build started - check Jenkins for more info. ▶️

@scholzj
Copy link
Member

scholzj commented Jun 21, 2024

/azp run kraft-regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

ppatierno
ppatierno approved these changes Jun 21, 2024
View reviewed changes
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. Just a couple of nits.

CHANGELOG.md Outdated Show resolved Hide resolved
...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java Outdated Show resolved Hide resolved
@strimzi-ci
Copy link

▶️ Build started - check Jenkins for more info. ▶️

@strimzi-ci
Copy link

✔️ Test Summary ✔️

TEST_PROFILE: all
GROUPS: quotasplugin
TEST_CASE:
TOTAL: 1
PASS: 1
FAIL: 0
SKIP: 0
BUILD_NUMBER: 2
OCP_VERSION: 4.15
BUILD_IMAGES: true
FIPS_ENABLED: false
PARALLEL_COUNT: 5
EXCLUDED_GROUPS: loadbalancer,nodeport,olm

@im-konge
Paolo's comments
4e4fc13 
Signed-off-by: Lukas Kral <[email protected]>
@im-konge
Copy link
Member Author

/azp run upgrade

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge im-konge merged commit e54c310 into strimzi:main Jun 22, 2024
17 checks passed
@im-konge im-konge deleted the quotas-api-and-030 branch June 22, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scholzj scholzj scholzj approved these changes

@ppatierno ppatierno ppatierno approved these changes

@kyguy kyguy kyguy left review comments

@PaulRMellor PaulRMellor PaulRMellor approved these changes

@Frawless Frawless Frawless approved these changes

@fvaleri fvaleri fvaleri approved these changes

Assignees

@im-konge im-konge

Labels
needs review
Projects
None yet
Milestone
0.42.0
8 participants
@im-konge @scholzj @strimzi-ci @ppatierno @kyguy @Frawless @fvaleri @PaulRMellor