Skip to content

Operator cleanly exits in non-existent CRD #523

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

Closed
wants to merge 1 commit into from
Closed

Operator cleanly exits in non-existent CRD #523

wants to merge 1 commit into from

Conversation

ccronca
Copy link
Contributor

@ccronca ccronca commented Sep 24, 2018

This prevents operator panics if it starts before creating the CRD

Fixes #183

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 24, 2018
@openshift-ci-robot
Copy link

Hi @camilocot. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 24, 2018
@allamand
Copy link

Why we won't let the Operator create it's CRD in case it is missing ?

@ccronca
Copy link
Contributor Author

ccronca commented Oct 3, 2018

@allamand It was already discussed on the issue #183

@bradbeam This won't always work in production use cases because installing the CRD requires cluster-level permissions, but running the operator in a namespace only requires permissions in that namespace. Some installations will require the CRD to be installed by a more privileged user than the person who is actually using the operator.

@hasbro17
Copy link
Contributor

hasbro17 commented Oct 8, 2018

@camilocot Sorry for the late review.
Let's just log the error and exit cleanly with os.Exit(1) for all errors. That's much simpler.
I'm not sure of the benefit of only exiting for non-existent CRD and panic for other errors.

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 10, 2018
@hasbro17
Copy link
Contributor

@camilocot Can you please rebase your changes with the master first.
We're have to source your PR branch in our e2e test(to properly resolve dependencies) and there's been some changes to the k8s deps since then. We're on 1.11 now.
https://github.com/camilocot/operator-sdk/blob/feature/exit-crd-inexistent/Gopkg.toml#L11-L23
https://github.com/operator-framework/operator-sdk/blob/master/Gopkg.toml#L11-L23

This prevents operator panics if it starts before creating the CRD

Fixes #183
@ccronca
Copy link
Contributor Author

ccronca commented Oct 15, 2018

@hasbro17 rebase is done, is something else missing?

@estroz
Copy link
Member

estroz commented Oct 16, 2018

@camilocot we made a fairly large change to master today; instead of rebasing, I would recommend creating a new branch, doing a git checkout --hard master on the old branch, then cherry-picking this commit onto your old branch from new. Thanks!

@hasbro17
Copy link
Contributor

@estroz Actually after the rebase we won't have this issue anymore. We don't even have pkg/sdk/api.go anymore.
The runtime's manager exits cleanly if a controller fails to watch a resource that's not registered.

2018/10/16 16:20:16 Go Version: go1.10.2
2018/10/16 16:20:16 Go OS/Arch: darwin/amd64
2018/10/16 16:20:16 operator-sdk Version: 0.0.6+git
2018/10/16 16:20:16 Registering Components.
2018/10/16 16:20:16 no matches for kind "Memcached" in version "cache.example.com/v1alpha1"
exit status 1
2018/10/16 16:20:16 failed to run operator locally: exit status 1

@camilocot We've replaced the SDK apis with the controller-runtime on the master so this should be fixed. We'll have a release out soon that will include this fix.

@hasbro17 hasbro17 closed this Oct 16, 2018
@ccronca ccronca deleted the feature/exit-crd-inexistent branch October 21, 2018 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants