From 0f34784fa396fe197635f1f37d4bd90a1ee3da9a Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 5 Apr 2019 19:05:19 +0200 Subject: [PATCH] Move HelmRelease mutation prevention to operator Before this change there was a ticker in the ChartChangeSync loop which 'rereleased' all HelmReleases on the set interval, to prevent mutations due to e.g. manual chart releases. This commit moves this logic to the operator by utilizing the resync interval that can be set on the shared informer. On every resync interval, all objects known at that time to the informer are redelivered as 'updates', causing the same effect as the ticker process described above, but with several improvements. 1. The syncHandler validates if a HelmRelease still exists before attempting to run the release. This is an improvement compared to the blind run we did before, where in case a user had many HelmReleases, the Helm release could be removed at the time the release attempt was made. 2. We now act on information from one source (except for the `reconcileReleaseDef()` call on repository changes, see added comment). --- cmd/helm-operator/main.go | 4 +-- integrations/helm/chartsync/chartsync.go | 43 ++++-------------------- integrations/helm/operator/operator.go | 14 ++++---- 3 files changed, 14 insertions(+), 47 deletions(-) diff --git a/cmd/helm-operator/main.go b/cmd/helm-operator/main.go index 80ce7fb79..e3df2f11d 100644 --- a/cmd/helm-operator/main.go +++ b/cmd/helm-operator/main.go @@ -171,17 +171,15 @@ func main() { rel := release.New(log.With(logger, "component", "release"), helmClient) chartSync := chartsync.New( log.With(logger, "component", "chartsync"), - chartsync.Polling{Interval: *chartsSyncInterval}, chartsync.Clients{KubeClient: *kubeClient, IfClient: *ifClient}, rel, chartsync.Config{LogDiffs: *logReleaseDiffs, UpdateDeps: *updateDependencies, GitTimeout: *gitTimeout}, *namespace, - statusUpdater, ) chartSync.Run(shutdown, errc, shutdownWg) nsOpt := ifinformers.WithNamespace(*namespace) - ifInformerFactory := ifinformers.NewSharedInformerFactoryWithOptions(ifClient, 30*time.Second, nsOpt) + ifInformerFactory := ifinformers.NewSharedInformerFactoryWithOptions(ifClient, *chartsSyncInterval, nsOpt) fhrInformer := ifInformerFactory.Flux().V1beta1().HelmReleases() // start FluxRelease informer diff --git a/integrations/helm/chartsync/chartsync.go b/integrations/helm/chartsync/chartsync.go index 48f68e296..9a75136ac 100644 --- a/integrations/helm/chartsync/chartsync.go +++ b/integrations/helm/chartsync/chartsync.go @@ -75,10 +75,6 @@ const ( ReasonSuccess = "HelmSuccess" ) -type Polling struct { - Interval time.Duration -} - type Clients struct { KubeClient kubernetes.Clientset IfClient ifclientset.Clientset @@ -106,7 +102,6 @@ type clone struct { } type ChartChangeSync struct { - Polling logger log.Logger kubeClient kubernetes.Clientset ifClient ifclientset.Clientset @@ -121,10 +116,9 @@ type ChartChangeSync struct { namespace string } -func New(logger log.Logger, polling Polling, clients Clients, release *release.Release, config Config, namespace string, statusUpdater *status.Updater) *ChartChangeSync { +func New(logger log.Logger, clients Clients, release *release.Release, config Config, namespace string) *ChartChangeSync { return &ChartChangeSync{ logger: logger, - Polling: polling, kubeClient: clients.KubeClient, ifClient: clients.IfClient, release: release, @@ -148,9 +142,6 @@ func (chs *ChartChangeSync) Run(stopCh <-chan struct{}, errc chan error, wg *syn wg.Done() }() - ticker := time.NewTicker(chs.Polling.Interval) - defer ticker.Stop() - for { select { case reposChanged := <-chs.mirrors.Changes(): @@ -239,18 +230,14 @@ func (chs *ChartChangeSync) Run(stopCh <-chan struct{}, errc chan error, wg *syn } } + // TODO(hidde): if we could somehow signal the + // operator an 'update' has happened we can control + // everything through the queue it maintains... + // Annotating the FHR could be an option as those + // count as updates but I am not sure if Flux would + // see those as in-cluster mutations. chs.reconcileReleaseDef(fhr) } - case <-ticker.C: - // Re-release any chart releases that have apparently - // changed in the cluster. - chs.logger.Log("info", fmt.Sprint("Start of releasesync")) - err := chs.reapplyReleaseDefs() - if err != nil { - chs.logger.Log("error", fmt.Sprintf("Failure to do manual release sync: %s", err)) - } - chs.logger.Log("info", fmt.Sprint("End of releasesync")) - case <-stopCh: chs.logger.Log("stopping", "true") return @@ -383,22 +370,6 @@ func (chs *ChartChangeSync) reconcileReleaseDef(fhr fluxv1beta1.HelmRelease) { } } -// reapplyReleaseDefs goes through the resource definitions and -// reconciles them with Helm releases. This is a "backstop" for the -// other sync processes, to cover the case of a release being changed -// out-of-band (e.g., by someone using `helm upgrade`). -func (chs *ChartChangeSync) reapplyReleaseDefs() error { - resources, err := chs.getCustomResources() - if err != nil { - return fmt.Errorf("failed to get HelmRelease resources from the API server: %s", err.Error()) - } - - for _, fhr := range resources { - chs.reconcileReleaseDef(fhr) - } - return nil -} - // DeleteRelease deletes the helm release associated with a // HelmRelease. This exists mainly so that the operator code can // call it when it is handling a resource deletion. diff --git a/integrations/helm/operator/operator.go b/integrations/helm/operator/operator.go index 22f1cdddb..921f6de0e 100644 --- a/integrations/helm/operator/operator.go +++ b/integrations/helm/operator/operator.go @@ -297,15 +297,13 @@ func (c *Controller) enqueueUpdateJob(old, new interface{}) { return } - if diff := cmp.Diff(oldFhr.Spec, newFhr.Spec); diff != "" { - c.logger.Log("info", "UPGRADING release") - if c.logDiffs { - c.logger.Log("info", "Custom Resource driven release upgrade", "diff", diff) - } else { - c.logger.Log("info", "Custom Resource driven release upgrade") - } - c.enqueueJob(new) + c.logger.Log("info", "UPGRADING release") + if diff := cmp.Diff(oldFhr.Spec, newFhr.Spec); diff != "" && c.logDiffs { + c.logger.Log("info", "Custom Resource driven release upgrade", "diff", diff) + } else { + c.logger.Log("info", "Custom Resource driven release upgrade") } + c.enqueueJob(new) } func (c *Controller) deleteRelease(fhr flux_v1beta1.HelmRelease) {