-
Notifications
You must be signed in to change notification settings - Fork 114
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
Call rebalance API once all pods are updated #625
Call rebalance API once all pods are updated #625
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I have two edge cases that I think we need to account for:
- In the case that all the pods are updates, the async rebalance command will be started. The user can then change the spec again, and the updatedPodCount will then again be
0
, because all pods will be out of date. At that point we will have forgotten about the rebalance command that we started. We need to make sure that we complete any async command before we start a new one, so I suggest that we split off the async command to a separate ClusterOp, that we start after the restart is complete. That way another restart will only begin once the rebalance is finished. - If I'm correct, we are rebalancing even if replicas weren't moved around during the rolling restart. In my opinion, we should only rebalance if replicas were moved around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go @radu-gheorghe , give it a look!
Looks good to me, too, thanks a lot @HoustonPutman! I already had a look yesterday, too, and noticed you couldn't help yourself and fixed your 2) above 🙂 |
Haha I remembered that we had the lovely readinessConditions that told us if the cloud was ephemeral, so it was very easy to do it! |
Timid attempt to fix #615
It works on my machine, it seems to fix the problem, but:
Any feedback is welcome, I'd be glad to push this forward!