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

MGDSTRM-9181: Have kas ingress controller utilise the openshift-router's dynamic config manager. #850

Merged
merged 13 commits into from
Jan 17, 2023

Conversation

k-wall
Copy link
Contributor

@k-wall k-wall commented Jan 13, 2023

Enables the (unsupported) dynamic config manager option on openshift-ingress. This is done as otherwise, haproxy reconfigurations (which occur every time new kafka instance is provisioned or deprovisioned) cause all kafka connections to all instances to be dropped. This is too disruptive for applications.

Work based on ideas from @shawkins and @showuon .

@k-wall k-wall requested a review from shawkins January 13, 2023 15:42
@github-actions github-actions bot added the operator changes related to operator label Jan 13, 2023
@k-wall k-wall changed the title Mgdstrm 9181 MGDSTRM-9181: Have kas ingress controller utilise the openshift-route's dynamic config manager. Jan 13, 2023
@k-wall k-wall changed the title MGDSTRM-9181: Have kas ingress controller utilise the openshift-route's dynamic config manager. MGDSTRM-9181: Have kas ingress controller utilise the openshift-router's dynamic config manager. Jan 13, 2023
@k-wall k-wall requested a review from showuon January 13, 2023 16:31
Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@showuon showuon left a comment

Choose a reason for hiding this comment

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

@k-wall ,Thanks for this exciting PR!!! Left some comments.

operator/src/main/resources/application.properties Outdated Show resolved Hide resolved
operator/src/main/resources/application.properties Outdated Show resolved Hide resolved
Copy link
Contributor

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the sync changes related to sync label Jan 16, 2023
@k-wall
Copy link
Contributor Author

k-wall commented Jan 16, 2023

It's quite difficult to give the responsibility to create the blueprints to fleetshard. It is the admin route that the problem. We have the possibility that developer and standard profiles can have different rate limits, and to support kas-installer, we support edge/passthrough and http/https. These requirements made me push the responsibility for creating the blueprint to KafkaCluster and AdminServer.

I've used a pattern where the operator will create the "required" blueprints as the user requests the first kafka. So expect to see an initial haproxy reload when the first kafka instance is created. You should see no more after that.

I don't like the fact that blueprints will accumulate if we ever change the annotations but haven't got a reasonable way to identify stale ones.

Copy link
Contributor

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

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

I have a few style "preferences" but I'd rather see this shipped to prod than polished.

Copy link
Contributor

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

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

Running locally I'm seeing

023-01-17 12:44:49,899 DEBUG [org.bf2.ope.man.StrimziBundleManager] (QuarkusQuartzScheduler_Worker-12)  Strimzi bundle Subscription periodic check
2023-01-17 12:44:50,098 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:44:50,743 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:44:51,379 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:44:51,584 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:44:52,007 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:44:52,224 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:44:52,457 INFO  [org.bf2.ope.man.IngressControllerManager] (QuarkusQuartzScheduler_Worker-9)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:44:52,647 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:44:52,862 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:44:53,061 INFO  [org.bf2.ope.man.IngressControllerManager] (QuarkusQuartzScheduler_Worker-9)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:44:53,268 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:44:54,771 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:44:55,420 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:44:56,051 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:44:56,261 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:44:56,887 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:44:57,101 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:44:57,735 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:44:57,954 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:44:58,571 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:44:58,781 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:44:59,626 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:45:00,464 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:45:01,312 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:45:02,148 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:45:02,778 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:45:02,990 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:45:03,631 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:45:03,835 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:45:04,465 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:45:04,671 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:45:05,299 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:45:06,071 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:45:06,823 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:45:07,663 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:45:08,503 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:45:09,134 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:45:09,344 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:45:10,001 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:45:10,201 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:45:10,827 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:45:11,033 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:45:11,662 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:45:11,872 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:45:12,716 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:45:13,563 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:45:14,411 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:45:15,234 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:45:15,865 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:45:16,076 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:45:16,713 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:45:16,923 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a
2023-01-17 12:45:17,555 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:45:17,759 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1c
2023-01-17 12:45:18,391 INFO  [org.bf2.ope.man.IngressControllerManager] (OkHttp https://api.sbarker-osd-410.felr.s1.devshift.org:6443/...)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:45:18,597 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas
2023-01-17 12:45:19,449 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1b
2023-01-17 12:45:20,278 INFO  [org.bf2.ope.man.IngressControllerManager] (CachedSingleThreadScheduler-1415400893-pool-13-thread-1)  Updating the resource limits/container command for Deployment openshift-ingress/router-kas-us-east-1a

Almost continuously which seems suspicious.

@SamBarker
Copy link
Contributor

SamBarker commented Jan 17, 2023

@showuon and I have verified the fix in dev (using OSD 4.10.15).

The continuous update loop mentioned above was because I forgot to stop the on cluster version of fleetshard when spooling up the local copy using quarkus:dev

…llerManager.java

Co-authored-by: Sam Barker <sam@quadrocket.co.uk>
Copy link
Contributor

@showuon showuon 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, except the max-dynamic-servers config needs to modify to 6. Besides, there's a test failure seems to be related to the change:

Error:    IngressControllerManagerWithProfilesTest.testIngressControllerReplicaCountsWithoutCollocation:63 expected: <2> but was: <4>
Error:  Errors: 
Error:    IngressControllerManagerWithProfilesTest.testIngressControllerNodePlacement:48 NullPointer

@shawkins
Copy link
Contributor

Besides, there's a test failure seems to be related to the change:

It isn't related these changes, it's an unresolved issue we have with quarkus deterministically using mocks - for some reason it doesn't always use what you try to setup in the test.

@showuon
Copy link
Contributor

showuon commented Jan 17, 2023

Besides, there's a test failure seems to be related to the change:

It isn't related these changes, it's an unresolved issue we have with quarkus deterministically using mocks - for some reason it doesn't always use what you try to setup in the test.

Good to know. Then I don't have any other comments. Thanks.

Copy link
Contributor

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the important fix!

Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Jan 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

80.6% 80.6% Coverage
0.0% 0.0% Duplication

@k-wall k-wall added this to the 0.32.2 milestone Jan 17, 2023
@k-wall k-wall merged commit 5b91dfe into bf2fc6cc711aee1a0c2a:main Jan 17, 2023
@k-wall k-wall deleted the MGDSTRM-9181 branch January 17, 2023 13:43
biswassri pushed a commit that referenced this pull request Jan 17, 2023
…r's dynamic config manager. (#850)

* MGDSTRM-9181: Have kas ingress controller utilise the openshift-route's dynamic config manager.
* give fleetshard the responsibility to create blueprints for bootstrap/admin routes

Co-authored-by: Sam Barker <sam@quadrocket.co.uk>
biswassri pushed a commit that referenced this pull request Jan 17, 2023
…r's dynamic config manager. (#850)

* MGDSTRM-9181: Have kas ingress controller utilise the openshift-route's dynamic config manager.
* give fleetshard the responsibility to create blueprints for bootstrap/admin routes

Co-authored-by: Sam Barker <sam@quadrocket.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
operator changes related to operator sync changes related to sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants