csi: add check for pending clone #311
Conversation
yati1998
requested review from
Madhu-1,
travisn and
subhamkrai
and removed request for
Madhu-1 and
travisn
| state, err := getSubvolumeState(ctx, clientsets, operatorNamespace, clusterNamespace, fs.Name, sv.Name, svg.Name) | ||
| // subvolume info returns error in case of pending clone or if it is not ready | ||
| // it is suggested to delete the pvc before deleting the subvolume. | ||
| if err != nil { |
There was a problem hiding this comment.
Considering the err!=nil as pending is not the right assumption, it can fail for other reasons as well. please add a valid check to look for specific strings in error
| logging.Info("Subvolume info not found for: %q", err) | ||
| logging.Info("Please delete the pending pv if any before deleting the subvolume %s", sv.Name) | ||
| continue | ||
| } |
There was a problem hiding this comment.
what if the error is not nil and not EAGAIN?
can you please test this and see if we are getting EAGAIN here to make sure we are good 👍🏻
There was a problem hiding this comment.
yatipadia:kubectl-rook-ceph$ ./bin/kubectl-rook-ceph subvolume ls
Filesystem Subvolume SubvolumeGroup State
myfs csi-vol-9e760985-c5cd-46d1-8484-8ecfa1e3d6bc csi in-use
myfs csi-vol-5d13c38b-2263-46bc-826b-6c0df2ad4bcb csi in-use
Error: Error EAGAIN: subvolume 'csi-vol-71bb4f78-1373-4b7e-8400-ea5fd96a22dc' is not ready for operation info
. failed to run command. command terminated with exit code 11%!(EXTRA string=failed to get subvolume info)
Info: Subvolume info not found for: "Error EAGAIN: subvolume 'csi-vol-71bb4f78-1373-4b7e-8400-ea5fd96a22dc' is not ready for operation info\n. failed to run command. command terminated with exit code 11"
Info: Please delete the pending pv if any before deleting the
Here's the output if the clone is in pending state , the info command is not ready incase the subvolume is not properly created, so this should be fine.
There was a problem hiding this comment.
@yati1998 in case of err !=nil and error is not syscall.EAGAIN we need to log it as error and let the user run the tool again, we should not ignore errors unless we know why we are doing like that
There was a problem hiding this comment.
please review it again, thank you @Madhu-1
pkg/filesystem/subvolume.go
Outdated
| if checkSnapshot(ctx, clientsets, operatorNamespace, clusterNamespace, fs.Name, sv.Name, svg.Name) { | ||
| status = staleWithSnapshot | ||
| // check for snapshot only if the subvolume info doesn't return any error. | ||
| if err == nil { |
There was a problem hiding this comment.
why we need this check here? i assume err should be handled above not here
There was a problem hiding this comment.
in case there is error running subvolume info command, we shouldn't check for the snapshot also, that would just be an addition to run time.
There was a problem hiding this comment.
okay, after adding continue to above case, we no more need this check as in case of err it will not go further. Thanks for notifying.
pkg/filesystem/subvolume.go
Outdated
| if err != nil { | ||
| if errors.Is(err, syscall.EAGAIN) { | ||
| logging.Info("Subvolume info not found for: %q %q", sv.Name, err) | ||
| logging.Info("Please delete the pending pv if any before deleting the subvolume %s", sv.Name) |
There was a problem hiding this comment.
they also need to scale down the cephfs csi deployment to avoid stale resources isn't it?
There was a problem hiding this comment.
added the log @Madhu-1
yati1998
force-pushed
the
pending-lone
branch
2 times, most recently
from
8ba12c2 to
2f80851
Compare
Incase any pvc is in pending state and the backend subvolume is create, it will be counted as stale. this commit adds the error check to make sure user doesn't delete the subvolume if it the pvc is in pending state. Signed-off-by: yati1998 <[email protected]>
Incase any pvc is in pending state and the backend subvolume is create, it will be counted as stale.
this commit adds the error check to make sure user doesn't delete the subvolume if it the pvc is in
pending state.