Migrate the PipelineRun controller to genreconciler. #2760
Conversation
tekton-robot
added
the
size/L
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
/kind feature |
tekton-robot
added
the
kind/feature
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
The following is the coverage report on the affected files.
|
|
It is notable that the |
|
|
||
| // Code generated by injection-gen. DO NOT EDIT. | ||
|
|
||
| package pipelinerun |
There was a problem hiding this comment.
Our controller.go is not in an API specific folder, but it uses this genreconciler which is API specific.
We only store one version of the API, so I suppose this simply means that in the controller will use the reconciler for the API version that is stored?
There was a problem hiding this comment.
So controllers use the version of the "primary resource" that they set up informers for, which is orthogonal from the stored version. This is cheapest when it matches the storage version because the informer will go through the conversion webhooks.
We used to (briefly) version our controller directories, but moved away from this when we realized that we'd only have a controller for the single canonical version and never multiple versions of the controller for a particular resource.
This is versioned because you could generate it for each version of a resource, and have multiple controllers[*] of a resource that consumer different versions of the generated code (likely during a version transition).
[*] - We do this in a few cases: extensibility (class-based reconciliation for plug-points), background processes that have been split off the main reconciliation loop (only one writes status).
Hopefully that clarifies, but if not I'd be happy to try to explain further.
There was a problem hiding this comment.
Thanks a lot for this!
A have a few minor comments, but nothing blocking.
Testing to the test controller might be good, but that can be a different PR.
In future we may want to be able to use further functionality from the gencontroller, like returning events and possibly finalizers, but it's great to see that this can fit our controllers already.
Since this is a rather significant change, I was wondering if we should leave this change for v0.14, so that we have some time to play with it before introducing it in a major release?
|
@afrittoli Yeah, I don't want to land this right before a release, which I assumed was imminent. /hold For good measure 😅 |
tekton-robot
added
the
do-not-merge/hold
|
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.
|
|
/hold cancel 0.13 is cut according to Slack :D |
tekton-robot
removed
the
do-not-merge/hold
|
/lgtm |
|
(It depends on #2762 right?) |
|
Yes, rebasing these now. |
|
The following is the coverage report on the affected files.
|
|
/hold Until I can rebase on merged #2762 |
tekton-robot
added
the
do-not-merge/hold
This change migrates the PipelineRun controller to use `// +genreconciler`, which codifies many of the best practices we have adopted across Knative. There are two key pieces to this change: 1. Change the RunningPipelineRun (and the TaskRun equiv) metric reporting to happen on a simple 30s period (see fixed issue below). 2. Change the way we update labels *back* to an Update (vs. Patch). The bulk of the diff (that's not generated) is due to `2.`. I had introduced the Patch in a previous commit in order to avoid optimistic concurrency failures and to avoid needing to update the lister cache for it to properly reflect the preceding status update. With the genreconciler transition the status update now happens second, so I had to bite the bullet. Now this was not as simple as just updating the informer cache due to two gaps in the fake client: 1. Status sub-resource updates modify the entire resource (mismatches K8s). 2. Fake clients don't update or check ResourceVersion for optimistic concurrency. The other major difference here is that genreconciler uses its original copy of the resource when it updates status, so the intervening label update isn't even refetched from the informer cache before the update is attempted. However, because of the gaps above the update was allowed to happen despite the fast that it should have failed optimistic concurrency, and should only have updated status. To address this, I actually taught the fakes to simulate what will happen in reality: 1. The label update will go through, and bump the resource version, 2. The status update will attempt to happen with the stale resource version and FAIL, but 3. The genreconciler's built-in retry logic will notice the conflict, refetch the resource thru the client, and retry the status update (this time succeeding). This is done through new reactors in `test/controller.go`, and should now happen for all of the resources in the Pipeline client (when setup through the seed function). Fixes: tektoncd#2729
|
/hold cancel This is rebased and RFAL |
tekton-robot
removed
the
do-not-merge/hold
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli 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
This change migrates the PipelineRun controller to use
// +genreconciler,which codifies many of the best practices we have adopted across Knative.
There are two key pieces to this change:
Change the RunningPipelineRun (and the TaskRun equiv) metric reporting to
happen on a simple 30s period (see fixed issue below).
Change the way we update labels back to an Update (vs. Patch).
The bulk of the diff (that's not generated) is due to
2.. I had introduced thePatch in a previous commit in order to avoid optimistic concurrency failures
and to avoid needing to update the lister cache for it to properly reflect the
preceding status update. With the genreconciler transition the status update now
happens second, so I had to bite the bullet.
Now this was not as simple as just updating the informer cache due to two gaps in
the fake client:
The other major difference here is that genreconciler uses its original copy of
the resource when it updates status, so the intervening label update isn't even
refetched from the informer cache before the update is attempted. However, because
of the gaps above the update was allowed to happen despite the fast that it should
have failed optimistic concurrency, and should only have updated status.
To address this, I actually taught the fakes to simulate what will happen in reality:
thru the client, and retry the status update (this time succeeding).
This is done through new reactors in
test/controller.go, and should now happenfor all of the resources in the Pipeline client (when setup through the seed function).
Fixes: #2729
Submitter Checklist
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes