Skip to content
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

Added configuration for PodDisruptionBudget #161

Merged
merged 10 commits into from
Apr 3, 2023
Merged

Added configuration for PodDisruptionBudget #161

merged 10 commits into from
Apr 3, 2023

Conversation

d7oc
Copy link
Contributor

@d7oc d7oc commented Mar 22, 2023

Description

This PR adds the configuration for PodDisruptionBudget. It can be configured globally and/or overwrites by service/app.

Related Issue

Motivation and Context

Feature Request

How Has This Been Tested?

Tested with helm template and the following own values YAML:

features:
  basicAuthentication: true
podDisruptionBudget:
  maxUnavailable: 1
services:
 webdav:
   podDisruptionBudget:
     maxUnavailable: 2

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation generated (make docs) and committed
  • Documentation ticket raised:
  • Documentation PR created:

@d7oc d7oc added the Status:Needs-Review Needs review from a maintainer label Mar 22, 2023
@d7oc
Copy link
Contributor Author

d7oc commented Mar 22, 2023

@wkloucek It would be nice if we could harmonise the app names between the values file and the name of the app in the metadata in the future.

After this change the template I used for PDB generation could be reduced by one argument in the dict.

@d7oc d7oc requested a review from wkloucek March 22, 2023 16:51
@wkloucek wkloucek mentioned this pull request Mar 23, 2023
@wkloucek
Copy link
Contributor

@wkloucek It would be nice if we could harmonise the app names between the values file and the name of the app in the metadata in the future.

After this change the template I used for PDB generation could be reduced by one argument in the dict.

I addressed this in #164. Something we should definitely do early (since it's most likely a breaking change)

@d7oc
Copy link
Contributor Author

d7oc commented Mar 23, 2023

Then I would move this PR to draft until #164 is done and grab #164 in between. Fine for you?

charts/ocis/templates/notifications/pdb.yaml Outdated Show resolved Hide resolved
charts/ocis/templates/_common/_tplvalues.tpl Outdated Show resolved Hide resolved
@d7oc d7oc marked this pull request as draft March 24, 2023 14:58
@d7oc
Copy link
Contributor Author

d7oc commented Mar 24, 2023

I converted the PR to draft right now to avoid accidental merging. I will address #164 first.

@d7oc d7oc marked this pull request as ready for review March 31, 2023 16:23
@d7oc
Copy link
Contributor Author

d7oc commented Mar 31, 2023

Ready for final review and merge.

@wkloucek
Copy link
Contributor

wkloucek commented Apr 3, 2023

@d7oc wouldn't it make sense to add the PDB to all services?

From the docs (https://kubernetes.io/docs/tasks/run-application/configure-pdb/):

Single-instance Stateful Application:
Concern: do not terminate this application without talking to me.
Possible Solution 1: Do not use a PDB and tolerate occasional downtime.
Possible Solution 2: Set PDB with maxUnavailable=0. Have an understanding (outside of Kubernetes) that the cluster operator needs to consult you before termination. When the cluster operator contacts you, prepare for downtime, and then delete the PDB to indicate readiness for disruption. Recreate afterwards.

Copy link
Contributor

@wkloucek wkloucek left a comment

Choose a reason for hiding this comment

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

Otherwise looks good

@@ -0,0 +1,2 @@
{{- include "ocis.appNames" (dict "scope" . "appName" "appNameAudit" "appNameSuffix" "") -}}
{{ include "ocis.pdb" . }}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some more no "Newline at end of file" files.

@d7oc
Copy link
Contributor Author

d7oc commented Apr 3, 2023

@d7oc wouldn't it make sense to add the PDB to all services?

From the docs (https://kubernetes.io/docs/tasks/run-application/configure-pdb/):

Single-instance Stateful Application: Concern: do not terminate this application without talking to me. Possible Solution 1: Do not use a PDB and tolerate occasional downtime. Possible Solution 2: Set PDB with maxUnavailable=0. Have an understanding (outside of Kubernetes) that the cluster operator needs to consult you before termination. When the cluster operator contacts you, prepare for downtime, and then delete the PDB to indicate readiness for disruption. Recreate afterwards.

We can add it but as written in the statement the technical protection won't properly work there needs to be communication outside the Kubernetes environment. So to me the benefit is quite limited. And the downside I see is that people set this to 1 and start to wonder why the single-instance pods aren't updated.
How are chances that we get replica support for the outstanding services? That would be my preferred approach.

@wkloucek
Copy link
Contributor

wkloucek commented Apr 3, 2023

@d7oc wouldn't it make sense to add the PDB to all services?
From the docs (https://kubernetes.io/docs/tasks/run-application/configure-pdb/):
Single-instance Stateful Application: Concern: do not terminate this application without talking to me. Possible Solution 1: Do not use a PDB and tolerate occasional downtime. Possible Solution 2: Set PDB with maxUnavailable=0. Have an understanding (outside of Kubernetes) that the cluster operator needs to consult you before termination. When the cluster operator contacts you, prepare for downtime, and then delete the PDB to indicate readiness for disruption. Recreate afterwards.

We can add it but as written in the statement the technical protection won't properly work there needs to be communication outside the Kubernetes environment. So to me the benefit is quite limited. And the downside I see is that people set this to 1 and start to wonder why the single-instance pods aren't updated. How are chances that we get replica support for the outstanding services? That would be my preferred approach.

Fine for me. You're right some services' Deployment could be replaced by a StatefulSet (IDM, NATS, IDP; but we're not recommending to to use them anyways, so no priority). other services still wait to be scalable, see #15

@wkloucek wkloucek merged commit 96eb15f into master Apr 3, 2023
@delete-merged-branch delete-merged-branch bot deleted the d7_issue_149 branch April 3, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add configuration for PodDisruptionBudget
2 participants