Uninstallation flow for DFC offering #63
Conversation
bindrad
force-pushed
the
dfc_uninstall
branch
6 times, most recently
from
d838477 to
9e958e0
Compare
controllers/datafoundationclient.go
Outdated
| @@ -40,11 +43,16 @@ const ( | |||
| csiKMSConnectionDetailsConfigMapName = "csi-kms-connection-details" | |||
| storageClientCRDName = "storageclients.ocs.openshift.io" | |||
| storageClassClaimCRDName = "storageclassclaims.ocs.openshift.io" | |||
| defaultStorageClientName = "ocs-storageclient" | |||
There was a problem hiding this comment.
Why default? this is the only storageclient we support right now.
| defaultStorageClientName = "ocs-storageclient" | |
| storageClientName = "ocs-storageclient" |
controllers/datafoundationclient.go
Outdated
| for i := range storageClassClaimList.Items { | ||
| storageClassClaim := &storageClassClaimList.Items[i] | ||
| r.Log.Info("Issuing a delete for storageClassClaim %s", storageClassClaim.Name) | ||
| if err := r.unrestrictedDelete(storageClassClaim); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to issue a delete for storageClassClaim %s: %v", storageClassClaim.Name, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Two issues I see with this loop.
- if one SCC delete fails, you will not issue and delete for anything else after it.
- You might be issuing a delete for an already "marked for deletion" SCC again and again and again.
| r.Log.Info("issuing a delete for StorageClient") | ||
| if err := r.delete(&r.storageClient); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to issue a delete for Storage Client: %v", err) | ||
| } |
There was a problem hiding this comment.
Do we need this? we are the controller owner of this resource. It will get deleted when the offering CR will get deleted. Am I wrong here?
There was a problem hiding this comment.
I will try to explain using a scenario
- Let's assume that the ocs-client operator adds a finalizer to a secret for some reason if a storageClient is created.
- This finalizer should be removed by the ocs-client operator if storageClient is in deleting state.
Now, if we delete offering CR that is the owner of storageClient then storageClient will be removed instantly as the owner(offering CR) was removed. At this moment, it is possible that ocs-client operator didn't trigger a reconcile quick enough to remove the finalizer from the secret. This will block deletion of namespace. So according to me, the agent should initiate the deletion of storageClient and after the successful removal of storageClient, we should remove offering CR.
There was a problem hiding this comment.
We do the same for storageCluster in DF offering
There was a problem hiding this comment.
What you are describing is a bug in the uninstallation/deletion flow of the client. If the client added a finalizer on a resource it needs to act in one of two consistent ways:
- remove the finalizer if the client is marked for delete
- stop uninstallation until the secret is removed.
Which one is dependent on what it tries to achieve.
Any other behavior will be considered a bug and the client developers will need to fix it.
There was a problem hiding this comment.
I believe we do the same because we copy-pasted the current behavior from the old deployer where there was no governing offering CR that held an owner ref. We need to evaluate if a change is also needed there.
It all depends on how you want the behavior to look like. Do we implement an eventually consistent model for offering removal or a sync one?
There was a problem hiding this comment.
where there was no governing offering CR that held an owner ref
We had managedOCS CR owning the storageCluster, right?
eventually consistent model for offering removal or a sync one
Can you elaborate on this?
There was a problem hiding this comment.
For now, we can follow the behaviour of the deployer. If required, we can evaluate if a change is required for both offerings. I have updated the PR to follow an eventually consistent model for offering removal.
- checks for PVs using ocs storage classes and halt uninstallation if found - issue delete for all storageclass claims - issue delete for storageclient Signed-off-by: Dhruv Bindra <[email protected]>
Raised this PR as replacement of #55
This PR implements uninstallation flow for DFC offering