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

Remove scc on uninstall #191

Closed
wants to merge 2 commits into from
Closed

Conversation

gkurz
Copy link
Member

@gkurz gkurz commented May 13, 2022

Let's make sure that the controller removes the SCC it has created when it exits.
This ensures that the SCC doesn't stay around when OSC is uninstalled or when
installation fails.

Fixes: https://issues.redhat.com/browse/KATA-156

gkurz added 2 commits May 13, 2022 16:12
Some code in main() can fail but there's no rollback path. It isn't
possible to use `defer` because error paths call os.Exit(), which
means that main() doesn't return.

Move all the code to a _regular_ function that returns its status
instead of calling os.Exit(). This will to implement proper
rollback using `defer`.

Signed-off-by: Greg Kurz <groug@kaod.org>
The controller creates the SCC but never deletes it. This causes the SCC
to remain alive in the cluster even if the rest of the installation fails,
e.g. failure to create the webhook, or if the operator is uninstalled.

Introduce proper rollback with a defered function call.

Signed-off-by: Greg Kurz <groug@kaod.org>
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 13, 2022
@openshift-ci openshift-ci bot requested review from bpradipt and jensfr May 13, 2022 16:06
@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 13, 2022
@openshift-ci
Copy link

openshift-ci bot commented May 13, 2022

Hi @gkurz. Thanks for your PR.

I'm waiting for a openshift 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

Copy link

@c3d c3d left a comment

Choose a reason for hiding this comment

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

Looks like a necessary first step, but as usual, problems with nested errors (e.g. error in a defer)

@@ -152,6 +153,13 @@ func createScc(ctx context.Context, mgr manager.Manager) error {
return err
}

func deleteScc(ctx context.Context, mgr manager.Manager) error {
Copy link

Choose a reason for hiding this comment

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

Notice that this function returns an error that is ignored.

@@ -146,6 +153,13 @@ func createScc(ctx context.Context, mgr manager.Manager) error {
return err
}

func deleteScc(ctx context.Context, mgr manager.Manager) error {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check for the presence of existing KataConfig and proceed with delete when no KataConfig exist.

@bpradipt
Copy link
Contributor

@gkurz can you please review the following PR - #193

@bpradipt bpradipt closed this Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants