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

prepare-downscale: exclude pods in the same zone when finding unavailable pod #99

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

dimitarvdimitrov
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov commented Dec 12, 2023

Having unavailable or not up to date pods in the same zone we're trying to scale down shouldn't really stop us from downscaling the zone.

…able pods

Having unavailable or not up-to-date pods in the same zone we're trying to scale down shouldn't really stop us from downscaling the zone.
@dimitarvdimitrov dimitarvdimitrov requested a review from a team as a code owner December 12, 2023 08:31
@dimitarvdimitrov
Copy link
Contributor Author

dimitarvdimitrov commented Dec 12, 2023

@56quarters can you take a look at this? I'm not sure if there's an edge case that I didn't acccount for.

I checked what would happen if we try to prepare a non-ready pod for downscale - supposedly the request to it will fail and the whole downscale will be aborted. But in reality this should be rare because we need only the pods being downscaled to be ready - the rest can be non-ready or not up to date.

@jhalterman
Copy link
Member

jhalterman commented Dec 12, 2023

I'm not sure if this restriction was intended for downscales, but resharding and avoiding gaps in data is the concern that comes to mind for me, more than the request succeeding.

@dimitarvdimitrov
Copy link
Contributor Author

I'm not sure if this restriction was intended for downscales, but resharding and avoiding gaps in data is the concern that comes to mind for me, more than the request succeeding.

I'm not sure I follow @jhalterman. If we are downscaling zone-X, then it shouldn't matter that there are unavailable pods in zone-X. We avoid gaps by making sure that the other zones have available pods.

And resharding is I think a natural consequence of scaling down. Not sure if we can avoid it.

Did I understand your comment correctly?

@jhalterman
Copy link
Member

If we are downscaling zone-X, then it shouldn't matter that there are unavailable pods in zone-X. We avoid gaps by making sure that the other zones have available pods.

That's right - I forgot we have that cross-sts check. This change seems safe to me then.

@dimitarvdimitrov dimitarvdimitrov merged commit bc406fe into main Dec 13, 2023
6 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/downscale-same-zone-with-not-ready branch December 13, 2023 18:41
@pracucci
Copy link
Collaborator

I reviewed this change too and I also think this change is good. It's not much different from the same check done in findDownscalesDoneMinTimeAgo(), which allows to downscale the same zone even if it was already downscaled less than "min time" ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants