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

Add explicit observed artifact content configurations in status #926

Merged
merged 3 commits into from
Oct 11, 2022

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Oct 3, 2022

GitRepositoryReconciler and OCIRepositoryReconcilers support no-op reconciliation which makes these reconcilers efficient in their operations. The no-op reconciliation is determined by taking into considerations all the variables that affect the source artifact content. In #724 and #917, the values of these variables were computed as a checksum and stored in the status in a field called the content config checksum. Although it worked, the checksum of multiple variables wasn't friendly to debug or see the observations of the controller. This change replaces the content config checksum with explicit status fields for the observed variables that contribute to the source artifact content. The content config checksum remains in the status API for now, but is not used and unset for any existing resources. It'll be removed in the next API version.

In GitRepository source, three new status fields observedIgnore, observedRecurseSubmodules and observedInclude are introduced. https://gist.github.com/darkowlzz/3ffc1000754580a0fac6f9068e06643e shows the status with these new fields under some relevant scenarios.

In OCIRepository source, two new status fields observedIgnore and observedLayerSelector are introduced. https://gist.github.com/darkowlzz/ece8718c5ed195948d1c8e2f4ea2f941 shows the status with these new fields under some relevant scenarios.

Spec docs and tests have been updated for these changes.

@darkowlzz darkowlzz force-pushed the status-observed-source-config branch from 3fd81d0 to c5a1996 Compare October 4, 2022 10:28
@darkowlzz darkowlzz changed the title Add explicit observed source configurations in status Add explicit observed artifact content configurations in status Oct 4, 2022
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.

Can you please add observedIgnore to the Bucket status also, it would be good to have consistency across all source APIs.

@darkowlzz darkowlzz force-pushed the status-observed-source-config branch from c5a1996 to 4715b2e Compare October 10, 2022 14:07
@darkowlzz
Copy link
Contributor Author

Can you please add observedIgnore to the Bucket status also, it would be good to have consistency across all source APIs.

Added status.observedIgnore in bucket. Looks like the same as others.
Just for reference:

status:
  artifact:
    checksum: 1dbc7991adb507aaf62cb8ac0d973b2bd5929c0642632d66ec8b594ed277064a
    lastUpdateTime: "2022-10-10T14:01:44Z"
    path: bucket/default/foo/e271cae427bae7c073844cacafe6ef05ec2e064c69f9e7e06abab1fe849bf994.tar.gz
    revision: e271cae427bae7c073844cacafe6ef05ec2e064c69f9e7e06abab1fe849bf994
    size: 173
    url: http://localhost:9090/bucket/default/foo/e271cae427bae7c073844cacafe6ef05ec2e064c69f9e7e06abab1fe849bf994.tar.gz
  conditions:
  - lastTransitionTime: "2022-10-10T14:01:44Z"
    message: stored artifact for revision 'e271cae427bae7c073844cacafe6ef05ec2e064c69f9e7e06abab1fe849bf994'
    observedGeneration: 3
    reason: Succeeded
    status: "True"
    type: Ready
  - lastTransitionTime: "2022-10-10T14:01:44Z"
    message: stored artifact for revision 'e271cae427bae7c073844cacafe6ef05ec2e064c69f9e7e06abab1fe849bf994'
    observedGeneration: 3
    reason: Succeeded
    status: "True"
    type: ArtifactInStorage
  observedGeneration: 3
  observedIgnore: |
    /deploy/**/*.md
    /deploy/**/*.txt
  url: http://localhost:9090/bucket/default/foo/latest.tar.gz

@stefanprodan
Copy link
Member

@darkowlzz with the Bucket changes in this PR we can close #712 right?

@darkowlzz
Copy link
Contributor Author

with the Bucket changes in this PR we can close #712 right?

@stefanprodan IIRC, I think I heard that PR was abandoned because the contributor later found out that what they were trying was working fine for buckets. But based on my experience and understanding of it, I don't think it'll rebuild the artifact. We need to do similar artifact content config checks that we do in this PR for GitRepo and OCIRepo.
That PR can be closed, but the issue remains.

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

Small nit, otherwise LGTM.

controllers/gitrepository_controller.go Outdated Show resolved Hide resolved
@stefanprodan
Copy link
Member

We need to do similar artifact content config checks that we do in this PR for GitRepo and OCIRepo.

Let's do it in this PR.

@darkowlzz
Copy link
Contributor Author

Let's do it in this PR.

I tested it and looks like it does work but in a different way. Unlike other sources where we download all the content and then apply the ignore rules, in case of bucket, the ignore rule is applied in the etag fetcher itself and an index is calculated based on the etag values of the files. Files that are ignored are not included in the index computation, resulting in a different index value due to ignore value. Due to this, an ineffective ignore rule doesn't result in rebuilding of the artifact, but an effective rule does. Seems like the correct behavior.
With that #712 can be closed. That issue originated from GitRepo not respecting ignore change but that's been fixed already.

Replace content config checksum with explicit artifact content config
observations. It makes the observations of the controller more
transparent and easier to debug.

Introduces `observedIgnore` and `observedLayerSelector` status fields.

Signed-off-by: Sunny <[email protected]>
Replace content config checksum with explicit artifact content config
observations. It makes the observations of the controller more
transparent and easier to debug.

Introduces `observedIgnore`, `observedRecurseSubmodules` and
`observedInclude` status fields.

Signed-off-by: Sunny <[email protected]>
Introduce status.observedIgnore in the Bucket API for consistency with
other sources with ignore.

Signed-off-by: Sunny <[email protected]>
@darkowlzz darkowlzz force-pushed the status-observed-source-config branch from 4715b2e to a6d7948 Compare October 10, 2022 17:36
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 🏅

Copy link
Member

@pjbgf pjbgf 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 ☀️!

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Looks great to me as well! 🙇

@stefanprodan stefanprodan merged commit 5e83eca into main Oct 11, 2022
@stefanprodan stefanprodan deleted the status-observed-source-config branch October 11, 2022 10:46
@pjbgf pjbgf added this to the GA milestone Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants