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

bug fix: create blueprints for suspended routes too #856

Merged
merged 2 commits into from
Jan 19, 2023
Merged

bug fix: create blueprints for suspended routes too #856

merged 2 commits into from
Jan 19, 2023

Conversation

k-wall
Copy link
Contributor

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

#850 was incomplete as we failed to consider suspended routes, so instance suspension was still leading to a haproxy restart. This creates blueprints for those routes too.

@k-wall k-wall requested a review from grdryn January 19, 2023 12:15
@github-actions github-actions bot added the operator changes related to operator label Jan 19, 2023
@k-wall k-wall requested a review from shawkins January 19, 2023 12:15
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

@k-wall k-wall added this to the 0.32.3 milestone Jan 19, 2023
@k-wall
Copy link
Contributor Author

k-wall commented Jan 19, 2023

I've confirmed this is properly handling the suspended case. Suspending an instance results in the addition of two more blueprints.

The bootstrap one has:

    haproxy.router.openshift.io/balance: leastconn
    haproxy.router.openshift.io/rate-limit-connections: "true"
    haproxy.router.openshift.io/rate-limit-connections.concurrent-tcp: "5"
    haproxy.router.openshift.io/rate-limit-connections.rate-tcp: "15"

and the one corresponding to the normal broker routes:

   haproxy.router.openshift.io/rate-limit-connections: "true"
    haproxy.router.openshift.io/rate-limit-connections.concurrent-tcp: "5"
    haproxy.router.openshift.io/rate-limit-connections.rate-tcp: "15"

Looking at memory, the ingress pods with the extra blueprints are now sitting at ~350Mi.

@shawkins
Copy link
Contributor

Looking at memory, the ingress pods with the extra blueprints are now sitting at ~350Mi.

Presumably the 500 setting is per pool, so I think we could safely drop that to 300 (50 instances * 3 brokers * 2 margin) now that we're subdividing things so much.

@k-wall
Copy link
Contributor Author

k-wall commented Jan 19, 2023

Looking at memory, the ingress pods with the extra blueprints are now sitting at ~350Mi.

Presumably the 500 setting is per pool, so I think we could safely drop that to 300 (50 instances * 3 brokers * 2 margin) now that we're subdividing things so much.

Yes, 500 is per pool. I'd be happy with 300 pool size. I'll do it. As we mentioned elsewhere there is provision to annotate the blueprint with a poolsize (after all 300 suspended instances = unlikely), but I vote we don't do that now. We can tune that later if we like.

@sonarqubecloud
Copy link

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 0 Code Smells

71.4% 71.4% Coverage
0.0% 0.0% Duplication

@k-wall k-wall merged commit 35615ad into bf2fc6cc711aee1a0c2a:main Jan 19, 2023
@k-wall k-wall deleted the add-blueprint-for-suspended-routes branch January 19, 2023 14:24
biswassri pushed a commit that referenced this pull request Jan 19, 2023
* bug fix: create blueprints for suspended routes too

* reduce pool size 500=>300
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
operator changes related to operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants