-
Notifications
You must be signed in to change notification settings - Fork 155
Update eck pdb docs #2361
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
Update eck pdb docs #2361
Conversation
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
🔍 Preview links for changed docs |
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.
provided a suggestion with the info at hand. might need some adjustment depending on the impact of the stack version on this process.
ECK manages a default PDB per {{es}} resource. It allows one {{es}} Pod to be taken down, as long as the cluster has a `green` health. Single-node clusters are not considered highly available and can always be disrupted. | ||
ECK manages either a single default PDB, or multiple PDBs per {{es}} resource according to the license available. | ||
|
||
## Enterprise licensed customers |
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.
## Enterprise licensed customers | |
## Enterprise licensed customers | |
```{applies_to} | |
deployment: | |
eck: ga 3.2 | |
``` |
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've implemented this suggestion, but why do we note ga
here?
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.
you can view ga
as the default - we just want record the lifecycle state so it's clear the what state of the feature was in the indicated version. ga
in the frontend doesn't render - only the version # renders.
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 am not super happy with these headings:
- wording: we do not license customers, customers buy a license from us
- correctness: non-enterprise users are technically not customers but users (they did not buy anything?)
- I would rather we lead with the feature difference than the license (which needs to be mentioned too no argument about that)
Suggestion:
Advanced PodDisruptionBudget logic (Enterprise License Required)
Default PodDisruptionBudget logic (Basic License)
Thoughts? @shainaraskas @naemono
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 have updated the headings, but I've used the word rules
rather than logic
. It seemed to be more clear. Let me know if you feel the opposite.
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.
Rules is good too I think 👍
|
||
It allows one {{es}} Pod to be taken down, as long as the cluster has a `green` health. Single-node clusters are not considered highly available and can always be disrupted. | ||
|
||
In the {{es}} specification, you can change the default behavior as follows: |
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 the instructions below this line only apply to non-enterprise? if not, you might want to change the headings a little:
## Default behavior
...
### Enterprise licensed customers
...
### Non-enterprise licensed customers
...
## Override the default behavior
...
## Pod disruption budget ...
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 have also made this suggestion, as the instructions apply to both licensed and non-licensed customers. I'll see how it turns out when it deploys...
Signed-off-by: Michael Montgomery <[email protected]>
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.
looks good. have a few more suggestions for you - let me know if you'd like me to commit a pass of them
[`maxUnavailable`](https://kubernetes.io/docs/tasks/run-application/configure-pdb/#arbitrary-controllers-and-selectors) cannot be used with an arbitrary label selector, therefore `minAvailable` is used in this example. | ||
:::: | ||
|
||
## Pod disruption budget per nodeSet [k8s-pdb-per-nodeset] |
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.
should this be a child of the override instructions? wonder if it makes sense to indicate at the top of the "override" section that you can do it at either of these levels, and then put the examples each under a H3
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.
let me know if you'd like me to make a commit with my suggested changes
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've added these as indicated. Let me know if the changes make sense.
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
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.
Pull Request Overview
Updates the ECK Pod Disruption Budget documentation to reflect new enterprise-licensed behavior introduced in ECK 3.2, where different PDB handling is provided based on license type.
- Adds distinction between enterprise and non-enterprise licensed behavior for PDB management
- Documents new per-nodeSet PDB creation for enterprise licenses with role-specific health requirements
- Reorganizes content structure with clearer sections and updated examples
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
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.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
approving to unblock but please fix those heading levels before shipping :)
|
||
In {{eck}} clusters that do not have an enterprise license, one {{es}} Pod can be taken down at a time, as long as the cluster has a health status of `green`. Single-node clusters are not considered highly available and can always be disrupted. | ||
|
||
### Overriding the default behavior |
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.
we still need to fix these heading levels per this comment #2361 (comment)
easiest way is to make this an H2 and its child an H3. alternative is outlined in the linked thread
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.
+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.
I have adjusted the headings. I swore I had previously done this, but clearly not. Let me know if this is what you intended.
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 following elastic/cloud-on-k8s#8780 is planned to be released with the ECK 3.2/Stack 9.2 release.
A bit off topic: this PR is targeting main
, do we want to target a release branch instead, as we did for 3.1? (mostly to make life easier for the next release manager)
I have both created an |
|
||
In {{eck}} clusters that do not have an enterprise license, one {{es}} Pod can be taken down at a time, as long as the cluster has a health status of `green`. Single-node clusters are not considered highly available and can always be disrupted. | ||
|
||
### Overriding the default behavior |
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.
+1
ECK manages a default PDB per {{es}} resource. It allows one {{es}} Pod to be taken down, as long as the cluster has a `green` health. Single-node clusters are not considered highly available and can always be disrupted. | ||
ECK manages either a single default PDB, or multiple PDBs per {{es}} resource according to the license available. | ||
|
||
## Enterprise licensed customers |
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 am not super happy with these headings:
- wording: we do not license customers, customers buy a license from us
- correctness: non-enterprise users are technically not customers but users (they did not buy anything?)
- I would rather we lead with the feature difference than the license (which needs to be mentioned too no argument about that)
Suggestion:
Advanced PodDisruptionBudget logic (Enterprise License Required)
Default PodDisruptionBudget logic (Basic License)
Thoughts? @shainaraskas @naemono
Signed-off-by: Michael Montgomery <[email protected]>
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.
A few more nits around capitalization and how the headings render. Otherise lgtm.
Signed-off-by: Michael Montgomery <[email protected]>
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.
new headings and heading nesting looks good. couple of small comments while I'm here
Signed-off-by: Michael Montgomery <[email protected]>
The following elastic/cloud-on-k8s#8780 is planned to be released with the ECK 3.2/Stack 9.2 release. How do we tag this specifically for that?