Introduce Simplify Runnable Usage RFC #705
Conversation
|
✔️ Deploy Preview for elated-stonebraker-105904 canceled. 🔨 Explore the source changes: 40c3cc8 🔍 Inspect the deploy log: https://app.netlify.com/sites/elated-stonebraker-105904/deploys/622bc3672de2f000085d4918 |
| .spec.selector field. Should we enable users that are leveraging this | ||
| behavior to use the simplification specified in this RFC? |
There was a problem hiding this comment.
Should we enable users that are leveraging this behavior to use the simplification specified in this RFC?
Not until regular supply chain templates can also select and read values from arbitrarily selected resources.
Co-authored-by: Scott Andrews <[email protected]>
| ## Inputs | ||
| The templated object will have two types of fields: hard-coded and templated. | ||
| Hard coded fields can simply be copied directly into the ClusterRunTemplate. | ||
| Templated fields will be translated. Rather than reading from the specified |
There was a problem hiding this comment.
In order for this to take place, I imagine we have to limit the scope to jsonpath-based templating as we couldn't do that for ytt, is that right?
There was a problem hiding this comment.
Yes, that's a good call out. I think that is fine, as the goal of this RFC is to provide a convenience method for easy cases, not to replace the direct usage of Runnable and CRT in all cases.
|
|
||
| ## Names | ||
| The runnable will be created with the same name assigned to the templated | ||
| object with the object kind appended. |
There was a problem hiding this comment.
pretty interesting - I think this has been discussed elsewhere, but, wdyt of using metadata.generateName? that way we can be sure that the name we got is not conflicting with anything in that namespace
There was a problem hiding this comment.
I'm not opposed to using generate name for the runnable. I expect it will require slightly more processing by our workload, as we would need to list all the runnables in the namespace and filter for the one that we want.
|
|
||
| What is meant by "ensure that a matching ClusterRunTemplate exists"? For any | ||
| given supply chain template (a cluster scoped object) there will only need | ||
| to be one ClusterRunTemplate. All of the N Runnables created by N workloads |
There was a problem hiding this comment.
I wonder if we're getting into a non crucial optimization here - as ClusterRunTemplate objects just carry data, I wonder if it'd be enough to just always create one for a given runnable (then be able to rely on generateName for these). wdyt?
There was a problem hiding this comment.
For every ClusterSourceTemplate that stamps out an immutable object, there is one logical ClusterRunTemplate. That ClusterRunTemplate is determinative (it uses no information from the workload). I don't think we should create N ClusterRunTemplates which are all identical except for their name, when instead we could create just one.
There was a problem hiding this comment.
Ensuring that an expected object exists is also standard Carto behavior. For every workload, the controller ensures that the expected objects have been created.
There was a problem hiding this comment.
In some discussions the concern for a race condition has been raised. I do not believe that is a concern. In the worst case scenario a user defines a ClusterSourceTemplate with an immutable lifecycle. This template is then referenced by both a delivery and a supply chain. A reconciliation is kicked off for both at the same time. Both check for the existence of the expected ClusterRunTemplate. Neither see it. Both submit the same ClusterRunTemplate with the same name (because it is based on the name of the ClusterSourceTemplate). One submission is handled first (and creates the object), the other second (and updates the object, with no changes). No conflict exists.
I welcome further poking of this, but I do not see a concern about name collisions or race conditions.
| [how-it-works]: #how-it-works | ||
|
|
||
| When Cartographer processes a template with the immutable flag, rather than | ||
| creating/updating the templated object it will create/update a Runnable and |
There was a problem hiding this comment.
have you thought about how this would impact workload.status.resources[*].stampedRef? I wonder if it'd be strange for someone to see in their templates that they're stamping out kind: PipelineRun and then they see Runnable there.
I particularly really like the fact that this proposal keeps that approach of the direct children being all level triggered (via the use of Runnable) though
There was a problem hiding this comment.
I discuss the .status.resources here:
https://github.com/vmware-tanzu/cartographer/blob/rfc-simplify-runnable-usage/rfc/rfc-0000-simplify-runnable-use.md#resource-reporting-on-workload
I'll note that in Office Hours Rash advocated for not creating a Runnable, and rather moving this functionality into the workload controller's code:
https://youtube.com/clip/Ugkx0s1Qy_CEr7aG2QWaiMQK1N01PRjCXNAu
I am fully amenable to that change if there is a member of the technical oversight committee that will approve that change.
|
marking as blocked as we'd like to tackle #787 first |
Readable
Release Note
PR Checklist
Note: Please do not remove items. Mark items as done
[x]or usestrikethroughif you believe they are not relevant[ ] Linked to a relevant issue. Eg:Fixes #123orUpdates #123wipcommits[ ] Filled in the Release Note section above[ ] Modified the docs to match changes