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

Set replicas to MaxReplicas if HPA is enabled #833

Merged
merged 2 commits into from
Apr 27, 2022
Merged

Set replicas to MaxReplicas if HPA is enabled #833

merged 2 commits into from
Apr 27, 2022

Conversation

binjip978
Copy link
Contributor

If number of replicas in current deployment status is bigger than
MaxReplicas and HPA is enabled set deployment replicas to MaxReplicas.

@binjip978
Set replicas to MaxReplicas if HPA is enabled
3b6440c 
If number of replicas in current deployment status is bigger than
MaxReplicas and HPA is enabled set deployment replicas to MaxReplicas.
@binjip978 binjip978 requested a review from a team April 22, 2022 07:14
@pavolloffay
Copy link
Member

@binjip978 was the current behavior causing any issues?

@binjip978
Copy link
Contributor Author

@binjip978 was the current behavior causing any issues?

Yeah, during initial deployment or upgrade I saw that replicas count in status was much higher than it was specified in deployment or HPA configuration, the reason is not clear to me (maybe some issue with metrics server in our cluster)
But if current replica count in status will be bigger than MaxReplicas it will became part of deployment spec. And then two controllers will fight each other, most of time in our case it will converge to HPA values, but it takes time.

This problem is not always reproducible in our environment, but it may happen, most of time it works as expected.

@pavolloffay
Copy link
Member

@jpkrohling could you review it as well? You did a bunch of work around HPA in Jaeger operator.

jpkrohling
jpkrohling reviewed Apr 25, 2022
View reviewed changes
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

This PR is lacking tests. If e2e tests aren't easy to simulate the reported behavior, then at least unit tests should be added>

@binjip978
Move replicas calculation in seperate function
d9ca106 
Signed-off-by: binjip978 <pdp.eleven11@gmail.com>
@binjip978 binjip978 force-pushed the honor-max-replicas-for-hpa branch from dbc0877 to d9ca106 Compare April 27, 2022 06:41
@jpkrohling jpkrohling merged commit 2f80f65 into open-telemetry:main Apr 27, 2022
@binjip978 binjip978 deleted the honor-max-replicas-for-hpa branch April 27, 2022 15:56
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
@binjip978
Set replicas to MaxReplicas if HPA is enabled (open-telemetry#833)
e210ffc 
* Set replicas to MaxReplicas if HPA is enabled

If number of replicas in current deployment status is bigger than
MaxReplicas and HPA is enabled set deployment replicas to MaxReplicas.

* Move replicas calculation in seperate function

Signed-off-by: binjip978 <pdp.eleven11@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VineethReddy02 VineethReddy02 VineethReddy02 approved these changes

@pavolloffay pavolloffay pavolloffay approved these changes

@jpkrohling jpkrohling jpkrohling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@binjip978 @pavolloffay @jpkrohling @VineethReddy02