Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCIRepositoryReconciler no-op improvements #917

Merged
merged 2 commits into from
Sep 29, 2022

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Sep 27, 2022

#913 introduced optimizations to OCIRepository reconciliation by skipping image pull when the artifact revision and the remote image revisions are the same. And it also introduced .spec.layerSelector which contributes to how the artifact is built.
Layer selector and the source ignore rules defined in the spec are ignored by the optimization as described in #913 (comment).

This change introduces content config checksum in the status fields to be able to detect any change in the source configurations and trigger an artifact rebuild if needed. It introduces .status.contentConfigChecksum to store the checksum of the observed values that contribute to the source artifact. Source ignore and layer selector values are considered in the checksum at present. Content config checksum along with the revision of the image to determine if the image pull can be skipped.

Also, when building the artifact, an ArchiveFileFilter is passed to the Storage.Archive() to effectively apply the source ignore rules.

Tests have been updated and new tests are added to cover all the changes described above.

NOTE: This change updates the OCIRepository CRD with new fields which should be updated before any testing.

Refer to #724 for similar considerations in no-op clone reconciliations in GitRepository Reconciler.

@darkowlzz darkowlzz added the area/oci OCI related issues and pull requests label Sep 27, 2022
@darkowlzz darkowlzz force-pushed the improv-ocirepo-optimized-reconcile branch from 79fad1b to 459f954 Compare September 27, 2022 01:34
// ObservedIgnore is the observed exclusion patterns used for constructing
// the source artifact.
// +optional
ObservedIgnore *string `json:"observedIgnore,omitempty"`
Copy link
Contributor Author

@darkowlzz darkowlzz Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears like this:

  observedGeneration: 10
  observedIgnore: |
    hpa.yaml
    foo.txt
  observedLayerSelector:
    mediaType: application/vnd.docker.image.rootfs.diff.tar.gzip
    operation: extract
  url: http://source-controller.flux-system.svc.cluster.local./ocirepository/default/podinfo-layer/latest.tar.gz

Not sure if it's okay or if we should convert it to a string containing the checksum of the ignore value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are disadvantages to this:

  1. The user now needs to be aware of the mechanics of .generation and .status.observedGeneration to be able to assume that the checksum will match the current .spec elements, over just looking at a reflection of plain values.
  2. By combining the values into a single checksum, you lose the ability to selectively notice changes to a sub-set of the spec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll end up with 99% of the spec reflected into the status sub-resource which feels very wrong to me. IMO we should be reconciling on generation drift, but in SC we decided to skip reconciliation when interval changes, which got us to this point where we have to track every other filed. I'm for triggering a reconciliation on spec changes, is not like interval will change so often.

Copy link
Contributor Author

@darkowlzz darkowlzz Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I"ve updated the implementation to add content config checksum in the status. It looks like the following:

  contentConfigChecksum: sha256:e5be314a56171026ffe3b828fec184d6144fb602511ee7243d02193e26114c45
  observedGeneration: 6
  url: http://source-controller.flux-system.svc.cluster.local./ocirepository/default/podinfo/latest.tar.gz

Based on some private discussions, we are adding content config checksum here for now to align with the GitRepository API.
In a future release, we'll replace this checksum with more explicit observed values in the status to improve the usability by looking at the status and easily seeing the differences in the declared state and the observations made by the controller.

@darkowlzz darkowlzz force-pushed the improv-ocirepo-optimized-reconcile branch from 459f954 to cc7e317 Compare September 27, 2022 01:43
@darkowlzz darkowlzz force-pushed the improv-ocirepo-optimized-reconcile branch from cc7e317 to 6fe1af8 Compare September 28, 2022 15:18
@darkowlzz darkowlzz force-pushed the improv-ocirepo-optimized-reconcile branch from 885f962 to 5fdd26b Compare September 28, 2022 15:47
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @darkowlzz 🏅

Introduce contentConfigChecksum in the OCIRepository status to store a
checksum of the values that affect the source artifact. It is used to
detect when to rebuild an artifact when the spec changes.

The considerations for this are similar to the GitRepository
reconciler no-op clone implementation. Both reconcileSource and
reconcileArtifact need to consider the source configuration change
when deciding if the artifact in the storage is up-to-date.

Adds tests for reconcileSource and reconcileArtifact for the noop
cases.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
This implements source ignore in OCIRepositoryReconcilers'
reconcileArtifact so that the ignore rules are considered when building
the artifact.

Adds tests based on the artifact checksum change when ignore rules are
applied.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@stefanprodan stefanprodan force-pushed the improv-ocirepo-optimized-reconcile branch from 8434429 to dcd0db4 Compare September 29, 2022 06:48
@stefanprodan stefanprodan merged commit 95cbf40 into main Sep 29, 2022
@stefanprodan stefanprodan deleted the improv-ocirepo-optimized-reconcile branch September 29, 2022 07:39
@pjbgf pjbgf added this to the GA milestone Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oci OCI related issues and pull requests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants