[RFE] Delete operator and operands safely in one command about kubectl-operator HOT 18 CLOSED

So you're suggesting kubectl uninstall operator etcd should fail if etcd operands still exist?

Exactly, I think it needs to fail unless the user indicates "also delete operands" or "delete the operator anyway". That's why it feels appropriate to me to consider a multi-option flag versus additive boolean flags (i.e. --also-delete) with a default setting that aborts (or prompts!) if operands exist. Users still need a path to intentionally orphan operands.

from kubectl-operator.

it might be better to base this on CSV presence instead of subscription presence

Maybe both/either. The presence of a Subscription means that someone at one point wanted an operator to be installed, and it's possible for a lone Subscription to be temporarily stuck (for example, due to a transient CatalogSource reachability issue).

Default: delete only the operator

I'd suggest a descriptive name like cancel that answers the question "what will this command do if operands exist," even if it's the default behavior. I called them cancel/prompt/ignore/delete in an earlier mockup I made (https://asciinema.org/a/BZu3ksvHdat9sfDdcBhtAXTFh).

from kubectl-operator.

So you're suggesting kubectl uninstall operator etcd should fail if etcd operands still exist? How does this look:

$ kubectl operator uninstall etcd
uninstall operator "etcd": operands still exist

To uninstall operator "etcd", use `--force` or delete the following operands first:
<list the operands here>

$ kubectl operator uninstall etcd --force
subscription "etcd" deleted
clusterserviceversion "etcdoperator.v0.9.4-clusterwide" deleted
operator "etcd" uninstalled

OR

$ kubectl operator uninstall etcd
uninstall operator "etcd": operands still exist

To uninstall operator "etcd", use `--force` or delete the following operands first:
<list the operands here>

$ kubectl operator list-operands etcd -o json | kubectl delete -f-
etcdcluster "cluster" deleted

$ kubectl operator uninstall etcd
subscription "etcd" deleted
clusterserviceversion "etcdoperator.v0.9.4-clusterwide" deleted
operator "etcd" uninstalled

from kubectl-operator.

the operands are deleted first, allowing the finalizers on them to be processed by the operator

We currently delete CRDs before the CSV for this exact reason.

So we need some kind of polling or watch on when the operands themselves have successfully been removed, and then delete the operator.

Already handling this via polling in the uninstall action 😎

Some commands already implement timeouts, but I think it makes more sense to configure --timeout as a global persistent flag, and then build a context.WithTimeout with the value and use that context throughout. I can submit a PR for this tomorrow.

from kubectl-operator.

There already exists an kubectl operator uninstall command that deletes objects on cluster. This command can also delete CRDs, subscriptions, and other resources. We could simply modify this command to meet expected behavior.

👍

Let's incorporate operand deletion into the existing uninstall command (and make other uninstall command improvements).

The command should probably have a warning and display the list of CRs to be removed before deleting them, with an additional manual opt in (with a --force override)

Not sure I agree with this. I think the default behavior of uninstall should be to delete just the Subscription and CSV so that no CRDs, operands, and operator groups are affected. Pretty sure that's what uninstall does today.

If users want to opt-in to more deletion, then they should have to explicitly ask for it with extra flags. And if they do ask for it via a flag, that should serve as the only user affirmation needed. This is how kubectl operator uninstall works today with --delete-operator-groups, --delete-crds, etc.

We can add a new --delete-operands (note: --delete-crds should require --delete-operands to be set). The kubectl delete commands don't have an interactive "are you sure?" mode, so I don't think we should either since that mode does not align with kubectl user expectations.

from kubectl-operator.

The kubectl delete commands don't have an interactive "are you sure?" mode, so I don't think we should either since that mode does not align with kubectl user expectations.

Is there a convention that kubectl plugins need to adhere to the same limitations as kubectl? I think one of the advantages of the kubectl plugin system is that it provides the opportunity to extend the feature set and UX of kubectl to do things it doesn't do today.

I argue that the plugin is the correct place to have an interactive experience. I would say this is how it's most used today - interactively, especially when plugins are used. The operand deletion is a special case because we do not have have declarative equivalent. Hence I propose to add a --yes switch suppress any interactive prompt and assume a positive answer from the user.

from kubectl-operator.

I wonder if we can reach a middle ground and not have an interactive option, but still warn the user by copying the kubectl drain behavior. By default, the command drains all nodes from the pods. If there are daemon sets, an explicit flag must be set. If there are pods not managed by built-in controllers the --force option must be used to drain the node.

We could mimic the same behavior. By default, if the deletion is "safe enough" ie no other operators are affected, the command deletes the operator and operands. If we do detect a potential dependency or other issue that could affect the stability of the cluster workloads, we list the operands and return an error stating why it may be potentially unsafe to delete. Then we require the user to use the --force option to opt-in to a deletion that is risky.

This gets us what we want generally -- a non interactive CLI command that still gives the user a warning in the case where it may potentially be risky to delete the operator and its operands. WDYT? @dmesser @joelanford

from kubectl-operator.

Have you considered a mode switch, similar to the way GNU Grep has a --binary-files option that selects from a several modes of handling binary files?

Users may have reasonable use cases for strategies like:

• take no action
• delete the operator but do not delete the operands
• delete the operator and the operands

It's also plausible that a user may not realize that an operator they are deleting is managing any operands. In that case, I think it's not ideal to proceed with operator deletion without requiring the user to make a decision (whether interactively or non-interactively). I don't think it's necessarily a problem for automation if the command is interactive by default. It can call isatty and fall back to a failure message if no terminal is attached.

One upside of an interactive mode that I haven't seen mentioned is that it allows the user to review and approve the exact set of objects to be deleted. That set could change between one invocation and the next.

from kubectl-operator.

One upside of an interactive mode that I haven't seen mentioned is that it allows the user to review and approve the exact set of objects to be deleted. That set could change between one invocation and the next.

That's a really good point

from kubectl-operator.

Is there a convention that kubectl plugins need to adhere to the same limitations as kubectl?

There are no conventions like that I'm aware of. But I think its unfair to call kubectl's lack of interactivity a limitation. Lots of interactive CLIs I've seen have buggy implementations that don't support cursor manipulation or they require esoteric keyboard controls to make selections or change focus.

I think my view is mainly "people are using kubectl delete non-interactively, so let's follow the principle of least surprise and make our deletion workflows non-interactive as well, but with plenty of safeguards to ensure what gets deleted is really what users expect to be deleted".

One upside of an interactive mode that I haven't seen mentioned is that it allows the user to review and approve the exact set of objects to be deleted. That set could change between one invocation and the next.

I guess I see this a little differently. IMO uninstalling an operator and deleting operands are mostly orthogonal. You can remove an operator without deleting operands and you can remove operands without deleting their operator.

So what we're talking about here, I think, is the use case of fully cleaning up everything associated with an operator. That's a completely valid use case that I'm sure many people will use. What I don't think users will do (often at least) is delete the operator and some subset of the operands. Those seem like separate operations that should happen with separate invocations.

Users may have reasonable use cases for strategies like:

• take no action
• delete the operator but do not delete the operands
• delete the operator and the operands

This is mostly what I was explaining exists today, except that there are separate boolean flags for each thing to delete. All I think I'm proposing is that operand deletion should be non-interactive but explicitly opt-in via a flag (whether that be a fully separate boolean flag or a flag value).

from kubectl-operator.

What I don't think users will do (often at least) is delete the operator and some subset of the operands.

My point is not that users want to delete some subset of operands -- I've been assuming that interactivity is limited to delete all or delete none. But I do think that the decision about whether or not a user wants to delete all operands with the operator will depend on what specifically those operands are. If I as a user do not realize that the operator is still managing some workloads, I do not want this command to remove the operator unless I have explicitly acknowledged in some way that I understand I'm orphaning some workloads.

from kubectl-operator.

Or maybe instead of --force it's what you were describing where another item needs to be passed to a --also-delete flag. For example:

• No --also-delete flag - delete the subscription and CSV only, but bail out if operands are present
• --also-delete=operatorgroups - If operands exist, bail out. Otherwise, delete subscription, CSV, CRDs, and if this was the only operator in the namespace, also delete the operator group.
• --also-delete=operands - delete subscription, CSV, and operands
• --also-delete=crds,operands - delete subscription, CSV, CRDs, and operands
• --also-delete=crds - fail out, can't delete crds without deleting operands
• --also-delete=crds,operands,operatorgroups - delete subscription, CSV, CRDs, and operands. and if this was the only operator in the namespace, also delete the operator group.
• --also-delete=all - same as --also-delete=crds,operands,operatorgroups

from kubectl-operator.

By the way, I'm removing the olm label, which I had intended to mean "blocked by something OLM needs to implement". I updated the label description to be more precise that it's to be used only when a feature is blocked.

I think we can implement this feature with no changes to OLM

from kubectl-operator.

One additional point I'd like to add regarding the UX, that hasn't yet been mentioned:

The whole point of this feature is to provide intelligent deletion of operators ie the operands are deleted first, allowing the finalizers on them to be processed by the operator, and only then deleting the operator afterwards. So we need some kind of polling or watch on when the operands themselves have successfully been removed, and then delete the operator. We also probably want some kind of default timeout or way of exiting the command with a non-zero exit code? It's possible the finalization can take more than say 5 minutes. Otherwise we could just return after the command runs with no explicit guarantees on what happens next (but what if the finalizer fails to be removed?)

In general this is another UX consideration that we have to keep in mind -- we don't just delete the operator and operands in one fell swoop like we do when we just delete operands. We have to pipeline the deletion in stages somehow.

from kubectl-operator.

I can submit a PR for this tomorrow

Global timeouts PR here: #43

from kubectl-operator.

I think we are largely in agreement here. Some additional thoughts:

• With regards to deleting the CRD, should we care if the CRD in question is a required API of another CSV? Deleting it in that case could leave other operators in a bad state. @dmesser stated that we do not enforce this check server-side already and let users delete operators out from other operators without stopping them. Client-side, I'm wondering if we can enforce this check by return an additional "this CRD is required by other operators" error and suggest they use the --force flag in this case.

This would require a cluster-wide scan of CSVs to enforce, but since this is client-side maybe its not too expensive to get away with? If we log to the user what we are doing.

• Deleting the operatorgroup only after the rest of the deletion has succeeded, instead of failing up front if more than one CSV is present in the namespace, is an interesting design choice. Not sure I disagree, but it might be a bit unexpected given our very deliberate strategy of being careful with regards to deletion and erroring early

from kubectl-operator.

Deleting the operatorgroup only after the rest of the deletion has succeeded, instead of failing up front if more than one CSV is present in the namespace, is an interesting design choice. Not sure I disagree, but it might be a bit unexpected given our very deliberate strategy of being careful with regards to deletion and erroring early

Right now, if there are multiple CSVs (actually subscriptions, I think) in the namespace, then the --delete-operator-groups flag is a no-op. It only tries to delete the operator group at the end if there's nothing left that needs an operator group. Though you do correctly point out that it might be better to base this on CSV presence instead of subscription presence.

from kubectl-operator.

Summarizing what we discussed on our implementation call:

• Use the existing kubectl operator uninstall command to remove an operator and operands
• Slim the existing command down to only delete the bare minimum we are interested in supporting in downstream OCP at the moment: the subscription, csv (operator), and optionally operands
• Make the command non-interactive but throw an error in-case of deleting an operator with operand without an explicit operand-strategy flag being set.
• The operand-strategy flag will have three settings:
  • Default: delete only the operator. This is the setting when no other strategy is provided and will throw an error if a user attempts to delete an operator with operands on-cluster
  • ignore. This will intentionally orphan operands on-cluster and only remove the operator
  • delete. This will delete both the operator and operands.
• Since CRD deletion is potentially fraught, remove this functionality from the command (as well as the operator group deletion as well potentially)
• These steps ensure that we have a small command that does just what we need it to while ensuring some level of safety. It is non-interactive but still expects the user to explicitly opt-in to operand deletion and provide a strategy to do so. As other objects are uninstalled additional strategies can be provided on a per-object Kind basis.

Additional follow-up work includes changing existing commands to ensure that they can be supported downstream (for example install), setting up prow for this repository, and getting more information from oc maintainers on downstreaming the plugin.

from kubectl-operator.