From 16ee16831b1fc338519ebcccbb62d3b804c7a42b Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Fri, 31 Jan 2020 01:50:13 +0100 Subject: [PATCH] Cache the result of the generator commands of patchUpdated configurations This change assumes that the generator commands of `patchUpdated` configurations have a constant output throughout the execution of an synchronization/policy update/image release operation. This was already assumed, to certain extent, by `patchUpdated`'s implementation since, for each workload/container update we run the `generators` in order to refresh the `patchFile`. In practice, this change caches the result of the generators throughout the lifespan of the `ConfigFile` variable (which, currently, is the execution of a synchronization, policy update or release operation). However, I'm not entirely happy about this, since the lifespan of `ConfigFile` may change in the future. I would rather explicitly track what the cache depends on but: * Using cache keys is probably not enough (Maybe use the Git Hash of the filesystem? But what if there are file modifications? and what about external network dependencies?). * `ConfigFile` is decoupled from the git repository implementation I would like it remain this way. So I would rather not invoke git commands in the `ConfigFile` implementation. * Alternatively to cache keys we could bookkeep the cache outside of `ConfigFile` but that's not easy since: 1. We are not caching the final manifests, just the ones returned by the generators (which in `patchUpdated` isn't the same) which are not accessible external. 2. The `Config` file is a few layers under the `manifests.Store` Maybe I will find a better solution later on. Anyways, this cache is effective because update/release operations are subdivided into individual workload/container operations. This smaller operations can be numerous, and each of them requires obtaining the result of the `generators` execution to recompute the `patchFile`. On top of that, every image release operation has a verification stage, which also requires the `generators` result. It's worth noting that we cannot do the same sort of caching for `commandUpdated` configurations, since the `updater` invocations are supposed to cause side-effects in the `generators` result. If it's of any consolation, `commandUpdated` configurations don't invoke the `generators` for updates, but, we do call the `updaters` which may be doing that under the hood. The conclusion is that `commandUpdated` configuarions should do the caching themselves. --- pkg/daemon/daemon.go | 2 +- pkg/manifests/configfile.go | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index a3b57d59d7..fd6b8346ac 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -411,7 +411,7 @@ func (d *Daemon) updatePolicies(spec update.Spec, updates resource.PolicyUpdates Status: update.ReleaseStatusSkipped, } } - if policy.Set(u.Add).Has(policy.Automated) { + if u.Add.Has(policy.Automated) { anythingAutomated = true } cm, err := d.getManifestStore(working) diff --git a/pkg/manifests/configfile.go b/pkg/manifests/configfile.go index c230468764..0ad035a788 100644 --- a/pkg/manifests/configfile.go +++ b/pkg/manifests/configfile.go @@ -141,8 +141,9 @@ type PolicyUpdater struct { // maintaining a patch, which is calculating from, and applied to, the // generated manifests. type PatchUpdated struct { - Generators []Generator `json:"generators"` - PatchFile string `json:"patchFile,omitempty"` + Generators []Generator `json:"generators"` + PatchFile string `json:"patchFile,omitempty"` + generatorsResultCache []byte } // ScanForFiles represents a config in which the directory should be @@ -324,11 +325,15 @@ type ConfigFileCombinedExecResult struct { // getGeneratedAndPatchedManifests is used to generate manifests when // the config is patchUpdated. func (cf *ConfigFile) getGeneratedAndPatchedManifests(ctx context.Context, manifests Manifests) ([]byte, []byte, string, error) { - generatedManifests, err := cf.getGeneratedManifests(ctx, manifests, cf.PatchUpdated.Generators) - if err != nil { - return nil, nil, "", err + generatedManifests := cf.PatchUpdated.generatorsResultCache + if generatedManifests == nil { + var err error + generatedManifests, err = cf.getGeneratedManifests(ctx, manifests, cf.PatchUpdated.Generators) + if err != nil { + return nil, nil, "", err + } + cf.PatchUpdated.generatorsResultCache = generatedManifests } - // The patch file is given in the config file as a path relative // to the working directory relPatchFilePath := cf.PatchUpdated.PatchFile