PodDisruptionBudget defaults can still block node drain

[ryanohnemus]

Component(s)

No response

What happened?

Description

I am unaware of a way to disable PodDisruptionBudgets (PDBs) altogether for our OpenTelemetryCollector deployment resources, so please let me know if there is a way to do that and we can close this issue.

The issue:

When a user has a working deployment of opentelemetry collector (managed by the operator) and then attempts an update on the OpenTelemetryCollector that has a breaking change (ie bad permission/misconfiguration/etc) it causes the rolling out deployment to stay in a crashloop, while the previous deployment stays up and can not be removed (due to the pdb default MaxUnavailable 1). This prevents node drains from removing the old deployment and causes k8s node upgrade/rollouts to time out.

Steps to Reproduce

1. Have a working OpenTelemetryCollector resource that is managed by opentelemetry-operator
2. update that resource with something that would cause a new deployment with misconfiguration (ie a bad feature-gate flag would probably work)
3. new deployment goes into a crash loop while old deployment stays up (expected, but blocks node drain)

Resolution / Suggestion:

The workaround I have found is by having users of the OpenTelemetryCollector resource setting:

spec:
  podDisruptionBudget:
    minAvailable: 0

I'm hoping an easy solution would just be to change the default of the MaxUnavailable: 1 to MinAvailable: 0 instead... code link:

opentelemetry-operator/internal/manifests/collector/poddisruptionbudget.go

Lines 26 to 28 in 5c86398

	MaxUnavailable: &intstr.IntOrString{
	Type: intstr.Int,
	IntVal: 1,

Is this possible, if not could there be an option added to disable setting PDBs at all at the operator level?

I understand users of the OpenTelemetryCollector resource should ensure their update/rollouts work when updating it, but unfortunately that not something i've been able to force users to do 😄 ...so having a less restrictive default seems like an easier option.

Kubernetes Version

1.31

Operator version

0.117

Collector version

0.117.0

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

Log output

Additional context

No response

[swiatekm]

I'm hoping an easy solution would just be to change the default of the MaxUnavailable: 1 to MinAvailable: 0 instead... code link:

Does this not work? If you supply your own PDB spec, the operator should not modify it in any way.

[ryanohnemus]

@swiatekm this works, but it has to be specified in every OpenTelemetryCollector resource instead of something that can be defaulted in the otel-operator when no pdb settings are specified.

It has unfortunately been a challenge to get our users to add that code block to each of their OpenTelemetryCollector resources to prevent the issue, so I opened this issue in the hopes we could change the defaults at operator level.

[swiatekm]

To be honest, what you're describing sounds like the right default behavior to me. The user pushes an invalid update, the collector stays in its pre-update state and an availability constraint represented by the default PDB is enforced. As a rule, we want the operator defaults to be as close to production-ready as we can, and I believe the default PDB we have right now accomplishes this goal.

It sounds like what you're really asking for is the ability to affect this default behavior at the operator level, which is not unreasonable, but we don't have very effective means of accomplishing this kind of customization right now. Could a more general-purpose admission control mechanism work for you? Kyverno, for example, exists specifically to enforce these kinds of policies

[ryanohnemus]

@swiatekm that sounds fair... I will look into the kyverno option, I have not looked into it before. Do you think it would be reasonable to convert this to a feature request instead of a bug ticket, with the feature being the ability to disable default pod disruption budgets for deployment resource types?

If you don't think that feature is something we'd want in the operator, feel free to close this ticket.

[swiatekm]

I don't think we currently want a feature in the operator that allows CRD defaults to be modified via operator configuration. In a lot of cases, this is effectively impossible because the default value is embedded in the CRD itself anyway. I'm going to put this issue on the docket for the next operator SIG meeting to see if this opinion is controversial. With that said, if we ever do implement it, it's not going to happen quickly, so in the meantime kyverno's still your best bet.