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

pkg/restmapper,scaffold/cmd.go: override default restmapper #1329

Merged

Conversation

jcrossley3
Copy link
Contributor

This addresses a controller-runtime bug which can affect operators
that create CRD's and then CR's that refer to them, as the default
mapper never "sees" the new CRD's.

Fixes #1328 (originally from @danwinship)

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 16, 2019
@AlexNPavel
Copy link
Contributor

Looks good. To fix the tests, you need to update internal/pkg/scaffold/cmd_test.go to match the expected scaffold output, and you need to commit the Gopkg.lock file after running make dep. Otherwise, this looks good to me.

/cc @shawn-hurley @joelanford @hasbro17

Copy link
Member

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

LGTM just had a question on if this should be the default or not.

@@ -122,6 +123,7 @@ func main() {
// Create a new Cmd to provide shared dependencies and start components
mgr, err := manager.New(cfg, manager.Options{
Namespace: namespace,
MapperProvider: restmapper.NewDynamicRESTMapper,
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we should make this an opt-in feature because missing there is no mechanism's in place to back off expensive reloads on every miss.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reasoned that it's better to be slow than wrong, by default, but I could go either way.

Choose a reason for hiding this comment

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

Solly wanted a backoff upstream (controller-runtime); that's why I haven't just submitted this there already.

I guess the case where it would matter would be if you're waiting for another operator to create a bunch of objects of some CRD type. So then every now and then you do 50 GETs, to see if the 50 objects have been created. Which, with this code, could result in 50 restmapper reloads if the CRD hadn't been created yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm probably missing something but do we only incur the miss when the client/cache is used to CRUD an instance/CR of a newly created CRD whose mapping is not present in the DynamicRestMapper?

If so then I assume most likely this call(e.g GET) would be made inside the reconcile loop. If the DynamicRestMapper reloads and misses(because the CRD is not yet created), and we get an error from client.Get(), then we can just rely on backoff of the controller's workqueue right?
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/internal/controller/controller.go#L210-L215

If this call is not in the reconcile loop, then I imagine most likely the operator exits and restarts in the absence of a retry.

@jcrossley3
Copy link
Contributor Author

/retest

@shawn-hurley
Copy link
Member

We probably want to update the changelog with this change as well

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2019
This addresses a controller-runtime bug which can affect operators
that create CRD's and then CR's that refer to them, as the default
mapper never "sees" the new CRD's.

Fixes operator-framework#1328 (originally from @danwinship)
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2019
@jcrossley3
Copy link
Contributor Author

changelog updated. i think i got the rebase right. lemme know if you need anything else.

Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@AlexNPavel AlexNPavel left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2019
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM

@jmazzitelli
Copy link

This PR didn't fix my problem in #1330, which I was thinking it should have fixed it:

Make sure I'm running 0.8.0:

{"level":"info","ts":1558465126.8952413,"logger":"cmd","msg":"Go Version: go1.12.5"}
{"level":"info","ts":1558465126.8952928,"logger":"cmd","msg":"Go OS/Arch: linux/amd64"}
{"level":"info","ts":1558465126.895298,"logger":"cmd","msg":"Version of operator-sdk: v0.8.0"}
{"level":"info","ts":1558465126.895325,"logger":"cmd","msg":"Watching namespace.","Namespace":"kiali-operator"}

But I still see the error:

{"level":"error","ts":1558465170.3098834,"logger":"proxy","msg":"Could not get rest mapping for: monitoring.kiali.io/v1alpha1, Kind=MonitoringDashboard","error":"no matches for kind \"MonitoringDashboard\" in version \"monitoring.kiali.io/v1alpha1\"","stacktrace":"github.com/operator-framework/operator-sdk/vendor/github.com/go-logr/zapr.(*zapLogger).Error\n\tsrc/github.com/operator-framework/operator-sdk/vendor/github.com/go-logr/zapr/zapr.go:128\ngithub.com/operator-framework/operator-sdk/pkg/ansible/proxy.shouldAddOwnerRef\n\tsrc/github.com/operator-framework/operator-sdk/pkg/ansible/proxy/inject_owner.go:151\ngithub.com/operator-framework/operator-sdk/pkg/ansible/proxy.(*injectOwnerReferenceHandler).ServeHTTP\n\tsrc/github.com/operator-framework/operator-sdk/pkg/ansible/proxy/inject_owner.go:89\ngithub.com/operator-framework/operator-sdk/pkg/ansible/proxy.(*cacheResponseHandler).ServeHTTP\n\tsrc/github.com/operator-framework/operator-sdk/pkg/ansible/proxy/cache_response.go:127\nnet/http.serverHandler.ServeHTTP\n\t/home/travis/.gimme/versions/go1.12.5.linux.amd64/src/net/http/server.go:2774\nnet/http.(*conn).serve\n\t/home/travis/.gimme/versions/go1.12.5.linux.amd64/src/net/http/server.go:1878"}
{"level":"error","ts":1558465170.309941,"logger":"proxy","msg":"Could not determine if we should add owner ref","error":"no matches for kind \"MonitoringDashboard\" in version \"monitoring.kiali.io/v1alpha1\"","stacktrace":"github.com/operator-framework/operator-sdk/vendor/github.com/go-logr/zapr.(*zapLogger).Error\n\tsrc/github.com/operator-framework/operator-sdk/vendor/github.com/go-logr/zapr/zapr.go:128\ngithub.com/operator-framework/operator-sdk/pkg/ansible/proxy.(*injectOwnerReferenceHandler).ServeHTTP\n\tsrc/github.com/operator-framework/operator-sdk/pkg/ansible/proxy/inject_owner.go:92\ngithub.com/operator-framework/operator-sdk/pkg/ansible/proxy.(*cacheResponseHandler).ServeHTTP\n\tsrc/github.com/operator-framework/operator-sdk/pkg/ansible/proxy/cache_response.go:127\nnet/http.serverHandler.ServeHTTP\n\t/home/travis/.gimme/versions/go1.12.5.linux.amd64/src/net/http/server.go:2774\nnet/http.(*conn).serve\n\t/home/travis/.gimme/versions/go1.12.5.linux.amd64/src/net/http/server.go:1878"}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. 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.

Default RESTMapper returns stale resources
8 participants