csi: delete stale subvolumesnapshot #307
Conversation
pkg/filesystem/subvolume.go
Outdated
| if len(splitSnapshotHandle) < 6 { | ||
| return nil | ||
| } | ||
| snapshotHandleId := splitSnapshotHandle[len(splitSnapshotHandle)-1] |
There was a problem hiding this comment.
There was a problem hiding this comment.
yes, it just compares the snap-id ignore the prefix which matches with snapshothandle.
yati1998
force-pushed
the
subvolid
branch
4 times, most recently
from
b9b5b61 to
bd4100e
Compare
|
@subhamkrai do you have any idea why is the ci failing, it seems it is failing due to some plugin test. Can you please check? |
pkg/filesystem/subvolume.go
Outdated
| if err != nil || snapshotcontentname == "" { | ||
| logging.Info("No snapshot content found for snapshot %s: %s", snap, err) | ||
| return "" | ||
| } |
There was a problem hiding this comment.
am not sure this check is correct, we can consider the error !=nil as not found right?
There was a problem hiding this comment.
This looks like a wrong assumption to consider err!=nil as not found and continue , can you please check this again
There was a problem hiding this comment.
Having this assumption will leave a stale entry in the omap, can you please check, if there is any error when running this command it should be logged as Fatal not just info and return empty struct which will skip omap deletion operation.
There was a problem hiding this comment.
okay checked that, added the required change
There was a problem hiding this comment.
@Madhu-1 please do review again.
yati1998
force-pushed
the
subvolid
branch
2 times, most recently
from
4de207a to
2b4598b
Compare
There was a problem hiding this comment.
The CI is failing:
go: github.com/kubernetes-csi/external-snapshotter/client/[email protected] requires go >= 1.22.0 (running go 1.21.11)
make: *** [Makefile:19: build] Error 1
yes, checked that, we need to update the go.mod here itself or send another PR and merge it first? |
@yati1998 I created this issue #309 please rebase your PR once that issue is closed. |
pkg/filesystem/subvolume.go
Outdated
| snapshotHandles := make(map[string]snapshotInfo) | ||
| for _, snap := range snapList.Items { | ||
| driverName := snap.Spec.Driver | ||
| if snap.Status != nil && snap.Status.SnapshotHandle != nil && strings.Contains(driverName, "cephfs") { |
There was a problem hiding this comment.
cephfs.csi.ceph.com use this one
pkg/filesystem/subvolume.go
Outdated
| if err != nil || snapshotcontentname == "" { | ||
| logging.Info("No snapshot content found for snapshot %s: %s", snap, err) | ||
| return "" | ||
| } |
There was a problem hiding this comment.
This looks like a wrong assumption to consider err!=nil as not found and continue , can you please check this again
@Madhu-1 I think this is fine, either there is no omap or some error running the command, anywhich ways we are printing the error too. we do the same for pv also, looks fine to me. |
pkg/filesystem/subvolume.go
Outdated
| args := []string{"getomapval", snapomapval, "csi.snapname", "-p", poolName, "--namespace", "csi", "/dev/stdout"} | ||
| cmd := "rados" | ||
| snapshotcontentname, err := runCommand(ctx, clientsets, operatorNamespace, cephClusterNamespace, cmd, args) | ||
| if snapshotcontentname == "" { |
There was a problem hiding this comment.
this should also have err==nil check to work as expected or else if its err!=nil we will have it snapshotcontentname set to empty string
|
@yati1998 Could you look at the CI? Not sure at a glance what the failure is. |
|
@yati1998 Please rebase it to resolve the merge conflicts |
this commit checks if there are any stale volumesnapshots, if yes, it deletes the snapshots Signed-off-by: yati1998 <[email protected]>
can you please review now? |
this commit checks if there are any stale
volumesnapshots, if yes, it deletes the
snapshots