WIP Refactor ref resolution #4108
Conversation
ghost
added
the
kind/cleanup
ghost
requested review from
tekton-robot
added
do-not-merge/work-in-progress
tekton-robot
added
the
size/XL
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
This means that if I were to checkout the 1st commit, the controller wouldn't reconcile any new PipelineRun/TaskRun right ? |
👍🏼 on this. If we feel it's not ready for external consumption yet, we could always hide it behind the |
| newPR, err := r.pipelineClientSet.TektonV1beta1().PipelineRuns(pr.Namespace).Update(ctx, pr, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| err = fmt.Errorf("error committing updated pipeline spec to pipelinerun %s/%s: %w", namespace, name, err) | ||
| pr.Status.MarkFailed(resolution.ReasonPipelineRunResolutionFailed, err.Error()) | ||
| logger.Error(err) | ||
| return fmt.Errorf("error updating pipelinerun spec: %w", err) | ||
| } |
There was a problem hiding this comment.
Any reason to do an update instead of a patch ? I think it could this result in a weird race condition (if there is a mutating webhook doing something with PipelineRun in the meantime).
There was a problem hiding this comment.
Very interesting! I hadn't thought about the mutating webhook case. I'll try this with a patch operation instead.
There was a problem hiding this comment.
As far as I can tell our test clients don't support Patch() operations on resources. I can update the resolver reconcilers to use Patch instead of Update but the unit tests will rely on a workaround that uses Update still. This means we're not exercising the exact same resolution behaviour between unit tests and release. I'm hopeful integration will work without any such workarounds though. @vdemeester are you ok with this tradeoff?
There was a problem hiding this comment.
I've updated the pkg/reconciler/taskrun/taskrun_test.go file so that the tests pass. Changes aren't perfect yet (in particular - it looks like the number of events emitted has changed between main and my branch) but it shows how the resolver updates in unit tests use Update() while the resolver reconciler uses Patch()
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
@vdemeester You're absolutely right - checking out the first commit results in no taskruns or pipelineruns being reconciled. They remain fixed in limbo. I like your idea of keeping |
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
tekton-robot
added
size/XXL
|
The following is the coverage report on the affected files.
|
|
Moved the library funcs under the |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test tekton-pipeline-unit-tests /test pull-tekton-pipeline-alpha-integration-tests |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
This commit introduces two new experimental reconcilers to Pipelines' codebase. These reconcilers perform "resolution" of taskRefs and pipelineRefs. When they're enabled the taskrun and pipelinerun reconcilers no longer attempt to resolve taskRefs/pipelineRefs themselves and instead they'll wait for the `status.taskSpec` and `status.pipelineSpec` to be populated before they proceed with actuating the TaskRun/PipelineRun. There are two new flags introduced with this commit: 1. A behavioural feature flag (`experimental-disable-ref-resolution`) that switches off the TaskRun and PipelineRun reconciler resolution processes. When this flag is to "true" TaskRuns and PipelineRuns will be left in a transitional state until their `status` is populated with a `taskSpec`/`pipelineSpec`. This allows an external process (like the resolver reconcilers, or a third-party controller) to perform this resolution work instead and update the resources with the specs. When this flag is "false" (the default) then resolution happens just as it always did in the TaskRun / PipelineRun reconcilers. In this way it's totally backwards compatible. 2. A command line flag that must be passed to the controller binary via an arg (`-experimental-enable-resolution-reconcilers`) which switches on the experimental "resolution reconcilers". When this flag is passed the controller will spin up two additional reconcilers when it boots up. This means a restart of the controller is required when changing this flag. Both of these modes are extremely experimental.
|
The following is the coverage report on the affected files.
|
|
I've decided to start again with a completely different approach to this refactor and will open a new PR with a clean slate. See #4168 |
ghost
closed this
Changes
Fixes #3305
This PR refactors the way task and pipelinerun resolution works. The intention with this PR is to be entirely backwards-compatible with the existing way resolution works. In other words, I'm not trying to introduce new behaviours with these commits, just set the stage for future possible improvements.
I'm pushing this up now to start getting feedback from interested parties. I've still got a ways to go in terms of getting new unit tests updated and introduced. I'm hopeful that integration tests shouldn't be too badly affected. 🤞
There are two commits as part of this PR to aid reviewers:
I'll squash these once we're happy to move ahead.
Prior to this commit the taskrun and pipelinerun reconcilers were responsible for resolving specs from taskRef / taskSpec / pipelineRef / pipelineSpec / bundles.
The first commit in this PR updates the existing taskrun/pipelinerun reconcilers to ignore any taskrun or pipelinerun that doesn't have a spec cached in their status. This means all taskruns and pipelineruns are ignored - the logic for populating status from a spec has been removed entirely from the controller.
The second commit in this PR introduces 2 new controllers whose sole responsibility is to resolve specs from taskruns and pipelineruns. Resolved specs then get written to the status field of the runs and the "old" reconcilers are able to pick them up and begin actuating them as usual.
The new resolver reconcilers are also currently responsible for propagating labels and annotations from the task / pipeline to the taskrun / pipelinerun. This is because our existing taskrun/pipelinerun reconcilers expect only the spec to be embedded in their status fields, not the entire task/pipeline objects. The knock-on effect of this is that the metadata from the resolved task/pipeline is lost by the time the taskrun/pipelinerun reconciler receive them.
One final thing this commit introduces is a library,
internal/resolutionthat may eventually be available for downstream projects to implement their own task and pipeline resolvers. At the moment all it does is expose the default resolution behaviour of Tekton Pipelines as a func + a couple of useful helper methods for propagating labels/annotations. This would allow someone implementing their own resolver reconciler to generally adhere to the Tekton Pipelines "resolution contract" with only 100 lines or so of Go. It's not yet intended for external consumption but I think it's an interesting direction we can take future work on tekton's built-in resolution code and provides a nice enough separation of concerns in our own codebase.Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes