Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add PodDisruptionBudget for NodePools #547
Add PodDisruptionBudget for NodePools #547
Changes from 5 commits
ed4a738
233af07
cba18fd
3753f82
9010511
2977313
9fdaa45
dc6c129
5a48b8a
e9cdcd8
da1d0fa
0f7cea6
b9c6536
880d744
dec7dee
07be50a
923dc89
b0eb3b7
627f849
5d5dc7d
dce4cf9
9bce071
4663230
68867a1
cafddb4
1930f59
7353284
94a2086
dfb6702
85d9071
c71c864
f554e36
c947b2c
5107018
4ec85fb
6d4ed49
97bcf4b
981ceba
59958e2
9634e3e
3f3314e
a4d2120
64e7169
090a11f
500a5de
4844217
1120c45
75082d8
e4126d5
4dd25e1
ec1ce0c
5b8fd71
2a9999a
74694a4
5bf31d0
67a7463
7d57ef2
7eda7f7
f4bffae
283d2be
d5e6c5d
89aa2ba
b8c487b
9235ac9
adcf3e1
ffc1580
7c760e5
4bc3e37
cb1a571
8afb5a5
5c42fdf
ac4361f
a10b277
70c8e6f
60a90cc
88eef00
fc3abf8
c6bfd15
ed9fe07
7a5169c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
The change also needs to be copied into the helm chart
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.
CRD changes have not been copied to helm chart
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.
Changes have still not been copied to the helm chart
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.
No need to do this diff logic, just generate the PDB as it should be and then use
reconcileResource
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.
Do you mean that i have to reconcile it everytime after that it exist without any diff checking ? why ?
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.
The diff checking is done by
reconcileResource
using kubernetes mechanisms, which is safer and cleaner than doing it yourself. Also we use this mechanism everywhere else in the operator so usingreconcileResource
is consistent.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 you should also have logic to generate a default PDB if neither MinAvailable or MaxUnavailable are set. A good default could be to set
MaxUnAvailable=1
.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.
So i thought about that also, but i could not find any docs of ES/OS that they are reocmmend about default value...
only that : https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-pod-disruption-budget.html
and the value here is true.
I dont what to make customers to expirince things that they are not defined.
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.
In the second paragraph of the elastic docs you linked is written "ECK manages a default PDB per Elasticsearch resource. It allows one Elasticsearch Pod to be taken down". That for me is a clear indication that the default is
MaxUnavailable=1
. So doing it the same for our operator would be consistent.They need to explicitly enable the budget anyway, and as long as it is documented what the default budget is I don't see a problem with that. On the contrary, one of the jobs of the operator is to do sensible defaults, so setting a default config for the budget if the user enables it is normal behaviour IMO.
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 needs an else to delete a potentially existing PDB if enable is set to false
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.
Else case is still missing
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.
Deletion is still not handled, is it?
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.
Else with deletion is not handled