Sync chart mirror on chart spec change to prevent incorrect reconciliation #573
Conversation
amit-handda
force-pushed
the
sync-before-helm-reconcile
branch
from
8028d93
to
0fb087e
Compare
pkg/operator/operator.go
Outdated
| // repo's ref-sha. To avoid spurious deploy, mirror is synced before | ||
| // doing helm reconciliation | ||
| if csDiff := cmp.Diff(oldHr.Spec.ChartSource,newHr.Spec.ChartSource); csDiff != "" { | ||
| c.apiServer.SyncMirrors() |
There was a problem hiding this comment.
I do not see how this is different from what the controller already does, as during the preparation stage GetMirrorCopy is called, which automatically results in an additional sync:
helm-operator/pkg/chartsync/git.go
Line 169 in 90f4946
There was a problem hiding this comment.
thanks for your kind review. IIUC, this sync ^ is between helmrelease sourceref and helm. It assumes Mirror is uptodate and works to reconcile helmrelease.
while c.apiServer.SyncMirrors() <- this one is basically git fetch --tags origin for each Mirror.
this ensures the Mirror is uptodate before doing helmrelease sync. I trigger it only if there is a change in helmrelease.spec.chartsource
Please let me know if I am wrong. I tested this behaviour locally, before creating a PR.
There was a problem hiding this comment.
I think you are indeed correct, what worries me about using SyncMirrors it that it triggers it for each mirror, while the task is just for a single repository. This is depending on the amount of repositories it mirrors, either inefficient or really inefficient.
There was a problem hiding this comment.
yes, it would be inefficient. For us, we are only using single repo.
Before doing that optimization, wanted a goahead from the project authors.
I can try my hand at it.
There was a problem hiding this comment.
You have my blessings to go ahead and change it, but be aware that the efforts will likely have a short lifetime, as we are slowly moving users to the helm-controller.
There was a problem hiding this comment.
yes, thanks. aware of the short lifetime. this fix is important to us since we are using it in production and its impacting our operations. we want to fix it before migrating to v2.
amit-handda
force-pushed
the
sync-before-helm-reconcile
branch
from
1bfb300
to
bf67175
Compare
There was a problem hiding this comment.
LGTM @amit-handda, thank you for following up 🥇
Before I merge, can you please smash your commits into one?
Signed-off-by: amit handa <amit.handa@doordash.com>
amit-handda
force-pushed
the
sync-before-helm-reconcile
branch
from
4692b5e
to
b88c03e
Compare
|
Thanks @hiddeco, its a pleasure. merged the commits. |
hiddeco
changed the title
|
@hiddeco morning ! may I know when a release will be cut from the master ? we would like to use the bug fix in our clusters ASAP. if the release is far away, please let me know if there is any way we could use a release based on master. thanks |
|
@amit-handda there are pre-release builds available at https://hub.docker.com/r/fluxcd/helm-operator-prerelease/tags |
|
thanks @hiddeco. I am running and testing the prerelease into our cluster. I faced repeated I reverted to helm-operator v1.1.0, it upgraded as well (revision 48) once but stopped thereafter. helm-operator v1.1.0 logs helm-release prerelease logs are similar. but hard to pinpoint what triggers the upgrade. I am digging into it. |
|
from logs, it seems helm-operator/pkg/release/release.go Line 148 in 8a3f39f it decides the files are changed and upgrades the deploy. but why repeatedly ? |
|
@hiddeco I think the bug is the following. which leads to repeated upgrades as following: |
|
should I create an issue to track it ? UPDATE: |
|
@amit-handda were you able to debug this further? I have been looking into it as I am planning to prepare a release, but have been unable to reproduce the issue. One thing that comes to mind is that you may have an older version of the CRD applied? This has been the cause of spurious upgrades before... |
|
@hiddeco @amit-handda I can confirm that if you upgrade from 1.1.0 to 1.2.0 without updating your CRDs, and then update a chart, it will update forever without being able to update |
This PR fixes the issue observed in #562
if there is a change in the chartsource (ref change, eg), its possible that the mirror's ref-sha be obsolete w.r.t. upstream repo's ref-sha. To avoid spurious deploy, mirror is synced before doing helm reconciliation.
CHANGELOG :
Bug fixes:
operator: incase of a helmrelease's chartsource update, to prevent incorrect helm reconciliation, sync chart mirror beforehand