HelmController triggers multiple upgrades for same helm release

[anshuishere]

Issue
I used the new watcher which triggers a helm upgrade in the configMap, which is source for the value for a helm chart.

Both my configmap and HR object live in the same git repo.
When I update the HR to change the chart version as well as new fields in my values.yaml file, I notice multiple upgrades are triggered.

First upgrade: Old Helm Chart Version + New ConfigMap with new values
Second upgrade: New Helm Chart Version + New ConfigMap with new values

Problem
The first state is unintended. If the new values has a parameter which will not work with old chart version, my first upgrade goes to "pending upgrade" until timeout is reached.

Expectation
HC should not attempt an upgrade for new values if its coming from same commit where referred helm chart version is also changed.
It should avoid the half applied state since user intent is new HC + new value

Normal UpgradeSucceeded 12s helm-controller Helm upgrade succeeded for release test-namespace/podinfo.v6 with chart podinfo@6.9.2+971fe │ │ f0d04d5 │ │ Normal UpgradeSucceeded 4s helm-controller Helm upgrade succeeded for release test-namespace/podinfo.v7 with chart podinfo@6.9.4+8b862 │ │ d260d24

[stefanprodan]

HC should not attempt an upgrade for new values if its coming from same commit where referred helm chart version is also changed.

HC has no knowledge of Git, the HR is applied by kustomize-controller, so helm-controller does what it suppose to do.

[anshuishere]

Thanks @stefanprodan for the response

Isn't this a problem everyone will face though?

If my configmap is updated by KC and HC reacts to it first triggering an upgrade with incompatible helm values with old helm chart version , then triggering a second one with new version of chart and finally succeeding. Helm treats helm chart and values as one entity per release. But we're needing to divide it to two because of distributed nature of controllers.

Even though user intention is to trigger 1, not 2 upgrades when they change values and helm chart version in HR object simultaneously in a single commit.
I understand its an expected behaviour with the current setup but everyone will face this unintentional intermediate upgrade of helm release?

Looping in @matheuscscp as well as we discussed this ,when I brought this up in Jan 13th dev meeting.

Can be think of some form of debounce logic to avoid reconciliation of intermediate, undesired state such as this one.

[matheuscscp]

As mentioned in the dev meeting, Flux 2.8 (coming late next month) will come with controller-runtime v0.23:

https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.23.0

Under the Features section you can see this:

• Priorityqueue: Enable per default by @sbueringer in ✨ Priorityqueue: Enable per default kubernetes-sigs/controller-runtime#3332

The controller-runtime priority queue has debouncing logic.

Besides upgrading controller-runtime I don't see anything else we can do for this problem. The helm-controller does not know about Git, we won't drastically change the architecture of the Flux controllers to deal with this problem.

[sbueringer]

I don't think the priority queue has debouncing logic

[matheuscscp]

Hmm I read this in so many places, I remember @alvaroaleman saying this somewhere, let me find it

[matheuscscp]

kubernetes-sigs/controller-runtime#3334 (comment)

// PriorityQueue is a priority queue for a controller. It
// internally de-duplicates all items that are added to
// it. ...

[sbueringer]

Ah. I would say debouncing and de-duplication is not the same.

Anyway, the PQ in fact de-duplicates. But the queue implementation we used before also de-duplicated to some degree (IIRC it didn't duplicate between "ready" items and items still waiting to be come ready after err / RequeuAfter / Requeue was returned in Reconcile)

PQ will (intentionally) not de-duplicate e.g. if there is already a Reconcile in progress when an additional request is enqueued

[alvaroaleman]

First upgrade: Old Helm Chart Version + New ConfigMap with new values
Second upgrade: New Helm Chart Version + New ConfigMap with new values

Unless you atomically update the version and config map I don't think its possible to avoid this. I think the issue is likely that you update the configmap in place. The only way I can think of to avoid this is to create a new configMap, then have the helmchart update include both the new version and the new configmap reference, that is the only way to make it atomic.

Anyway, the PQ in fact de-duplicates. But the queue implementation we used before also de-duplicated to some degree (IIRC it didn't duplicate between "ready" items and items still waiting to be come ready after err / RequeuAfter / Requeue was returned in Reconcile)

Exactly, the priority queue is slightly better at de-duplicating than the upstream queue but I don't think that is relevant here

PQ will (intentionally) not de-duplicate e.g. if there is already a Reconcile in progress when an additional request is enqueued

To expand on this, its not just the priority queue its also the upstream queue that does this and its required for correctness because imagine the spec of an object gets changed while its being reconciled. There is locking for threadsafety that a given key can only be reconciled by one routine, if we now dropped the new request because its already being reconciled, the latest spec will not get reconciled at all unless yet another update happens to come in.

[matheuscscp]

If there's an active Reconcile and another reconcile request on the PQ, and a new request arrives (1 Reconcile + 2 reqs). Will it be de-duplicated? I'm fine with that, that's probably the best we can do

[alvaroaleman]

If there's an active Reconcile and another reconcile request on the PQ, and a new request arrives (1 Reconcile + 2 reqs). Will it be de-duplicated? I'm fine with that, that's probably the best we can do

Active reconcile doesn't get de-duplicated in any way. Everything else does. So if two identical requests come in they will get de-duplicated, regardless of if there is an active reconcile or not.

[matheuscscp]

Yep, that's been my understanding all along 👍 Thanks you both for jumping here!

[sbueringer]

And maybe just to clarify. What I would understand under debouncing is:

• multiple events are coming in within a few ms
• we wait for a few ms after the first event if more events for the same Request are arriving
• only 1 Request gets enqueued after a certain duration

Or how Google AI overview is describing it:

Debouncing is a programming technique that limits how often a function can be called by delaying its execution until a specific period of inactivity has passed since the last trigger, preventing performance issues from rapid, repeated events

I think that can be also quite useful, just not implemented in priority queue or controller-runtime in general.

[stefanprodan]

A debounce feature in the controller-runtime queue will not solve @anshuishere's issue. If you apply a ConfigMap and a HelmRelease at the same time, this doesn't mean helm-controller will receive the events milliseconds apart. The Kubernetes API may be busy doing something else, the events can be seconds apart if the applier gets rate limited, or if etcd is overwhelmed, or if the network is flaky, etc.

If you chose distributed configs then you must accept eventual consistency and ship backwards compatible Helm charts. The only way Flux can ensure an atomic upgrade of a HelmRelease is if both the Helm chart version and the values are embedded in spec.

[anshuishere]

Thanks for the great insights everyone💡

@stefanprodan Even with helm chart version and values are embedded in spec, there is another issue which is rare but can occur.

Again citing discussion with @matheuscscp (IIRC), currently in HC logic:

When a HelmRelease update changes both spec.chart.template.spec.version and spec.values, helm-controller can nondeterministically perform an intermediate Helm upgrade using the old chart artifact with the new values before later reconciling again with the new chart artifact and new values; this is because reconcileChartTemplate() first mutates the HelmChart spec, then helm-controller immediately reads the HelmChart and calls isSourceReady(), which only checks generation == observedGeneration and status.artifact != nil (not that the artifact revision matches the new chart spec), so a stale artifact is treated as ready and is passed to SecureLoadChartFromURL(source.GetArtifact().URL, ...) while already using the new values, resulting in AtomicRelease.Reconcile() being invoked with a mismatched chart+values combination until a second reconcile is triggered later by SourceRevisionChangePredicate when the artifact is updated.

T0: You update HelmRelease (chart + values)
T1: Helm controller reconciles
T2: Helm controller updates HelmChart spec
T3: Helm controller immediately reads HelmChart status
T4: Source controller has NOT updated artifact yet
T5: Helm controller proceeds using OLD artifact
T6: Helm controller runs Helm upgrade (old chart + new values)
T7: Source controller finishes downloading new chart
T8: HelmChart status.artifact is updated
T9: Helm controller reconciles again (new chart + new values)

Could this be a possible reason?
Can we add a check on helm-controller that it must block reconciliation unless the HelmChart artifact revision matches the desired chart version?