Fix deletion on pending volume #2410
Conversation
The way we were handling a deletion while the creation is in progress was buggy. We were correctly detecting that we were creating the volume and thus were postponing the deletion until the creation was done (either with success or not). But we were exiting the reconciliation loop too early and never had a chance to keep continue the volume creation, thus we were forever stuck in a Pending state (and the deletion was posptone forever as well). The fix is easy enough though: in such case we don't exit the reconciliation loop immediatly, to be able to call `deployVolume` and made some progress until we're stabilized. Refs: #2409 Signed-off-by: Sylvain Laperche <[email protected]>
Closes: #2409 Signed-off-by: Sylvain Laperche <[email protected]>
Hello slaperche-scality,My role is to assist you with the merge of this Status report is not available. |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
| @@ -676,13 +683,6 @@ func (self *ReconcileVolume) finalizeVolume( | |||
| saltenv string, | |||
| ) (reconcile.Result, error) { | |||
| reqLogger := log.WithValues("Volume.Name", volume.Name) | |||
|
|
|||
| // Pending volume: can do nothing but wait for stabilization. | |||
There was a problem hiding this comment.
Does this need to be removed, or would it make sense to keep this around for safety sake?
There was a problem hiding this comment.
We could keep it, it wouldn't be dangerous as it would be dead code.
But it would be redundant with the check done at the upper level, no?
| When I create the following Volume: | ||
| apiVersion: storage.metalk8s.scality.com/v1alpha1 | ||
| kind: Volume | ||
| metadata: | ||
| name: test-volume11 | ||
| spec: | ||
| nodeName: bootstrap | ||
| storageClassName: metalk8s-prometheus | ||
| sparseLoopDevice: | ||
| size: 10Gi | ||
| Then the Volume 'test-volume11' is 'Pending' | ||
| When I delete the Volume 'test-volume11' | ||
| Then the Volume 'test-volume11' does not exist |
There was a problem hiding this comment.
Are we sure this is going to test "deletion while creation"?
It's the same TOCTOU as you often remember me about :) Sure, volume was Pending, but when you hit the delete Volume line, maybe it's not Pending anymore...
Maybe a single When I create and immediately delete the following Volume: ... would help.
There was a problem hiding this comment.
Yeah, it's racy but I trust Salt to be sloooooow enough to work (if it's flaky the test will succeed anyway so it won't bother us too much).
Having a single step may help but would still be racy (I had the case when testing manually where the delete would happen fast enough that the volume was still in "no state", not yet in Pending)
|
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye slaperche-scality. |
Component:
operator
Context:
When trying to delete a volume whose creation was in progress, the volume stay stuck in Pending state forever.
Summary:
Don't exit the reconciliation loop too early, to let
deployVolumemove the creation process forward.Acceptance criteria:
A new test was added, so the CI should be green.
Otherwise, you can test with
kubectl apply -f volume.yaml && sleep 1 && kubectl delete -f volume.yaml=> it should work.Closes: #2409