operator-framework / kubectl-operator

Manage Kubernetes Operators from the command line
https://operatorframework.io/
Apache License 2.0
128 stars 37 forks source link

fix: operator uninstall improvements #48

Closed exdx closed 2 years ago

exdx commented 3 years ago

This PR has several commits that improve the operator uninstall command ahead of the next release which will contain the new operand deletion functionality.

The primary improvement is that previously uninstalling an operator that had no owned APIs or was in a bad state would fail, since the list-operands library which uninstall relies upon would return an error. However, in the context of uninstall, these errors should not be terminal, and now the uninstall can proceed even if the list operands call returns an error. This bug occurred naturally since uninstall and list-operands have slightly different purposes.

Other fixes include handling the operand strategy cancel case properly, adding a list-operands description to the uninstall helptext, and pretty printing the object name and optionally namespace when deleting them from the cluster.

openshift-ci-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: exdx To complete the pull request process, please assign joelanford after the PR has been reviewed. You can assign the PR to them by writing /assign @joelanford in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/operator-framework/kubectl-operator/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci[bot] commented 2 years ago

@exdx: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
avisiedo commented 2 years ago

Hi @exdx , I have been searching some issue ticket or PR that actually could be holding the change that I suggest in the next lines to avoid repeat it, and I have found this PR that could make sense to hold this change. If I am wrong and a new issue ticket is necessary please let me know and I will create it.

The change I suggest is at the line below:

https://github.com/operator-framework/kubectl-operator/blob/9e5901c77b37c9a709cedc080733d3ca7bb50b64/internal/pkg/action/catalog_add.go#L193

to be changed by this:

command := []string{
        "/bin/sh",
        "-c",
        fmt.Sprintf(`mkdir -p %s && \
/bin/opm registry add   -d %s --mode=%s -b %s && \
exec /bin/opm registry serve -d %s -p %s`, dbDir, dbPath, a.InjectBundleMode, strings.Join(a.InjectBundles, ","), dbPath, grpcPort),
    }

It makes the last command to replace the shell so it receives the SIGTERM signal when the pod deletion is made; that should improve the time that is needed to get the pod deleted; actually the pod is deleted after the grace period as only the shell receives the SIGTERM signal and the signal is not propagated to the child processes, and after the grace period is reached, the SIGKILL signal is send to any process still running in any pod container (stopping the opmprocess). With this change the pod would be deleted almost immediately (the time that opm needs to finish orderly).

I have checked it by relaunching the pod with the modified command and when I deleted the bundle with OLM, the pod created for adding the catalog was deleted almost immediately.

I have observed the above behavior developing our operator when uninstalling the operator bundle by operator-sdk cleanup freeipa-operator --namespace $(oc project -q).

joelanford commented 2 years ago

@avisiedo Can you please make a separate issue/PR for that fix?

exdx commented 2 years ago

Closing in favor of #55