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

Take advantage of kmeta.OwnerRefable. #4110

Merged
merged 1 commit into from
Jul 21, 2021

Conversation

mattmoor
Copy link
Member

Changes

I noticed that the tekton controller.go files were inconsistently using controller.HandleAll (cosmetic), but also that several of the impl.EnqueueControllerOf lines in the PipelineRun controller weren't filtering on the kind of the owner (can cause unnecessary reconciliation).

For the latter, we also introduced a nice new helper controller.FilterController(&v1beta1.TaskRun) that takes advantage of kmeta.OwnerRefable to filter on the GVK in the owner reference.

This spiraled a bit because it turns out Tekton wasn't taking advantage of kmeta.OwnerRefable or kmeta.NewControllerRef! So the majority of the churn in this change is:

  • GetOwnerReference -> GetGroupVersionKind (implements kmeta.OwnerRefable)
  • foo.GetOwnerReference() -> *kmeta.NewControllerRef(foo)

As some background, our kmeta.NewControllerRef API was modeled after this from API machinery: https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#NewControllerRef

We added our own variant to drop the tedious GVK requirement, since the first argument should be sufficient (thus kmeta.OwnerRefable).

/kind cleanup

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Docs included if any changes are user facing
  • Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

NONE

cc @dlorenc @imjasonh @vdemeester @afrittoli

I noticed that the tekton `controller.go` files were inconsistently using `controller.HandleAll` (cosmetic), but also that several of the `impl.EnqueueControllerOf` lines in the PipelineRun controller weren't filtering on the kind of the owner (can cause unnecessary reconciliation).

For the latter, we also introduced a nice new helper `controller.FilterController(&v1beta1.TaskRun)` that takes advantage of `kmeta.OwnerRefable` to filter on the GVK in the owner reference.

This spiraled a bit because it turns out Tekton wasn't taking advantage of `kmeta.OwnerRefable` or `kmeta.NewControllerRef`!  So the majority of the churn in this change is:
 - `GetOwnerReference` -> `GetGroupVersionKind` (implements `kmeta.OwnerRefable`)
 - `foo.GetOwnerReference()` -> `*kmeta.NewControllerRef(foo)`

As some background, our `kmeta.NewControllerRef` API was modeled after this from API machinery: https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#NewControllerRef

We added our own variant to drop the tedious GVK requirement, since the first argument should be sufficient (thus `kmeta.OwnerRefable`).

/kind cleanup
@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jul 21, 2021
@tekton-robot tekton-robot requested review from afrittoli and a user July 21, 2021 15:42
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 21, 2021
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks for this cleanup!

For my own edification: Can you clarify what kinds of unnecessary reconciliations we might have experienced previously, that will be avoided with this new version?

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2021
@mattmoor
Copy link
Member Author

For my own edification: Can you clarify what kinds of unnecessary reconciliations we might have experienced previously, that will be avoided with this new version?

Absolutely. So one of the byproducts of how this was written before (unfiltered) the EnqueueControllerOf code would look at the owner reference and queue namespace/name key regardless of whether that key's type matches the controller's "primary key" type or not.

So if I have a MatrixRun resource named bob, which stamps out TaskRun or Run resources, then the PipelineRun controller will observe changes to these and queue namespace/bob. Likely this means log spam because there isn't a namespace/bob PipelineRun resource, but if there is, then it also means extra work is done.

controller.FilterController(kmeta.OwnerRefable) and kmeta.NewControllerRef(kmeta.OwnerRefable) are sort of duals. One sets up the owner reference, and the other filters things that aren't owned.

@dlorenc
Copy link
Contributor

dlorenc commented Jul 21, 2021

/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dlorenc

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2021
@tekton-robot tekton-robot merged commit d51e632 into tektoncd:main Jul 21, 2021
@mattmoor mattmoor deleted the owner-refable branch July 21, 2021 18:15
@mattmoor mattmoor mentioned this pull request Jul 21, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants