-
Notifications
You must be signed in to change notification settings - Fork 363
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
Feat: adding support for PodDisruptionBudget with eg control plane #3545
Conversation
c18b34a
to
86181a6
Compare
Signed-off-by: Alexander Volchok <[email protected]>
86181a6
to
5698a61
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3545 +/- ##
=======================================
Coverage 68.17% 68.17%
=======================================
Files 168 168
Lines 20484 20484
=======================================
Hits 13964 13964
Misses 5511 5511
Partials 1009 1009 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Alexander Volchok <[email protected]>
/retest |
Signed-off-by: Alexander Volchok <[email protected]>
f6ec14e
to
cca1e46
Compare
charts/gateway-addons-helm/README.md
Outdated
@@ -0,0 +1,86 @@ | |||
# gateway-addons-helm |
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.
Are the changes in gateway-addons-helm intentional?
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.
will be fixed via #3555
@@ -18,7 +18,8 @@ global: | |||
pullPolicy: IfNotPresent | |||
# List of secrets in the same namespace of the component that can be used to pull images from private repositories. | |||
pullSecrets: [] | |||
|
|||
podDisruptionBudget: |
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.
maybe keep this 0 for backwards compatibility?
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.
Changed this to 0.
This reverts commit cca1e46. Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
test/helm/default-config.out.yaml
Outdated
@@ -1,5 +1,19 @@ | |||
--- | |||
# Source: gateway-helm/templates/envoy-gateway-deployment.yaml | |||
apiVersion: policy/v1 |
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 seems unreasonable, PDB should be opt-in.
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 changed this, we will deploy an extra pdb resource only if podDisruptionBudget.minAvailable is configured to be > 0.
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
/retest |
@@ -5,6 +5,20 @@ metadata: | |||
namespace: '{{ .Release.Namespace }}' | |||
labels: | |||
{{- include "eg.labels" . | nindent 4 }} | |||
--- | |||
{{- if and .Values.podDisruptionBudget.minAvailable (ge (int .Values.podDisruptionBudget.minAvailable) 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.
is this indentation needed?
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.
fixed
Signed-off-by: Alexander Volchok <[email protected]>
6d65f31
to
019723b
Compare
/retest |
test/helm/default-config.out.yaml
Outdated
@@ -344,6 +344,7 @@ spec: | |||
targetPort: 19001 | |||
--- | |||
# Source: gateway-helm/templates/envoy-gateway-deployment.yaml | |||
--- |
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.
typo?
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.
it would be nice if you can add some helm test, under test/helm
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.
Sure, I have added a test.
can you rebase with |
Co-authored-by: zirain <[email protected]> Signed-off-by: Alex Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
3847c3f
to
e7318cc
Compare
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[email protected]>
Signed-off-by: Alexander Volchok <[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.
LGTM
Signed-off-by: Alexander Volchok <[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.
LGTM! Thanks
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.
LGTM!
By adding a PDB, users can define policies that prevent too many instances of the Envoy Gateway control plane from being simultaneously disrupted during voluntary operations such as node upgrades, rolling updates, or manual interventions.
Adds EG controller pdp for #3546