Decouple release reconciliation from chart source sync #99

hiddeco · 2019-11-08T13:28:37Z

This PR decouples the synchronization of charts from git sources from the synchronization of HelmRelease resources to Helm releases and/or cluster resources.

The synchronization of charts from Git sources has been rewritten so that we only do bookkeeping of git references for HelmRelease resources, but no longer maintain a working directory, this copy is instead made on request based on the references we have in our books at that moment.

The release synchronization is moved to the release package, and has been rewritten so that implementers no longer have to decide what actions they want to perform, they can simply call Sync (install or upgrade on creation and/or update of HelmRelease resource) or Uninstall (uninstall on delete of HelmRelease resource).

Fixes #98

pkg/chartsync/download.go

pkg/chartsync/git.go

pkg/release/release.go

pkg/chartsync/git.go

2opremio · 2019-11-18T18:15:41Z

pkg/chartsync/git.go

+// indicates whether the repo was already present (`true` if so,
+// `false` otherwise).
+func (c *GitChartSync) maybeMirror(mirrorName string, remote string) bool {
+	ok := c.mirrors.Mirror(


Maybe this isn't a new problem but, I only see a StopOne() counterpart to this Mirror() call in Delete() which seems to only be invoked when a HelmRelease() is deleted.

Doesn't this mean that there will be a mirror leak if the git entry of a HelmRelease is changed (or removed)?

Additionally, Delete() doesn't help once the git field is modified, since it uses helmReleasesForMirror() which relies in live data, and not the mirrors created by the HelmRelease()

If we don't want to track changes on thegit field explicitly, I think this can be fixed by periodically comparing the output of helmReleasesForMirror() on c.mirrors. I don't think it's ideal though.

Now I see that forHelmRelease() is used in get() to take into account that the repo of HelmReleases can change with respect to what's book-kept in GitChartSync, but I don't think we do anything to correct divergences.

Maybe this isn't a new problem but, I only see a StopOne() counterpart to this Mirror() call in Delete() which seems to only be invoked when a HelmRelease() is deleted.

The current master version does not stop any mirrors, they continue to live forever. I will let your comments simmer in my head for a while.

pkg/chartsync/git.go

2opremio · 2019-11-18T19:45:52Z

pkg/chartsync/git.go

+// record). In case of failure it returns an error.
+func (c *GitChartSync) sync(hr *v1.HelmRelease, mirrorName string, repo *git.Repo) (sourceRef, bool, error) {
+	source := hr.Spec.GitChartSource
+	if source == nil {


It's strange that the method needs to check for the GitChartSource and also provide a mirrorName, which should be a subproduct of the former).

I would change the interface to sync(hrID resource.ID, mirrorName string, repo *git.Repo)

But, that's not currently possible, because sync also consistency-checks that the mirror we have for the Helm release (in get()s invocation to forHelmRelease()).

However, I don't think that consistency-checking is the direct responsibility of sync() (or at least it makes it harder to understand what's going on since it's buried when invoking get()).

I think that we should take car of divergences explicitly, while also garbage collecting orphan mirrors (i.e. https://github.com/fluxcd/helm-operator/pull/99/files#r347533178 )

There is another reason this is not possible, without the v1.HelmRelease or a copy of the v1.GitChartSource, it is not possible to detect changes as done on L286, nor is it possible to construct a new sourceRef as done on L265.

pkg/chartsync/git.go

2opremio · 2019-11-18T20:12:07Z

I think that the implementation has been drastically improved and looks much cleaner 🏅 , particularly when it comes to concurrency

However, this has allowed me to focus on other parts, particularly the consistency of git references fromHelmReleases` which I think can be improved.

hiddeco · 2019-11-19T15:37:36Z

@fons I would like to propose to merge this as is, and tweak the bits around garbage collection and/or the imperfect clean up of mirrors at a later stage. This PR is holding back the developments around Helm v3 - which I would like to focus on the rest of this week, and the current state of the PR is arguably already ten times better as what it was.

We can work out the missing piece of the puzzle in an issue, as we both do not seem to have a direct answer to the question what the best way forward is for this given the triangular relationship of HelmRelease -> sourceRef <- mirror(Signal). I also think having some time off from this code, while focusing on other elements of the operator, will probably lead to a solution faster than being stuck on minor details.

To make Helm v3 a first class citizen, it would be better to mimic the `--atomic` behaviour from Helm v3 in the v2 client code, so that the release is removed automagically if the installation fails. This is however outside the scope of the PR this commit is part of.

The previous metric kept track of the time it would take to perform a Helm action, and some other actions around the synchronization of the `HelmRelease` resource to the cluster, but would not give a real insight in the total amount of time it would take to process a `HelmRelease` resource. This commit slightly tweaks the metric, so that it now measures the total amount of time it takes to synchronize a single `HelmRelease` resource to the cluster, and drops labels that due to this change have become irrelevant.

* remove obsolete `runtime.HandleCrash()` which was still included because of historical reasons * remove unnecessary mutex initialization * flatten return into something more Golang idiomatic

Plus various alignments around naming things.

If the atomic option is set during upgrades, Helm v3 will automatically rollback the release if it fails, which is not something we are after as this can be dangerous for e.g. charts that make use of `StatefulSet` resources. During installation however we _do_ want to use this flag, as this automatically removes a failed release for us, so that we do not have to perform or implement this logic ourselves.

Plus remove the explicit structure and instead return the `*git.Export` and HEAD reference string explicitly.

2opremio · 2019-11-20T10:52:02Z

Sounds good! Can you create a ticket for it?

This commit brings the leakage of mirrors close to zero, as the mirroring will be stopped if there are no references to it from `HelmRelease` resources on the first signal received.

hiddeco added the helm v3 Issue or PR related to Helm v3 label Nov 8, 2019

hiddeco mentioned this pull request Nov 12, 2019

remove replace directives #97

Closed

2opremio reviewed Nov 14, 2019

View reviewed changes

pkg/chartsync/download.go Outdated Show resolved Hide resolved

2opremio reviewed Nov 14, 2019

View reviewed changes