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

⚠️ Add TypedReconciler #2799

Merged
merged 1 commit into from
Jul 6, 2024
Merged

Conversation

alvaroaleman
Copy link
Member

This change adds a TypedReconciler which allows to customize the type being used in the workqueue. There is a number of situations where a custom type might be better than the default reconcile.Request:

  • Multi-Cluster controllers might want to put the clusters in there
  • Some controllers do not reconcile individual resources of a given type but all of them at once, for example IngressControllers might do this
  • Some controllers do not operate on Kubernetes resources at all

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 26, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 26, 2024
@alvaroaleman
Copy link
Member Author

/hold
This needs some more testing

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 26, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2024
@@ -64,70 +65,70 @@ type EventHandler TypedEventHandler[client.Object]
// Most users shouldn't need to implement their own TypedEventHandler.
//
// TypedEventHandler is experimental and subject to future change.
type TypedEventHandler[T any] interface {
type TypedEventHandler[object any, request comparable] interface {
Copy link
Member Author

Choose a reason for hiding this comment

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

The main annoyance I found with this change for standard k8s controllers is this, when using a typed eventhandler to have the added typesafety in there it is now also needed to pass the request type. Removing it would mean it is impossible to use a custom request type with source.Kind though, which is for example useful when having watches in different clusters

pkg/source/source.go Outdated Show resolved Hide resolved
@alvaroaleman
Copy link
Member Author

I've tried this change with some internal projects and the changes required there were minimal and mostly around the interface change in TypedEventHandler. It is really cool to be able to use a custom request type and improves readability and type safety a lot in places where something other than the default is needed.

examples/typed/main.go Show resolved Hide resolved
pkg/builder/controller.go Outdated Show resolved Hide resolved
pkg/source/source.go Outdated Show resolved Hide resolved
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Really really nice work! :)

Two high-level points:

  • would be good to have consistent type parameter & corresponding argument names
  • might be good to take another look if we have enough test coverage for the new "Typed" types

examples/typed/main.go Show resolved Hide resolved
examples/typed/main.go Show resolved Hide resolved
pkg/handler/eventhandler.go Outdated Show resolved Hide resolved
pkg/source/source.go Outdated Show resolved Hide resolved
pkg/builder/controller.go Outdated Show resolved Hide resolved
pkg/internal/controller/controller_test.go Show resolved Hide resolved
pkg/reconcile/reconcile.go Outdated Show resolved Hide resolved
pkg/source/source.go Outdated Show resolved Hide resolved
pkg/source/source.go Outdated Show resolved Hide resolved
pkg/source/source.go Show resolved Hide resolved
@@ -0,0 +1,60 @@
package main
Copy link
Member

Choose a reason for hiding this comment

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

The PR description mentions this can be a good use for multiple clusters. Could we add an example that expands on the current reconcile.Request to add Cluster information?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added one under examples/multiclustersync

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2024
This change adds a TypedReconciler which allows to customize the type
being used in the workqueue. There is a number of situations where a
custom type might be better than the default `reconcile.Request`:
* Multi-Cluster controllers might want to put the clusters in there
* Some controllers do not reconcile individual resources of a given type
  but all of them at once, for example IngressControllers might do this
* Some controllers do not operate on Kubernetes resources at all
@k8s-ci-robot
Copy link
Contributor

@alvaroaleman: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-controller-runtime-apidiff 64e0f0b link false /test pull-controller-runtime-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@alvaroaleman
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e68ce7d6eaa2729f0fc45cc1de0e7a62ef7512d7

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, vincepri

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:
  • OWNERS [alvaroaleman,vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 33446bc into kubernetes-sigs:main Jul 6, 2024
8 of 9 checks passed
@alvaroaleman alvaroaleman deleted the workq branch July 7, 2024 14:08
@sbueringer
Copy link
Member

Nice additional example, thx for adding it!

2uasimojo added a commit to 2uasimojo/hive that referenced this pull request Oct 1, 2024
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799
- Removal of yellow squiggles

HIVE-2616
2uasimojo added a commit to 2uasimojo/hive that referenced this pull request Oct 3, 2024
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799
- Removal of yellow squiggles

HIVE-2616
2uasimojo added a commit to 2uasimojo/hive that referenced this pull request Oct 3, 2024
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799
- Removal of yellow squiggles

HIVE-2616
2uasimojo added a commit to 2uasimojo/hive that referenced this pull request Oct 3, 2024
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799
- Removal of yellow squiggles

HIVE-2616
2uasimojo added a commit to 2uasimojo/hive that referenced this pull request Oct 3, 2024
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799
- Removal of yellow squiggles

HIVE-2616
2uasimojo added a commit to 2uasimojo/hive that referenced this pull request Oct 3, 2024
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799
- Removal of yellow squiggles

HIVE-2616
2uasimojo added a commit to 2uasimojo/hive that referenced this pull request Oct 3, 2024
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799
- Removal of yellow squiggles

HIVE-2616
benashz added a commit to hashicorp/vault-secrets-operator that referenced this pull request Oct 4, 2024
2uasimojo added a commit to 2uasimojo/hive that referenced this pull request Oct 4, 2024
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799
- Removal of yellow squiggles

HIVE-2616

(cherry picked from commit 08e4987)
benashz added a commit to hashicorp/vault-secrets-operator that referenced this pull request Oct 4, 2024
…pdates (#943)

* Update to support the TypedEventHandler interface
Related upstream change:
 - kubernetes-sigs/controller-runtime#2799

* Support the sha512sum Sprig template function

Bumps the gomod-backward-compatible group with 11 updates in the / directory:

| Package | From | To |
| --- | --- | --- |
| [cloud.google.com/go/compute/metadata](https://github.com/googleapis/google-cloud-go) | `0.5.0` | `0.5.2` |
| [github.com/Masterminds/sprig/v3](https://github.com/Masterminds/sprig) | `3.2.3` | `3.3.0` |
| [github.com/gruntwork-io/terratest](https://github.com/gruntwork-io/terratest) | `0.47.0` | `0.47.2` |
| [github.com/hashicorp/hcp-sdk-go](https://github.com/hashicorp/hcp-sdk-go) | `0.110.0` | `0.115.0` |
| [github.com/onsi/gomega](https://github.com/onsi/gomega) | `1.34.1` | `1.34.2` |
| [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) | `1.20.3` | `1.20.4` |
| [golang.org/x/crypto](https://github.com/golang/crypto) | `0.26.0` | `0.27.0` |
| [google.golang.org/api](https://github.com/googleapis/google-api-go-client) | `0.192.0` | `0.199.0` |
| [k8s.io/apiextensions-apiserver](https://github.com/kubernetes/apiextensions-apiserver) | `0.30.3` | `0.31.1` |
| [sigs.k8s.io/controller-runtime](https://github.com/kubernetes-sigs/controller-runtime) | `0.18.4` | `0.19.0` |
| [nhooyr.io/websocket](https://github.com/nhooyr/websocket-old) | `1.8.11` | `1.8.17` |



Updates `cloud.google.com/go/compute/metadata` from 0.5.0 to 0.5.2
- [Release notes](https://github.com/googleapis/google-cloud-go/releases)
- [Changelog](https://github.com/googleapis/google-cloud-go/blob/main/CHANGES.md)
- [Commits](googleapis/google-cloud-go@v0.5.0...auth/v0.5.2)

Updates `github.com/Masterminds/sprig/v3` from 3.2.3 to 3.3.0
- [Release notes](https://github.com/Masterminds/sprig/releases)
- [Changelog](https://github.com/Masterminds/sprig/blob/master/CHANGELOG.md)
- [Commits](Masterminds/sprig@v3.2.3...v3.3.0)

Updates `github.com/gruntwork-io/terratest` from 0.47.0 to 0.47.2
- [Release notes](https://github.com/gruntwork-io/terratest/releases)
- [Commits](gruntwork-io/terratest@v0.47.0...v0.47.2)

Updates `github.com/hashicorp/hcp-sdk-go` from 0.110.0 to 0.115.0
- [Release notes](https://github.com/hashicorp/hcp-sdk-go/releases)
- [Changelog](https://github.com/hashicorp/hcp-sdk-go/blob/main/CHANGELOG.md)
- [Commits](hashicorp/hcp-sdk-go@v0.110.0...v0.115.0)

Updates `github.com/onsi/gomega` from 1.34.1 to 1.34.2
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.34.1...v1.34.2)

Updates `github.com/prometheus/client_golang` from 1.20.3 to 1.20.4
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.20.3...v1.20.4)

Updates `golang.org/x/crypto` from 0.26.0 to 0.27.0
- [Commits](golang/crypto@v0.26.0...v0.27.0)

Updates `google.golang.org/api` from 0.192.0 to 0.199.0
- [Release notes](https://github.com/googleapis/google-api-go-client/releases)
- [Changelog](https://github.com/googleapis/google-api-go-client/blob/main/CHANGES.md)
- [Commits](googleapis/google-api-go-client@v0.192.0...v0.199.0)

Updates `k8s.io/apiextensions-apiserver` from 0.30.3 to 0.31.1
- [Release notes](https://github.com/kubernetes/apiextensions-apiserver/releases)
- [Commits](kubernetes/apiextensions-apiserver@v0.30.3...v0.31.1)

Updates `k8s.io/apimachinery` from 0.30.3 to 0.31.1
- [Commits](kubernetes/apimachinery@v0.30.3...v0.31.1)

Updates `k8s.io/client-go` from 0.30.3 to 0.31.1
- [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md)
- [Commits](kubernetes/client-go@v0.30.3...v0.31.1)

Updates `k8s.io/utils` from 0.0.0-20230726121419-3b25d923346b to 0.0.0-20240711033017-18e509b52bc8
- [Commits](https://github.com/kubernetes/utils/commits)

Updates `sigs.k8s.io/controller-runtime` from 0.18.4 to 0.19.0
- [Release notes](https://github.com/kubernetes-sigs/controller-runtime/releases)
- [Changelog](https://github.com/kubernetes-sigs/controller-runtime/blob/main/RELEASE.md)
- [Commits](kubernetes-sigs/controller-runtime@v0.18.4...v0.19.0)

Updates `nhooyr.io/websocket` from 1.8.11 to 1.8.17
- [Release notes](https://github.com/nhooyr/websocket-old/releases)
- [Commits](https://github.com/nhooyr/websocket-old/commits/v1.8.17)

---
updated-dependencies:
- dependency-name: cloud.google.com/go/compute/metadata
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: gomod-backward-compatible
- dependency-name: github.com/Masterminds/sprig/v3
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: gomod-backward-compatible
- dependency-name: github.com/gruntwork-io/terratest
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: gomod-backward-compatible
- dependency-name: github.com/hashicorp/hcp-sdk-go
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: gomod-backward-compatible
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: gomod-backward-compatible
- dependency-name: github.com/prometheus/client_golang
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: gomod-backward-compatible
- dependency-name: golang.org/x/crypto
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: gomod-backward-compatible
- dependency-name: google.golang.org/api
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: gomod-backward-compatible
- dependency-name: k8s.io/apiextensions-apiserver
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: gomod-backward-compatible
- dependency-name: k8s.io/apimachinery
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: gomod-backward-compatible
- dependency-name: k8s.io/client-go
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: gomod-backward-compatible
- dependency-name: k8s.io/utils
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: gomod-backward-compatible
- dependency-name: sigs.k8s.io/controller-runtime
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: gomod-backward-compatible
- dependency-name: nhooyr.io/websocket
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: gomod-backward-compatible
...

Signed-off-by: dependabot[bot] <[email protected]>


---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ben Ash <[email protected]>
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants