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

im-konge · 2024-03-29T01:46:08Z

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

scholzj

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

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginKafka.java

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java

...erator/src/main/java/io/strimzi/operator/cluster/model/quotas/QuotasPluginConfiguration.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/quotas/QuotasPluginUtils.java

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java

fvaleri

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

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginKafka.java

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java

...tor/src/test/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilderTest.java

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

PaulRMellor

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

api/src/main/java/io/strimzi/api/kafka/model/kafka/KafkaClusterSpec.java

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginKafka.java

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java

im-konge · 2024-04-05T12:40:28Z

@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

LGTM. Great work!

CHANGELOG.md

PaulRMellor

additional changes look good. Just a couple of suggestions

documentation/modules/managing/proc-setting-broker-limits.adoc

CHANGELOG.md

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPlugin.java

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

...-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/QuotasPluginUtils.java

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

...-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/QuotasPluginUtils.java

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

cluster-operator/src/test/java/io/strimzi/operator/cluster/ResourceUtils.java

im-konge · 2024-05-31T18:12:17Z

@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 :)

scholzj · 2024-06-02T20:52:55Z

CHANGELOG.md

+### Changes, deprecations and removals
+* Usage of Strimzi Quotas plugin version 0.2.0 is not supported and from Strimzi 0.42.0, the plugin is configured
+  in `.spec.kafka.quotas` section - the configuration of the plugin inside `.spec.kafka.config` should be removed.


You should have space between the title and the list

I think this is a bit misleading as at the same time the plugin changed significantly. So I would warn against that rather than just say that the configuration moved.

I think that it's useful to mention that the plugin is not configured in the .spec.kafka.config, but I agree that the plugin changed significantly. So maybe something like:

Usage of Strimzi Quotas plugin version 0.2.0 is not supported, the plugin was updated to 0.3.1 and changed significantly. Additionally, from Strimzi 0.42.0 the plugin should be configured in `.spec.kafka.quotas` section - the configuration of the plugin inside `.spec.kafka.config` should be removed.

?

CHANGELOG.md

api/src/main/java/io/strimzi/api/kafka/model/kafka/KafkaClusterSpec.java

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java

scholzj · 2024-06-02T20:58:00Z

api/src/main/java/io/strimzi/api/kafka/model/kafka/quotas/QuotasPluginStrimzi.java

+    @Description("A quota on the maximum bytes per-second that each client group can publish to a broker before " +
+        "the clients in the group are throttled. Defined on a per-broker basis.")


Is this the case? Isn't it overall throughput per broker? Per cluster?

Yeah this description is wrong, it should be byte-rate for all of the clients per cluster (I think).
So something like

A total quota limiting byte-rate for clients, independent on their number, that produce to a cluster, defined on a per-cluster basis. The limitation depends on the clients produce speed - in case that clients are producing messages as fast as possible, the limitation will be equal for all. In other case when one is producing at specific rate, the other one is limited by the remainder of the difference between the specified byte-rate and the speed of the first.

but I guess it's too long for a description 😄

Same applies for the fetch quota

WDYT @scholzj ?

...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java

scholzj · 2024-06-02T21:13:10Z

...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java

+            Map<String, Double> currentQuotas = kafkaAdmin.describeClientQuotas(ClientQuotaFilter.containsOnly(Collections.singletonList(ClientQuotaFilterComponent.ofDefaultEntity(ClientQuotaEntity.USER))))
+                .entities()
+                .get()
+                .get(DEFAULT_USER_ENTITY);


This sounds like blocking operation. You need to handle this properly as async code.

cluster-operator/src/test/java/io/strimzi/operator/cluster/ResourceUtils.java

...tor/src/test/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilderTest.java

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

...r/src/main/java/io/strimzi/operator/cluster/operator/assembly/DefaultKafkaQuotasManager.java

ppatierno · 2024-06-04T10:59:23Z

...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java

+     *
+     * @return  Future which completes when the default quotas are configured
+     */
+    protected Future<Void> defaultKafkaQuotas() {


so this method is calling something related to the specific Kafka quotas plugin (by using the DefaultKafkaQuotasManager) but then internally it's checking if the quota plugin is really an instance of the QuotasPluginKafka. I would move the check here and not inside so avoiding the call at all? It would make clear in the reconciliation code that you are going to configure default user quotas but only if the Kafka quotas plugin is used, or?

Don't you need to delete the quotas if they are not set in the Kafka CR?

Yes as Jakub mentioned, it is there for un-setting the default quotas.
That's for the scenario when we had configuration for the QuotasPluginKafka in previous spec, the default quotas were set in Kafka, and now we want to move to QuotasPluginStrimzi or null. It's not about checking if the actual spec is QuotasPluginKafka type or not.

My mental model was about expecting a call to a quotas manager which is able to configure the right quotas plugin (if it's Kafka or Strimzi) and not a dedicated call in the reconciler using a dedicated class like the DefaultKafkaQuotasManager which is just about the Kafka plugin (so configuring default quotas or removing them).

Strimzi quotas plugin is configured in the broker configuration and this is named defaultKafkaQuotas, so it should cover just the Kafka quotas plugin.
From the code I saw that it's both configured on different places and I think it would not be trivial to have something like configureQuotas() which would then, based on plugin type, configure the broker configuration for Strimzi plugin or create/alter the default quotas in Kafka using Kafka Admin client.

But maybe I'm wrong

Signed-off-by: Lukas Kral <lukywill16@gmail.com> fixups and tests Signed-off-by: Lukas Kral <lukywill16@gmail.com> few more changes, comments, changes to the QuotasST Signed-off-by: Lukas Kral <lukywill16@gmail.com> add one more check and OneOf to the Strimzi quota plugin fields Signed-off-by: Lukas Kral <lukywill16@gmail.com> changes after rebase Signed-off-by: Lukas Kral <lukywill16@gmail.com> comments Signed-off-by: Lukas Kral <lukywill16@gmail.com> crd install Signed-off-by: Lukas Kral <lukywill16@gmail.com> fix spotbugs Signed-off-by: Lukas Kral <lukywill16@gmail.com> fix tests Signed-off-by: Lukas Kral <lukywill16@gmail.com> update documentation, fix more tests, and apply comments from Paul Signed-off-by: Lukas Kral <lukywill16@gmail.com> comments Signed-off-by: Lukas Kral <lukywill16@gmail.com> excluded principals Signed-off-by: Lukas Kral <lukywill16@gmail.com> bump quotas plugin to 0.3.1 and do changes to code Signed-off-by: Lukas Kral <lukywill16@gmail.com> fix UT and update ST for the quotas plugin Signed-off-by: Lukas Kral <lukywill16@gmail.com> Jakub's and Paolo's comments Signed-off-by: Lukas Kral <lukywill16@gmail.com>

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

im-konge · 2024-06-06T09:17:56Z

@scholzj @ppatierno I resolved the comments -> some of them I kept open just because I'm not sure if the wording or my solution is 100% correct. Once you have time, could you please have another look?

im-konge added this to the 0.41.0 milestone Mar 29, 2024

im-konge added the needs review label Mar 29, 2024

im-konge requested review from scholzj and ppatierno March 29, 2024 01:47

im-konge force-pushed the quotas-api-and-030 branch from 692d830 to 97aa69f Compare March 29, 2024 01:51

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

Open

im-konge force-pushed the quotas-api-and-030 branch from 97aa69f to 3b5d419 Compare March 29, 2024 09:38

scholzj reviewed Mar 29, 2024

View reviewed changes

ppatierno reviewed Apr 2, 2024

View reviewed changes

fvaleri reviewed Apr 2, 2024

View reviewed changes

im-konge self-assigned this Apr 4, 2024

PaulRMellor approved these changes Apr 5, 2024

View reviewed changes

im-konge requested review from scholzj, ppatierno, fvaleri and PaulRMellor April 5, 2024 12:40

fvaleri approved these changes Apr 5, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

PaulRMellor approved these changes Apr 8, 2024

View reviewed changes

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 requested changes Apr 8, 2024

View reviewed changes

im-konge marked this pull request as draft April 9, 2024 21:11

scholzj modified the milestones: 0.41.0, 0.42.0 May 7, 2024

im-konge force-pushed the quotas-api-and-030 branch from e5ab858 to acac770 Compare May 25, 2024 21:57

im-konge requested a review from scholzj May 31, 2024 18:11

im-konge marked this pull request as ready for review May 31, 2024 18:11

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 reviewed Jun 2, 2024

View reviewed changes

ppatierno reviewed Jun 4, 2024

View reviewed changes

im-konge force-pushed the quotas-api-and-030 branch from 8e26e4d to dfc35e6 Compare June 5, 2024 19:13

im-konge added 2 commits June 5, 2024 21:15

remove blank lines in the config builder

c2fa26a

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

handle situation when we fail to create admin client

27c1ec4

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

im-konge requested review from scholzj and ppatierno June 6, 2024 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

im-konge commented Mar 29, 2024 •

edited

scholzj left a comment

fvaleri left a comment

PaulRMellor left a comment

im-konge commented Apr 5, 2024 •

edited

fvaleri left a comment

PaulRMellor left a comment

im-konge commented May 31, 2024

scholzj Jun 2, 2024

im-konge Jun 4, 2024

scholzj Jun 2, 2024

im-konge Jun 4, 2024

scholzj Jun 2, 2024

ppatierno Jun 4, 2024

scholzj Jun 4, 2024

im-konge Jun 5, 2024 •

edited

ppatierno Jun 5, 2024

im-konge Jun 5, 2024

im-konge commented Jun 6, 2024

		@Description("A quota on the maximum bytes per-second that each client group can publish to a broker before " +
		"the clients in the group are throttled. Defined on a per-broker basis.")

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

Are you sure you want to change the base?

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

Conversation

im-konge commented Mar 29, 2024 • edited

Type of change

Description

Checklist

scholzj left a comment

Choose a reason for hiding this comment

fvaleri left a comment

Choose a reason for hiding this comment

PaulRMellor left a comment

Choose a reason for hiding this comment

im-konge commented Apr 5, 2024 • edited

fvaleri left a comment

Choose a reason for hiding this comment

PaulRMellor left a comment

Choose a reason for hiding this comment

im-konge commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-konge Jun 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-konge commented Jun 6, 2024

im-konge commented Mar 29, 2024 •

edited

im-konge commented Apr 5, 2024 •

edited

im-konge Jun 5, 2024 •

edited