Skip to content

Use MaxUnavailable for PDB #8590

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mrkm4ntr
Copy link
Contributor

@mrkm4ntr mrkm4ntr commented Apr 3, 2025

We should use MaxUnavailable for simplicity.

// MaxUnavailable can only be used if the selector matches a builtin controller selector
// (eg. Deployments, StatefulSets, etc.). We cannot use it with our own cluster-name selector.

This comment was written by this reason. According to this, maxUnavailable can be used only when selected pods are managed by only 1 StatefulSet. But it's not true. We can use maxUnavailable for the case multiple StatefulSets are managing selected pods. See here.

If this change is valid, I will change the document as well.

@botelastic botelastic bot added the triage label Apr 3, 2025
@barkbay
Copy link
Contributor

barkbay commented Apr 28, 2025

Interesting, thanks for your suggestion! While I think you're right, I'm a bit on the fence with that change for the following reasons:

  • I'm wondering if we want to keep that idea that Elasticsearch Pods are actually controlled by the ECK operator, and using StatefulSets should be an implementation detail which may change in the future.
  • Do we know if that case (1 pdb with several built-in controllers) is covered by some unit tests in the pdb controller? You mentioned a comment, but I would like to know if we have any guarantee that this logic is not going to change (The K8s documentation is not really clear, so I'm trying to understand if we are not in a corner case that may no longer be supported at some point).

@mrkm4ntr
Copy link
Contributor Author

I'm wondering if we want to keep that idea that Elasticsearch Pods are actually controlled by the ECK operator, and using StatefulSets should be an implementation detail which may change in the future.

If ECK will stop to use StatefulSet, I think we will create new CRD for managing each NodeSet like current StatefulSet does. If so, that CRD can have scale subresource then we can use maxUnavailable.

Do we know if that case (1 pdb with several built-in controllers) is covered by some unit tests in the pdb controller? You mentioned a comment, but I would like to know if we have any guarantee that this logic is not going to change (The K8s documentation is not really clear, so I'm trying to understand if we are not in a corner case that may no longer be supported at some point).

Unfortunately no. But I feel current implementation is really straightforward. I hope they won't introduce a breaking change in this...

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

Successfully merging this pull request may close these issues.

2 participants