Signal cluster deletion

[abutcher]

/cc @sdodson @dgoodwin @abhinavdahiya

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abutcher
To complete the pull request process, please assign pmorie
You can assign the PR to them by writing /assign @pmorie 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

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dgoodwin]

Can this be done as a release image manifest?

[sdodson]

That would risk racing against the CVO which will always attempt to create release image content, no?

[wking]

This is always going to be best-effort, because there's no guarantee that the cluster is alive enough to be able to clean itself up when the aliveness CR is removed. So races are probably acceptable, although it would be good to minimize them where possible.

[dgoodwin]

Per above flags proposed, the delete failure policy would be helpful here.

[dgoodwin]

Does this need to be namespaced or should it be global?

[abutcher]

I don't think this needs to be namespaced.

[abutcher]

Namespace removed in last update.

[eparis]

I don't love non-namespaced things. Especially if we are going to make it a generic Kind like Alive instead of ClusterAlive then having something namespaced I could use the same way for me own purpose would be nice.

[abutcher]

Re-added namespace to the object.

[abhinavdahiya]

another think we should include is some mechanism to transfer information about the clusteralive delete.. the send delete and wait without any information about what's happening, or what's failing is going to be too frustrating for installer team to debug or users too.

[abhinavdahiya]

Also we should allow the admin to modify the clusteralive in such a way that destroy cannot proceed, just as a safety check.

[dgoodwin]

Admin being able to block seems useful. Possibly for hive as well.

Is examining the finalizers left on the ClusterAlive sufficient for reporting what's blocking it? Admins could inject their own.

[abhinavdahiya]

As long as it's documented

• how operators can opt-in
• how installer is supposed to trigger
• how installer is supposed to track
• how admins can add add safety checks, for me this requirement should be, that no deleting should start when admin has set something like this.

[dgoodwin]

Somewhere in this I'd like to flesh out what we expect to happen here. I believe the proposal from OLM was that operators set finalizers on their CRs, and OLM would set a finalizer on ClusterAlive. When deleted, it would delete every CR for every CRD for every operator, giving them their chance to cleanup resources.

This would still work with regard to @abhinavdahiya 's request for an admin override to block this. Assuming ClusterAlive.Spec.BlockTeardown = true, OLM could respect this flag and refuse to start it's teardown process, we only need to check BlockTeardown in one place.

If however operators start adding their finalizers directly to ClusterAlive, we would need to guarantee that they all also knew to respect this additional BlockTeardown field. The OLM layer of indirection seems required so I'd like to make sure they're still on board with that plan to use CRs+CRDs.

cc @ecordell

[eparis]

What is this? What does it do? How does it relate?

[abutcher]

Also we should allow the admin to modify the clusteralive in such a way that destroy cannot proceed, just as a safety check.

This was suggested by @abhinavdahiya as a mechanism for admins to block teardown in destroy when set by an admin. I'll add this description to the enhancement.

[eparis]

I don't understand the use case or why i'd want what is being described. so hopefully you can lay that out as well. Who is responding to this value in spec? Whatever set the finalizers? Why delete the object if you don't want those things to respond?

[abhinavdahiya]

#265 (comment)
#265 (comment)

[eparis]

I read those, but I don't think I get it. Is this supposed to prevent someone from 'accidentally' deleting the Alive object and triggering teardown? Are you trying to make it a 2-step process? What's the use case?

Say blockTeardown: true
I try to delete the clusterAlive object
We expect all finalizers on the object to pay attention to spec and just do nothing?
Should they remove themselves from the finalizer list?
Should the ClusterAlive object get deleted? Or should it just live forever with a deletionTimestamp but never be deleted?

And why am I doing this? I'm perfectly happy to understand answers to these questions inside the enhancement instead of here when ready.

[abhinavdahiya]

so currently when deleting the cluster, using destroy cluster we cleanup all the openshift-core-cluster created resources like the kubernetes.io/cluster/infra-id: owned

we don't delete any user created resources that don't users don't opt-in into the ownership, and that is also best-effort.

not with cluster alive and destroy cluster, there is higher degree of safety concern as now OLM might delete user created resources, like databases etc.

and therefore, i think it's necessary that the admins be allowed to stop prevent any such action to take place.

my initial recommendation was to add this to spec, such that when the user runs destroy cluster --shutdown that installer can fail saying that the admin has rejected such action.

also we could also write a admission plugin that would reject any deletes on the object when this field is set.

the operators like OLM probably shouldn't need to actively reconcile this i.e removing finalizers when this being set, but i don't see why that would be bad...

if we can have all operators always add their finalizers, and installer check this or even better has the admission plugin I think that would be very helpful @eparis

[abutcher]

I've added these details to the enhancement for blockTeardown.

[eparis]

How do I get a record of every time the --cluster-alive-delete failed?

[abutcher]

What do you think about a field in .spec or .status that we increment when we attempt the destroy?

[eparis]

I meant in CI, how do I get that record? how do we know it's broken in CI and not just getting ignored by the ||true forever

[wking]

...how do we know it's broken in CI...

We have lots of sub-components in the installer that are not independently tested, so I don't think we need to block on closing this one. If you consider it especially vulnerable, are you worried about the installer breaking or a particular component forgetting to register a finalizer or flubbing the finalizer implementation? If you're worried about the installer, you could have CI ask a trusted-enough operator to create an unknown-to-the-installer resource which would block teardown, and then see if the backstopping reapers light up with lots of leaked resources.

[eparis]

the || true would only hit the case of a flubbed implementation from something setting a finalizer, which would be a real bug.

Your suggestion, I think, is that the aws reaper might eventually notice we were wasting resources/money and we could then do some spelunking to figure out what left it behind.

Could we somehow, instead do (obviously psuedocode)

retval = openshift-install destroy cluster --cluster-alive-delete
if retval != success
openshift-install destroy cluster
return retval

This moves the failure to where it is introduced, not to some point later after DPP realizes we're wasting money.

I do agree, we don't have to block closing on this issue. But it doesn't seem that hard.

[abutcher]

Updated test plan to include returning the retval of the cluster alive delete test if it failed.

[wking]

The name is already expressing Cluster. Maybe the kind should just be Alive so it can be used as a top-level finalizer collector for different sets of resources as well? Like, I dunno:

apiVersion: v1alpha1
kind: Alive
metadata:
  name: human-resources
spec:
  blockTeardown: true

If you wanted to collect a bunch of resources created for HR so you could easily delete them all if HR moved off to a different cluster later.

[abutcher]

Name has been changed to Alive.

[wking]

Probably start with v1alpha1, right? Probably some kind of generic CRD stabilization procedure whose docs we can link in the Graduation Criteria section below about how long we expect to cook before moving to beta and stable.

[abutcher]

Updated object example to v1alpha1 and added a bit of text to graduation criteria re: usage and testing. If you have any docs to a pre-existing CRD graduation criterion I could add it.

[abutcher]

/assign @pmorie

[abutcher]

@LalatenduMohanty Heads up regarding this enhancement. We think it would make the most sense to add this to the CVO. Please let me know if you have any questions or concerns.

[abhinavdahiya]

No, since CVO is instrumental for working of the cluster.

hey there is no owner for this object, let put in CVO should be immediately stopped.

[abutcher]

@abhinavdahiya Where do you suggest this live? In its own separate operator?

[csrwng]

@abhinavdahiya this resource would be used to manage the lifecycle of operators. It seems logical that it belong with the thing that manages operators. I could see an argument for OLM since no SLO has external things it would want to destroy, but either the CVO or OLM are what would make the most sense to me.
@eparis thoughts?

[abhinavdahiya]

@abhinavdahiya this resource would be used to manage the lifecycle of operators.

can you explain how? because to me this helps mange the operands of the operators and not the operators themselves. Its like operators+their operands need k8s api, and therefore CVO should manage the k8s api directly.

Some operator should handle this object and everything required around it. The operator should be able to use the CVO to create the resources for it like CRDs, empty initlal resource instance like others.

[csrwng]

can you explain how?

This is a signal telling you that you are about to be destroyed. In my mind, something similar to https://developer.android.com/reference/android/app/Activity#onDestroy() or https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1623111-applicationwillterminate

It seems logical that the same thing that created you will tell you that you're going away. Admittedly not a perfect analogy. It can be used by either operators or operands.

IMHO creating a separate SLO just for this signal seems overkill.

[LalatenduMohanty]

AFAIU CVO does not mange the life cycle of operators. I agree with @abhinavdahiya with it manges the operands of the operators and not the operators themselves. So that's a big shift in CVO's scope if this comes to CVO.

[LalatenduMohanty]

I am wondering if we will have status conditions, history for this custom resource. It might be useful for debugging purpose when we want to track the events that proceeded a deletion. I think we should ask @smarterclayton about the this as it involves CVO.

[wking]

I see the blockTeardown thread debating the utility of that property. So there is some contention between:

• blockTeardown would be nice to have, but needs someone to own the admission hook behind it.
• CVO doesn't want to own that hook.
• Creating a new second-level operator seems heavy.

Personally, I come down with "so let's drop blockTeardown and suggest users scope their admin auth to not include the ability to delete Alive unless they assume some special role they reserve for super-dangerous things". Then we don't need any code behind this new CRD and CR, just manifests that live somewhere that ends up in the release image for the CVO to reconcile like it does all the other CRD and CR manifests it already manages.

[wking]

Also, if folks are wondering where the manifest would live, we have precedent in cluster-update-keys for a Git repo with thin OWNERS whose sole purpose is to hold manifests that get stuffed into the release image. Spinning up a new repo for openshift/alive or whatever seems like it would be slightly tedious from an internal-process side, but lightweight for everyone after it gets set up.

[ecordell]

This begs the implementation a bit - it's not clear at first glance why a new CRD would be needed to solve this problem.

[ecordell]

I think it's important to nail these down for readers whose introduction to the topic is this document.

I would suggest the following stories, but I may not have everyone's interests in mind:

1. As an OpenShift administrator, I would like for any external resources created in service of the cluster be removed when when I delete the cluster, so that:

• I do not have to track down those resources and remove them manually
• I do not have to pay for resources that are not being used.

2. As an OpenShift (software, big-O) Operator, I would like to have a way to know that the cluster is being shut down, so that I can perform any necessary cleanup steps and remove external resources.

3. As an OpenShift administrator, I would like to have a way to indicate that certain external resources that would normally be cleaned up on cluster deletion be preserved on cluster delete, so that I don't lose important resources stored externally.

[ecordell]

Is there a reason this couldn't piggy-back on existing resources, like ClusterVersion?

[abutcher]

Deletion of the ClusterVersion and other cluster resources are blocked by validation.

https://github.com/openshift/origin/blob/88baf7f/vendor/k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/config/deny_delete_cluster_config_resource.go#L30

[abutcher]

This validation gave us the impression that it could not be used.

[dgoodwin]

ping @jeremyeder , KB suggested pinging you here, this is driven by SD's need to have operators that have a chance to cleanup.

[derekwaynecarr]

It seems better imo for the service control plane when initiating cluster deletion to uninstall operator/operands rather than put this concept direct into the cluster. Can the enhancement weigh that option and why its not preferred?

[jim-minter]

This is probably inherently a best-effort process, and there is a risk that people might interpret it as a guaranteed process. An in-cluster mechanism cannot survive people getting fed up waiting for clean-up to happen, taking things into their own hands and hard deleting the cluster themselves. (Think when you last kill -9'ed a process that was taking too long to quit).

[jim-minter]

Do enough OCP users use openshift-install destroy rather than deleting the relevant cluster resources directly to make this viable? What about customers or automated systems (e.g. customers' CI systems) that might delete cluster resources directly rather than calling openshift-install destroy?

These arguments make me think that attempting to solve this problem inside the cluster might not be wise.

[jim-minter]

FWIW, ARO does not use openshift-install destroy and would need implementation work to support this enhancement if it happened.

[dgoodwin]

I will rework this to be less openshift-install specific. We do leverage openshift-install destroy code in hive and indeed other provisioning infra would need to do the same, if they want to make use of this cleanup functionality.

It would be difficult to manage externally because we simply don't know the details for cleanup of random things that might be in the cluster. The idea here was to allow operators (or any other componen) to maintain it's own cleanup logic, we just want to signal to tell it a teardown is imminent.

[jim-minter]

This could easily be a foot gun. If the external resource is incorrectly marked and holds customer data, it could be quite bad.

[kbsingh]

I agree with @jim-minter here, we should not rely on label and artifact markup outside the cluster - each resource incluster, should track the artifacts it needs to / wants to lifecycle and our experience should optimise for that.

[maleck13]

Agree. The interface for preserving anything off cluster should be a CR owned by the operator that created the off cluster resource(s).

[dgoodwin]

Enhancement updated for feedback, a little less openshift-install specific, explicitly called out need for support in our managed cloud offerings that do not use it, dropped the admin blocking functionality per arch call discussions.

[kbsingh]

@derekwaynecarr to answer the question around if this can be a service trigger and not a product instance trigger : its not possible to actively track how an operator or an operated resource has evolved in its own lifecycle, in cluster; were going to really need a way for an operator to mature along the path of being able to self-service and self-lifecycle its own assets, based on signals for cluster lifecycle that we can deliver

[luciddreamz]

Enhancement updated for feedback, a little less openshift-install specific, explicitly called out need for support in our managed cloud offerings that do not use it, dropped the admin blocking functionality per arch call discussions.

For reference, this has nothing to do with our managed offerings. This RFE is for OCP to signal to OCM (effectively Red Hat Subscription Manager) that the cluster is being destroyed - otherwise OCM doesn't know if the cluster was destroyed or is simply no longer reporting telemetry data (at least for the use case I care about).

[gregsheremeta]

this has nothing to do with our managed offerings

Actually the original intent is to signal RHMI that it can cleanup.

Agree that we could also tap into the deletion signal for OCM purposes, but the original intent of this enhancement was for managed services add-ons to know when they should clean themselves up

[hasbro17]

@abhinavdahiya @abutcher @dgoodwin Is this proposal abandoned or still under discussion?
It seems there are still some open questions on who manages the lifecycle of the Alive CRD.

The reason I'm asking is that I'm working on another enhancement where OLM needs to know signal for cluster destruction to initiate the cleanup of OLM managed operands and operators.
And I'm wondering if this is still the plan for signaling cluster destruction.

[gregsheremeta]

I'm working on another enhancement where OLM needs to know signal for cluster destruction to initiate the cleanup of OLM managed operands and operators.

That's the reason we opened this.

It's not abandoned, but it needs new ownership. @dgoodwin and @abutcher don't have capacity to work on this any longer.

[jsafrane]

At which point of cluster de-provision process is Alive deleted?

Volumes in some (most?) cloud platforms can't be deleted until all pods that use the volumes are deleted and the volumes are unmounted + detached from nodes. Should a new storage operator find these pods and start killing them in order to remove all storage resources when Alive is marked for deletion? The pods are likely to be recreated by StatefulSets or even other operators outside of our control. Or can we expect that Alive is deleted when all user workloads (Pods, StatefulSets, ...) or even Nodes are already deleted? What if a cluster component uses storage that needs to be cleaned out (e.g. monitoring or image registry)?

[cgwalters]

This may already be here but in realtime discussion about this one thing that came up is basically this should work a bit like Github repository deletion that requires you to type the name of the repository.

Scott suggested that there's a CRD that you create, a core controller (CVO) adds something to its spec (a random key) and you have to edit its status to match that key. Basically the thing performing the deletion needs to be a controller; you can't just create (or delete) an object and have the cluster start deleting itself.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[openshift-bot]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

[openshift-bot]

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

[openshift-ci-robot]

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.