-
Notifications
You must be signed in to change notification settings - Fork 190
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
Conversation
79fad1b
to
459f954
Compare
api/v1beta2/ocirepository_types.go
Outdated
// ObservedIgnore is the observed exclusion patterns used for constructing | ||
// the source artifact. | ||
// +optional | ||
ObservedIgnore *string `json:"observedIgnore,omitempty"` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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:
- 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. - By combining the values into a single checksum, you lose the ability to selectively notice changes to a sub-set of the spec.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
459f954
to
cc7e317
Compare
cc7e317
to
6fe1af8
Compare
885f962
to
5fdd26b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @darkowlzz 🏅
5fdd26b
to
8434429
Compare
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>
8434429
to
dcd0db4
Compare
#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 theStorage.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.