Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Commit

Permalink
Cache the result of the generator commands of patchUpdated configurat…
Browse files Browse the repository at this point in the history
…ions

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.
  • Loading branch information
2opremio committed Jan 31, 2020
1 parent 0810999 commit 16ee168
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 7 deletions.
2 changes: 1 addition & 1 deletion pkg/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
17 changes: 11 additions & 6 deletions pkg/manifests/configfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 16ee168

Please sign in to comment.