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

Relax pod disruption budget for single node clusters #3167

Merged
merged 1 commit into from
Jun 12, 2020
Merged

Relax pod disruption budget for single node clusters #3167

merged 1 commit into from
Jun 12, 2020

Conversation

sanderma
Copy link
Contributor

See issue PDB per Node type (Master, Data, Ingest) #2936

This PR will make sure that when someone deploys a single node cluster, the pdb does not get set to 1.

If someone deploys a single node cluster now, it can make sure the k8s cluster cannot reboot.
The default setting for single node, IMHO, should not set the pdb to 1.
This change makes sure that when it is single node, the pdb is 0. When the elastic cluster is scaled up, the pdb is calculated as "normal".

If you set maxUnavailable to 0% or 0, or you set minAvailable to 100% or the number of replicas, you are requiring zero voluntary evictions. When you set zero voluntary evictions for a workload object such as ReplicaSet, then you cannot successfully drain a Node running one of those Pods. If you try to drain a Node where an unevictable Pod is running, the drain never completes. This is permitted as per the semantics of PodDisruptionBudget.
source

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented May 30, 2020 •
edited
Loading

💚 CLA has been signed

@botelastic botelastic bot added the triage label May 30, 2020
sebgl
sebgl requested changes Jun 2, 2020
View reviewed changes
Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

Thanks @sanderma! I think your proposal makes sense 👍
I have a few suggestions about moving the code to a different place. Also can you make sure unit tests are modified accordingly?

pkg/controller/elasticsearch/pdb/reconcile.go Outdated Show resolved Hide resolved
@sebgl
Copy link
Contributor

sebgl commented Jun 2, 2020

There still is the state that if someone creates a nodeset with a single node, that pod will still be blocked from rebooting. But perhaps that is to the user to fix.

Yeah I would consider it the user responsibility at this point.

sebgl
sebgl reviewed Jun 4, 2020
View reviewed changes
Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

@sanderma it looks like you need to resolve some conflicts for this PR to be merged.
Also, can you sign the CLA? https://www.elastic.co/contributor-agreement

@sanderma
Copy link
Contributor Author

sanderma commented Jun 4, 2020

Changes LGTM!

@sanderma it looks like you need to resolve some conflicts for this PR to be merged.

Ooof, I was running on an old branch as it seems (used v1beta1, not v1) so changed that.

Also, can you sign the CLA? https://www.elastic.co/contributor-agreement

Signed it (I did in the past btw, but perhaps this needs to be done regularly?)

@sebgl
Copy link
Contributor

sebgl commented Jun 4, 2020

Jenkins test this please.

@sebgl
Copy link
Contributor

sebgl commented Jun 4, 2020

cla/check

@sebgl sebgl added the >enhancement Enhancement of existing functionality label Jun 4, 2020
@botelastic botelastic bot removed the triage label Jun 4, 2020
@sebgl
Copy link
Contributor

sebgl commented Jun 4, 2020

@sanderma it looks like your commits were signed by different users (I see sanderma and sanderMa). I guess that's why the cla check is complaining.

An easy way out could be to rebase and squash all your commits into a single one signed by the user you want.

@sanderma
set pdb to 0 if nodecount == 1
7d6abf4 
moved logic to allowedDisruptions()

comment dots removed.
@sanderma
Copy link
Contributor Author

@sanderma it looks like your commits were signed by different users (I see sanderma and sanderMa). I guess that's why the cla check is complaining.

An easy way out could be to rebase and squash all your commits into a single one signed by the user you want.

Pff, I've learned to start with a pull before diving in again...

@sanderma sanderma requested a review from sebgl June 10, 2020 17:11
@sebgl
Copy link
Contributor

sebgl commented Jun 12, 2020

Jenkins test this please

sebgl added a commit to sebgl/cloud-on-k8s that referenced this pull request Jun 12, 2020
@sebgl
Mention single-node clusters PDB exception in the docs
65c7517 
With elastic#3167 we allow the
single Pod of a single node cluster to be disrupted, since we consider a
single-node cluster is not intended to be highly available anyway.
sebgl added a commit to sebgl/cloud-on-k8s that referenced this pull request Jun 12, 2020
@sebgl
Mention single-node clusters PDB exception in the docs
7230d86 
With elastic#3167 we allow the
single Pod of a single node cluster to be disrupted, since we consider a
single-node cluster is not intended to be highly available anyway.
@sebgl sebgl mentioned this pull request Jun 12, 2020
Merged
sebgl
sebgl approved these changes Jun 12, 2020
View reviewed changes
Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @sanderma!

@sebgl sebgl added the v1.2.0 label Jun 12, 2020
@sebgl sebgl changed the title set pdb to 0 if nodecount == 1 Allow single-node clusters disruption in the default PDB Jun 12, 2020
@sebgl sebgl merged commit e1aa625 into elastic:master Jun 12, 2020
sebgl added a commit that referenced this pull request Jun 15, 2020
@sebgl
Mention single-node clusters PDB exception in the docs (#3229)
6dc94c2 
With #3167 we allow the
single Pod of a single node cluster to be disrupted, since we consider a
single-node cluster is not intended to be highly available anyway.
@charith-elastic charith-elastic changed the title Allow single-node clusters disruption in the default PDB Relax pod disruption budget for single node clusters Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebgl sebgl sebgl approved these changes

Assignees
No one assigned
Labels
>enhancement Enhancement of existing functionality v1.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sanderma @elasticmachine @sebgl