Skip to content

Commit

Permalink
Migrate the PipelineRun controller to genreconciler.
Browse files Browse the repository at this point in the history
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: #2729
  • Loading branch information
mattmoor committed Jun 5, 2020
1 parent 2f44504 commit 567c5c2
Show file tree
Hide file tree
Showing 9 changed files with 725 additions and 100 deletions.
1 change: 1 addition & 0 deletions pkg/apis/pipeline/v1beta1/pipelinerun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ var (
)

// +genclient
// +genreconciler
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// PipelineRun represents a single execution of a Pipeline. PipelineRuns are how
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 567c5c2

Please sign in to comment.