Add a `controller.Options` type #293

negz · 2021-09-20T01:23:12Z

Description of your changes

This type is intended to be passed as the argument to most Crossplane Setup functions, for example:

func Setup(mgr ctrl.Manager, o controller.Options) error

This allows us to add new options to be plumbed down to all or most controllers without increasing the number of arguments provided to each Setup function. Sets of controllers that require additional arguments (e.g. the pkg controllers from crossplane/crossplane) can define their own Options struct that embeds this one.

This PR also migrates the *feature.Flags type from crossplane/crossplane, and generalises the names of the two levels of default rate limiters we use. This is all in preparation of adding more configuration knobs to crossplane/crossplane and our providers (e.g. poll interval, average global RPS)

I have:

Read and followed Crossplane's contribution process.
Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

I've built crossplane/crossplane in a branch that uses this commit and ensured it works.

hasheddan

Thanks @negz! Glad to have the controller function signatures getting a little cleaner 👍🏻

pkg/feature/feature.go

pkg/feature/feature_test.go

https://github.com/crossplane/crossplane/tree/b7ce021e32/internal/feature crossplane/crossplane#2313. This is a copy of the (almost) identical crossplane/crossplane package, which will be removed in favor of this one. Moving to crossplane-runtime allows us to use the same package in providers, e.g. to disable Alpha APIs per the above issue. The package is _almost_ identical because the Flag type has been changed from int to string. This makes it easier to give flags string names, because the stringer tool we previously used requires that types and instances be defined in the same package. Signed-off-by: Nic Cope <negz@rk0n.org>

I'd like to reuse these existing ratelimiters for crossplane, where the names 'Provider' and 'Managed' don't make as much sense. Signed-off-by: Nic Cope <negz@rk0n.org>

This type is intended to be passed as the argument to most Crossplane Setup functions, for example: ```go func Setup(mgr ctrl.Manager, o controller.Options) error ``` This allows us to add new options to be plumbed down to all or most controllers without increasing the number of arguments provided to each Setup function. Sets of controllers that require additional arguments (e.g. the pkg controllers from crossplane/crossplane) can define their own Options struct that embeds this one. Signed-off-by: Nic Cope <negz@rk0n.org>

Signed-off-by: Nic Cope <negz@rk0n.org>

This will report that flags aren't enabled if *Flags is nil, rather than panicing. Signed-off-by: Nic Cope <negz@rk0n.org>

I don't really expect these to be used in practice. They're mostly useful for places like the XRD controllers where we need a default set of options to plumb down to the XR and XRC controllers when none are passed to use (i.e. in tests). Signed-off-by: Nic Cope <negz@rk0n.org>

negz · 2021-09-22T02:42:04Z

Requesting another review pass because I ended up pushing a few more commits after this was reviewed. :)

hasheddan · 2021-09-22T02:48:46Z

pkg/controller/options.go

+
+// DefaultOptions returns a functional set of options with conservative
+// defaults.
+func DefaultOptions() Options {


@negz is motivation for using a non-pointer type here that we may pass the options to multiple consumers who may in turn desire to modify them? And features being a pointer type ensures that feature flags are enabled consistently throughout all consumers?

Is motivation for using a non-pointer type here that we may pass the options to multiple consumers who may in turn desire to modify them?

Close, but also kind of the opposite. I tend to prefer to pass structs by value when:

Consumers are expected to treat the struct's values as read-only.

There's no significant performance benefit to passing by reference.

I could see that passing by value might signal "you can mutate your copy", but I feel it also signals "you're not allowed to mutate my copy".

You could make a good argument that Options falls under the "small structs that might grow" clause of https://github.com/golang/go/wiki/CodeReviewComments#pass-values, but I'm not super worried about performance loss due to copying this struct given that it's not really in any hot paths.

And features being a pointer type ensures that feature flags are enabled consistently throughout all consumers?

Actually I think I originally had intended for *Flags to be passed by value for similar reasons to Options, but the go vet copylocks check reminded me that I shouldn't be copying mutexes.

See crossplane/crossplane-runtime#293 This allows us to plumb more options down from our CLI flags to each controller without adding more and more arguments to our Setup functions. This commit also removes most of the internal/feature package, which has moved to crossplane-runtime. Signed-off-by: Nic Cope <negz@rk0n.org>

crossplane/crossplane-runtime#293 This commit adapts core Crossplane to use (mostly) the same pattern as our providers, which use both controller and globally scoped rate limiters to limit reconcile rate when in error or wait situations and fall back to a configurable poll interval when there is a need to speculatively poll (e.g. because we can't watch the thing we're interested in). This commit makes use of the above crossplane-runtime PR to use a new Options type to plumb settings down from main.go to each controller. The Options will be plumbed up to CLI flags in a future commit. One key difference between this implementation and the current managed resource reconciler is that we actually return errors we encounter, rather than swallowing them and returning Requeue: true. This may result in some duplicate logs when running in debug mode (due to our logger and crossplane-runtime's both logging) but it should result in the `controller_runtime_reconcile_errors_total` being a more accurate metric of the errors Crossplane has encountered. Signed-off-by: Nic Cope <negz@rk0n.org>