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

Introduce RFC to correlate resource inputs with output #799

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

idoru
Copy link
Contributor

@idoru idoru commented Apr 8, 2022

Readable

closes #744

PR Checklist

Note: Please do not remove items. Mark items as done [x] or use strikethrough if you believe they are not relevant

  • Linked to a relevant issue. Eg: Fixes #123 or Updates #123
  • Removed non-atomic or wip commits
  • Filled in the Release Note section above
  • Modified the docs to match changes

@netlify
Copy link

netlify bot commented Apr 8, 2022

Deploy Preview for elated-stonebraker-105904 canceled.

Name Link
🔨 Latest commit 84460de
🔍 Latest deploy log https://app.netlify.com/sites/elated-stonebraker-105904/deploys/62557cd877b94900090aa5a6

@idoru idoru force-pushed the 744-input-output-correlation branch 3 times, most recently from 2abe072 to f30295f Compare April 8, 2022 12:44
@idoru idoru force-pushed the 744-input-output-correlation branch from f30295f to 9ec9ef2 Compare April 8, 2022 12:54
against the `spec`, Cartographer must also ensure that the current output is a result of processing the current spec by
making additional assertions:

* Resource is healthy, as per [Allow Resources to Report Status](https://github.com/vmware-tanzu/cartographer/pull/738)
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't healthy/unhealthy be a concern for resources where the observedMatch is to a field in the status (as well as for resources where the observedMatch is to a field in the spec).

Copy link
Contributor Author

@idoru idoru Apr 12, 2022

Choose a reason for hiding this comment

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

The healthy check only exists when the spec is implicated because we're relying on fields in the spec corresponding to the status.

Once the output is in the status, it is true that the resource may reconcile again and be unhealthy, but as long as the exact inputs specified in correlation rules match the output we can reasonably assume it is the same input.

This, as #drawbacks states is the reason why the template authors must take great care. The matching rules must uniquely identify the instance of the input (e.g. as with URL+revision for a source)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I take that back. I think you're right, otherwise we may not always be able to correlate inputs and outputs.

failure and unknown states - which map to Healthy, Unhealthy and Unknown if the mechanism to be relied upon is
[Allow Resources to Report Status](https://github.com/vmware-tanzu/cartographer/pull/738).

# Drawbacks
Copy link
Contributor

Choose a reason for hiding this comment

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

Supply chains often rely on resources that continually report the latest good output (e.g. kpack). When such an object is processing new input, Cartographer is still able to read the output and use it to assert the proper shape of the following object.

WIth this proposal, Cartographer will need to adopt a caching strategy after reading good outputs of objects. Consider the case of a supply chain where resource B relies on resource A. When resource A has a good output, it will at some point get an update to its spec. It will begin process the new spec and Healthy will become unknown. As a result, Cartographer will no longer read resource A's output. That means that Cartographer will now be unable to assure that resource B has the proper spec, so resource B will be unreadable as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. A cache, that is, or something like lastGoodOtput on the status in your original RFC?

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the cache stored? Assume that the controller pod will crash or be rescheduled at an inconvenient time; a new controller instance needs to be able to pickup and continue processing as if nothing happened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be very convenient to put it on the owner status, but this may cause us to exceed maximum size. One thing we've discussed privately within the team is storing outputs as another custom resource. The latter feels like it may be the only choice, barring the addition of another component for durable cache storage.

@waciumawanjohi waciumawanjohi added the rfc Requests For Comment label Apr 8, 2022
rfc/rfc-0000-input-output-correlation.md Outdated Show resolved Hide resolved
rfc/rfc-0000-input-output-correlation.md Show resolved Hide resolved
rfc/rfc-0000-input-output-correlation.md Show resolved Hide resolved
failure and unknown states - which map to Healthy, Unhealthy and Unknown if the mechanism to be relied upon is
[Allow Resources to Report Status](https://github.com/vmware-tanzu/cartographer/pull/738).

# Drawbacks
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the cache stored? Assume that the controller pod will crash or be rescheduled at an inconvenient time; a new controller instance needs to be able to pickup and continue processing as if nothing happened.

idoru and others added 5 commits April 18, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Requests For Comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

R4RFC: Artifact tracing
3 participants