Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[incubator/elasticsearch] Add pod disruption budgets. #2530

Closed
wants to merge 4 commits into from

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Oct 20, 2017

So that clusters don't become unavailable during node restarts.

cc @icereval

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 20, 2017
Copy link
Collaborator

@icereval icereval left a comment

Choose a reason for hiding this comment

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

A couple small requests, thanks for the help!

@@ -127,3 +127,23 @@ spec:
resources:
requests:
storage: "{{ .Values.master.storage }}"
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move the inline definition to a file, naming suggestion master-pdb.yaml

@@ -131,3 +131,23 @@ spec:
resources:
requests:
storage: "{{ .Values.data.storage }}"
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inline definition to a file, naming suggestion data-pdb.yaml

@@ -38,6 +38,8 @@ master:
storage: "4Gi"
# storageClass: "ssd"
antiAffinity: "soft"
budget:
minAvailable: 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need readme documentation for this value

apiVersion: policy/v1beta1
kind: PodDisruptionBudget
metadata:
name: "{{ template "elasticsearch.master.fullname" . }}-master-budget"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should not need the ending suffix, assuming there is no specific reason behind this choice.

apiVersion: policy/v1beta1
kind: PodDisruptionBudget
metadata:
name: "{{ template "elasticsearch.master.fullname" . }}-data-budget"
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as below, but should be {{ template "elasticsearch.data.fullname" . }}

@unguiculus
Copy link
Member

Marking this as stale. Please update within one week.

@viglesiasce viglesiasce removed the stale label Nov 29, 2017
@viglesiasce
Copy link
Contributor

@unguiculus this is ready for review.

@viglesiasce
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 29, 2017
@unguiculus
Copy link
Member

@jmcarp Readme update still missing.

@unguiculus
Copy link
Member

Marking stale. Please update readme.

@unguiculus unguiculus self-assigned this Dec 9, 2017
@unguiculus
Copy link
Member

Closing as stale. Feel free to reopen if/when you decide to pick this up again.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
changes needed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants