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

Commit

Permalink
Cache the generators of patchUpdated configurations
Browse files Browse the repository at this point in the history
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 authored and squaremo committed Apr 1, 2020
1 parent 6377745 commit 6af82a6
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 6af82a6

Please sign in to comment.