Skip to content

Don't provide default values for PDBs in chart. #52199

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Don't provide default values for PDBs in chart. #52199

wants to merge 1 commit into from

Conversation

simi
Copy link
Contributor

@simi simi commented Jun 24, 2025

  • when minAvailable is used, maxUnavailable must be nullified on client side

currently

apiServer:
  replicas: 2
  podDisruptionBudget:
    enabled: true
    config:
      minAvailable: 1

fails with

Error: UPGRADE FAILED: failed to create resource: PodDisruptionBudget.policy "integ-rad-airflow-api-server-pdb" is invalid: spec: Invalid value: policy.PodDisruptionBudgetSpec{MinAvailable:(*intstr.IntOrString)(0xc022377660), Selector:(*v1.LabelSelector)(0xc022377680), MaxUnavailable:(*intstr.IntOrString)(0xc022377640), UnhealthyPodEvictionPolicy:(*policy.UnhealthyPodEvictionPolicyType)(nil)}: minAvailable and maxUnavailable cannot be both set

since it is merged with default value of maxUnavailable

apiServer:
  replicas: 2
  podDisruptionBudget:
    enabled: true
    config:
      minAvailable: 1
      maxUnavailable: 1

resulting in unsupported setup

as a "bandaid" null can be used (which is not super friendly IMHO)

apiServer:
  replicas: 2
  podDisruptionBudget:
    enabled: true
    config:
      minAvailable: 1
      maxUnavailable: null

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Jun 24, 2025
@simi simi force-pushed the friendlier-pdb branch from d7f824d to 43aa622 Compare June 25, 2025 02:05
@simi simi marked this pull request as draft June 25, 2025 04:41
@simi simi force-pushed the friendlier-pdb branch 2 times, most recently from f816215 to 4d81fc7 Compare June 25, 2025 04:59
@simi
Don't provide default values for PDBs in chart.
9ed8adc 
- when minAvailable is used, maxUnavailable must be nullified on client side
- keep it backwards compatible
- add null as valid value to schema (it was already accepted before)
@simi simi force-pushed the friendlier-pdb branch from 4d81fc7 to 9ed8adc Compare June 25, 2025 05:39
@simi
Copy link
Contributor Author

simi commented Jun 25, 2025

It turned out it is not that simple to make it backwards compatible. Sadly helm checks schema in "strange" way, so even value is in the end not part of the final output, it is checked when given. I had to fix schema to reflect better what's really happening (default and possible values).

In the end I'm not fully sure, this is worth it. On the other side it improves various parts.

@simi simi marked this pull request as ready for review June 25, 2025 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstandish dstandish Awaiting requested review from dstandish dstandish is a code owner

@jedcunningham jedcunningham Awaiting requested review from jedcunningham jedcunningham is a code owner

@hussein-awala hussein-awala Awaiting requested review from hussein-awala hussein-awala is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area:helm-chart Airflow Helm Chart
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@simi