Prevent creating a controller if a CRD does not exist.

[sophieliu15]

I hope to create a controller for each GVK that I added dynamically. Previously I noticed that if I started the manager and then installed a new CRD on the fly, I would get an error saying the CRD did not exist when I called NewControllerManagedBy(mgr)...Complete(r). Specifically, this line would return an error. The error message was the following, where CronTab was a new CRD that I installed after starting the controller manager:

Error while trying to create ObjectReconciler","gvk":"stable.example.com/v1, Kind=CronTab","error":"no matches for kind \"CronTab\" in version \"stable.example.com/v1\"

Then I found #554 and updated controller-runtime version to the latest one (v0.5.0) to use DynamicRESTMapper. After updated, I started manager without installing CRD for CronTab. I found that I could create controller for CronTab successfully (i.e., this line does not return any error). However, when the Reconcile method for the controller of CronTab was called, I got the error from pkg/source/source.go complaining the CRD was not found. Following was the error:

{"level":"error","ts":1583448155.5018284,"logger":"controller-runtime.source","msg":"if kind is a CRD, it should be installed before calling Start","kind":"CronTab.stable.example.com","error":"no matches for kind \"CronTab\" in version \"stable.example.com/v1\"","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/pkg/mod/github.com/go-logr/zapr@v0.1.0/zapr.go:128\nsigs.k8s.io/controller-runtime/pkg/source.(*Kind).Start\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.5.0/pkg/source/source.go:88\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func1\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.5.0/pkg/internal/controller/controller.go:165\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.5.0/pkg/internal/controller/controller.go:198\nsigs.k8s.io/controller-runtime/pkg/manager.(*controllerManager).Add.func1\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.5.0/pkg/manager/internal.go:226"}

My questions are:

• Is there any way to prevent creating controller if a specific CRD does not exist in the apiserver? I am currently using:

_, err := r.Manager.GetRESTMapper().RESTMapping(gvk.GroupKind(), gvk.Version)
if err != nil {
  // Do not create controller
}

Not sure if there is a better way (or common practice) to do that?

• After updated to the latest controller-runtime, I would expect the controller creation to fail if I did not install the CRD and succeed after I created the CRD on the fly. I am curious why I was able to create the controller without installing CRD after the update? Is this change expected?

[sophieliu15]

/help

[k8s-ci-robot]

@sophieliu15:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help
/triage/support

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/test-infra repository.

[k8s-ci-robot]

@sophieliu15: The label(s) triage/ cannot be applied, because the repository doesn't have them

In response to this:

/help
/triage/support

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/test-infra repository.

[DirectXMan12]

Using the RESTMapper is a reasonable choice if you want to continue on, but not create the controller.

As for the "no longer fails on .Complete", I have a suspicion that this is due to 0fdf465, which moved around when we start Watches to avoid leaks.

[DirectXMan12]

Based on the linked issue, this is mainly a "don't start this optional controller till this thing is installed", right?

[DirectXMan12]

As for the "no longer fails on .Complete"

Specifically, it should now fail when you start the manager instead.

[DirectXMan12]

or if the manager is already started (which it looks like from the stack trace), this error should induce manager shutdown

[DirectXMan12]

In retrospect this is semi, expected behavior, but it was not intended to break workflows. I've added a note to the v0.4.0 release. Technically, we're okay because that was a breaking release, but we shouldn't have done it without at least a note.

[DirectXMan12]

I do feel mildly uncomfortable about inducing manager failure here, but the general policy is that individual controller failure causes general manager failure, so it's in line with that.

[sophieliu15]

Hi @DirectXMan12

Thanks so much for the explanation! See my replies below.

Based on the linked issue, this is mainly a "don't start this optional controller till this thing is installed", right?

Yes that is correct!

or if the manager is already started (which it looks like from the stack trace), this error should induce manager shutdown

The manager is already started. Looks like in v0.2.2, if the kind does not exist, we cannot create the new controller, but the manager is also not shutted down. In the latest version, the controller can be created, but then it will fail at controller start time and cause manager shutdown (basically what you said in the release notes).

Thanks for the answers and updating the release notes.

[adrianludwin]

or if the manager is already started (which it looks like from the stack trace), this error should induce manager shutdown

Hey @DirectXMan12 , wdyt about just having NewControllerManagedBy(mgr)...Complete(r) fail if the type doesn't exist, as opposed to having the manager as a whole terminate? There's now several use cases of users being able to effectively start new reconcilers on the fly (HNC and Gatekeeper), so it might be nice to have controller-runtime provide an error rather than just shutting down if there's a problem.

Obviously this can wait for a future release of controller-runtime.

[DirectXMan12]

Hmm...

The whole weird thing here is that now non-existence is potentially transient, and that it doesn't actually matter if the type doesn't exist on a call to New -- it only matters when we go to run the watches.

I think this is a question of semantics of New -- it's traditionally in CR (mostly) been "prepare a thing", while Start is "make the thing go". I generally see New as "struct initialization + extra setup" (since Go can't do RAII), so I'm not sure I'd expect "type doesn't exist on the server" to fail, since that's more of a "make the thing go"-type error.

On the other hand, delaying it till start doesn't really make sense when the manager is the one in charge of calling start on all the things -- there's no good way to capture the specific failure without wrapping the controller is a new runnable, and there's currently no good way to say "run this and don't stop the manager if it fails".

All of this is a long-winded way of saying "I'm not sure". I can't help but think this a symptom of something being wrong with the overall architecture of manager, but I'm not quite certain what (maybe error handling in Start and the stoppability of controllers? something with context? unclear).

Anyone have any thoughts? Making .Complete go back to failing, while somewhat semantically troubling, is a pretty straightforward solution, so it's possible I'm just overcomplicating things.

[DirectXMan12]

FWIW, doing non-initialization things in New has led to leaks in the past. This obv wouldn't, but making the semantic distinction is potentially useful for avoiding that.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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/test-infra repository.