-
Notifications
You must be signed in to change notification settings - Fork 813
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
Implemented PodDisruptionBudget on relevant deployments #2740
Implemented PodDisruptionBudget on relevant deployments #2740
Conversation
Build Succeeded 👏 Build Id: 509435e5-e37b-4c21-a44b-953204dc26b2 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Thanks for the PR! I will take a look at it once the in-progress release is finalized (also, you will probably have to rebase and deal with conflicts in the documentation changes at that point). |
8ca4d02
to
454ade0
Compare
Build Succeeded 👏 Build Id: 9f9325d1-9094-497f-9c40-760d1d8edb4d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 9de7fb60-ffca-41e9-927b-85b51581ea61 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@valentintorikian - As expected, you now need to rebase your documentation changes. |
454ade0
to
807468d
Compare
@roberthbailey rebased |
Build Succeeded 👏 Build Id: 41e283be-ac80-41ea-9543-2c6456c75670 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@roberthbailey just gently bumping if this is good to go? Looks like you were reviewing 😃 |
name: agones-ping-pdb | ||
spec: | ||
minAvailable: {{ .Values.agones.ping.pdb.minAvailable }} | ||
maxUnavailable: {{ .Values.agones.ping.pdb.maxUnavailable }} |
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.
According to https://kubernetes.io/docs/tasks/run-application/configure-pdb/#specifying-a-poddisruptionbudget the above two fields are mutually exclusive (e.g. you can only set one or the other).
It isn't clear to me what happens if the spec looks like:
spec:
minAvailable: 2
maxUnavailable:
which is what will be produced if the boolean to enable this is toggled to true. Is that spec accepted by k8s or rejected?
Does the template here need to be more complicated so that we only set a single value?
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 was curious, so I tested this. It works fine, and sends the JSON you might expect: "spec":{"maxUnavailable":null,"minAvailable":0, [...]}
.
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.
Thanks @zmerlynn for checking that out. Do you think this is ok to submit as-is then?
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 it should still be documented, but we could also merge and come back to that later.
| Parameter | Description | Default | | ||
|---------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------| | ||
| `agones.allocator.pdb.enabled` | Set to `true` to enable the creation of a [PodDisruptionBudget](https://kubernetes.io/docs/tasks/run-application/configure-pdb/) for the allocator deployment | `false` | | ||
| `agones.allocator.pdb.minAvailable` | Description of the number of pods from that set that must still be available after the eviction, even in the absence of the evicted pod. Can be either an absolute number or a percentage. | `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.
In each of the lines for minAvailable and maxUnavailable can you note that the two fields are mutually exclusive? Following the link to PDBs you can find this but it would be nice to highlight it here as well.
Build Succeeded 👏 Build Id: e10468d0-5167-4688-91cc-6c0bade55422 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Just a gentle reminder that our Release Candidate comes out next week, so if you would like this in our next release, Tuesday next week is the deadline. |
Build Failed 😱 Build Id: 3dd445f7-851d-442a-84d6-f33b068dc276 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 4ce81b70-be04-441a-8f14-6eee240e15b8 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 68990e16-e340-41db-85fc-3d7da364742b The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
- added a pdb on the ping deployment - added a pdb on the allocator deployment - default to no creation of pdb, opt-in install
f1df1bd
to
0894d7b
Compare
Build Succeeded 👏 Build Id: 47000304-7854-4a68-9d75-626b078bce56 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: e40326e6-0fc9-4156-89d0-be92c08192a1 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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.
@roberthbailey I'm happy to fix up the documentation on mutual-exclusivity after this goes in.
It looks like we could even use an approach like https://stackoverflow.com/questions/53508229/helm-chart-with-dependent-values to just ensure it. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: roberthbailey, valentintorikian, zmerlynn The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build Succeeded 👏 Build Id: b062ad7a-4082-4a31-af8c-1caf1950a613 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Added (optional) PodDisruptionBudget creation for the
ping
andallocator
deployment. Disabled by default.Will allow for better management of voluntary disruptions.
Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
N/A