Skip to content

RBD: check for volume group existence #5184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mergify merged 3 commits into from
Mar 27, 2025
Merged

RBD: check for volume group existence #5184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
60ea14e
cleanup: move internal/rbd/errors.go to internal/rbd/errors pacakge
iPraveenParihar Mar 3, 2025
727aad5
rbd: check for volume group existence
iPraveenParihar Mar 3, 2025
4d7e636
cleanup: move Destroy() method to journalledObject interface
iPraveenParihar Mar 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion internal/csi-addons/rbd/encryptionkeyrotation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"

"github.com/ceph/ceph-csi/internal/rbd"
rbderrors "github.com/ceph/ceph-csi/internal/rbd/errors"
Copy link
Copy Markdown
Member

@nixpanic nixpanic Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it makes sense to move common RBD errors to internal/rbd/types/errors.go? That is what I decided to do with #5221.

Copy link
Copy Markdown
Contributor Author

@iPraveenParihar iPraveenParihar Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, rbd/errors is more suitable than rbd/types (which should be just for the interface and struct definition?)

Copy link
Copy Markdown
Member

@nixpanic nixpanic Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is fine with me too. I just wanted to locate the errors that are documented at the types/interfaces together. I' can move it to internal/rbd/errors too.

"github.com/ceph/ceph-csi/internal/util"
"github.com/ceph/ceph-csi/internal/util/log"

Expand Down Expand Up @@ -68,7 +69,7 @@ func (ekrs *EncryptionKeyRotationServer) EncryptionKeyRotate(
rbdVol, err := mgr.GetVolumeByID(ctx, volID)
if err != nil {
switch {
case errors.Is(err, util.ErrImageNotFound):
case errors.Is(err, rbderrors.ErrImageNotFound):
err = status.Errorf(codes.NotFound, "volume ID %s not found", volID)
case errors.Is(err, util.ErrPoolNotFound):
log.ErrorLog(ctx, "failed to get backend volume for %s: %v", volID, err)
Expand Down
3 changes: 2 additions & 1 deletion internal/csi-addons/rbd/reclaimspace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (

csicommon "github.com/ceph/ceph-csi/internal/csi-common"
rbdutil "github.com/ceph/ceph-csi/internal/rbd"
rbderrors "github.com/ceph/ceph-csi/internal/rbd/errors"
"github.com/ceph/ceph-csi/internal/util"
"github.com/ceph/ceph-csi/internal/util/log"

Expand Down Expand Up @@ -84,7 +85,7 @@ func (rscs *ReclaimSpaceControllerServer) ControllerReclaimSpace(
defer rbdVol.Destroy(ctx)

err = rbdVol.Sparsify(ctx)
if errors.Is(err, rbdutil.ErrImageInUse) {
if errors.Is(err, rbderrors.ErrImageInUse) {
// FIXME: https://github.com/csi-addons/kubernetes-csi-addons/issues/406.
// treat sparsify call as no-op if volume is in use.
log.DebugLog(ctx, fmt.Sprintf("volume with ID %q is in use, skipping sparsify operation", volumeID))
Expand Down
29 changes: 15 additions & 14 deletions internal/csi-addons/rbd/replication.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
csicommon "github.com/ceph/ceph-csi/internal/csi-common"
"github.com/ceph/ceph-csi/internal/rbd"
corerbd "github.com/ceph/ceph-csi/internal/rbd"
rbderrors "github.com/ceph/ceph-csi/internal/rbd/errors"
"github.com/ceph/ceph-csi/internal/rbd/types"
"github.com/ceph/ceph-csi/internal/util"
"github.com/ceph/ceph-csi/internal/util/log"
Expand Down Expand Up @@ -651,7 +652,7 @@ func (rs *ReplicationServer) ResyncVolume(ctx context.Context,
sts, err := mirror.GetGlobalMirroringStatus(ctx)
if err != nil {
// the image gets recreated after issuing resync
if errors.Is(err, util.ErrImageNotFound) {
if errors.Is(err, rbderrors.ErrImageNotFound) {
// caller retries till RBD syncs an initial version of the image to
// report its status in the resync call. Ideally, this line will not
// be executed as the error would get returned due to getMirroringInfo
Expand Down Expand Up @@ -785,13 +786,13 @@ func getGRPCError(err error) error {
}

errorStatusMap := map[error]codes.Code{
util.ErrImageNotFound: codes.NotFound,
util.ErrPoolNotFound: codes.NotFound,
corerbd.ErrInvalidArgument: codes.InvalidArgument,
corerbd.ErrFlattenInProgress: codes.Aborted,
corerbd.ErrAborted: codes.Aborted,
corerbd.ErrFailedPrecondition: codes.FailedPrecondition,
corerbd.ErrUnavailable: codes.Unavailable,
rbderrors.ErrImageNotFound: codes.NotFound,
util.ErrPoolNotFound: codes.NotFound,
rbderrors.ErrInvalidArgument: codes.InvalidArgument,
rbderrors.ErrFlattenInProgress: codes.Aborted,
rbderrors.ErrAborted: codes.Aborted,
rbderrors.ErrFailedPrecondition: codes.FailedPrecondition,
rbderrors.ErrUnavailable: codes.Unavailable,
}

for e, code := range errorStatusMap {
Expand Down Expand Up @@ -835,7 +836,7 @@ func (rs *ReplicationServer) GetVolumeReplicationInfo(ctx context.Context,
log.ErrorLog(ctx, "failed to get volume with id %q: %v", volumeID, err)

switch {
case errors.Is(err, util.ErrImageNotFound):
case errors.Is(err, rbderrors.ErrImageNotFound):
err = status.Error(codes.NotFound, err.Error())
case errors.Is(err, util.ErrPoolNotFound):
err = status.Error(codes.NotFound, err.Error())
Expand Down Expand Up @@ -872,7 +873,7 @@ func (rs *ReplicationServer) GetVolumeReplicationInfo(ctx context.Context,
if err != nil {
log.ErrorLog(ctx, "failed to get status for mirror %q: %v", mirror, err)

if errors.Is(err, util.ErrImageNotFound) {
if errors.Is(err, rbderrors.ErrImageNotFound) {
return nil, status.Error(codes.Aborted, err.Error())
}

Expand All @@ -895,7 +896,7 @@ func (rs *ReplicationServer) GetVolumeReplicationInfo(ctx context.Context,
if err != nil {
log.ErrorLog(ctx, "failed to parse last sync info from %q: %v", description, err)

if errors.Is(err, corerbd.ErrLastSyncTimeNotFound) {
if errors.Is(err, rbderrors.ErrLastSyncTimeNotFound) {
return nil, status.Errorf(codes.NotFound, "failed to get last sync info: %v", err)
}

Expand Down Expand Up @@ -924,12 +925,12 @@ func getLastSyncInfo(ctx context.Context, description string) (*replication.GetV
var response replication.GetVolumeReplicationInfoResponse

if description == "" {
return nil, fmt.Errorf("empty description: %w", corerbd.ErrLastSyncTimeNotFound)
return nil, fmt.Errorf("empty description: %w", rbderrors.ErrLastSyncTimeNotFound)
}
log.DebugLog(ctx, "description: %s", description)
splittedString := strings.SplitN(description, ",", 2)
if len(splittedString) == 1 {
return nil, fmt.Errorf("no snapshot details: %w", corerbd.ErrLastSyncTimeNotFound)
return nil, fmt.Errorf("no snapshot details: %w", rbderrors.ErrLastSyncTimeNotFound)
}
type localStatus struct {
LocalSnapshotTime int64 `json:"local_snapshot_timestamp"`
Expand All @@ -946,7 +947,7 @@ func getLastSyncInfo(ctx context.Context, description string) (*replication.GetV
// If the json unmarsal is successful but the local snapshot time is 0, we
// need to consider it as an error as the LastSyncTime is required.
if localSnapInfo.LocalSnapshotTime == 0 {
return nil, fmt.Errorf("empty local snapshot timestamp: %w", corerbd.ErrLastSyncTimeNotFound)
return nil, fmt.Errorf("empty local snapshot timestamp: %w", rbderrors.ErrLastSyncTimeNotFound)
}
if localSnapInfo.LastSnapshotDuration != nil {
// converts localSnapshotDuration of type int64 to string format with
Expand Down
27 changes: 14 additions & 13 deletions internal/csi-addons/rbd/replication_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"time"

corerbd "github.com/ceph/ceph-csi/internal/rbd"
rbderrors "github.com/ceph/ceph-csi/internal/rbd/errors"
"github.com/ceph/ceph-csi/internal/rbd/types"
"github.com/ceph/ceph-csi/internal/util"

Expand Down Expand Up @@ -450,7 +451,7 @@ func TestValidateLastSyncInfo(t *testing.T) {
LastSyncDuration: nil,
LastSyncBytes: 0,
},
expectedErr: corerbd.ErrLastSyncTimeNotFound.Error(),
expectedErr: rbderrors.ErrLastSyncTimeNotFound.Error(),
},
{
name: "description without last_snapshot_bytes",
Expand All @@ -472,7 +473,7 @@ func TestValidateLastSyncInfo(t *testing.T) {
LastSyncTime: nil,
LastSyncBytes: 0,
},
expectedErr: corerbd.ErrLastSyncTimeNotFound.Error(),
expectedErr: rbderrors.ErrLastSyncTimeNotFound.Error(),
},
{
name: "description without last_snapshot_sync_seconds",
Expand Down Expand Up @@ -516,7 +517,7 @@ func TestValidateLastSyncInfo(t *testing.T) {
LastSyncTime: nil,
LastSyncBytes: 0,
},
expectedErr: corerbd.ErrLastSyncTimeNotFound.Error(),
expectedErr: rbderrors.ErrLastSyncTimeNotFound.Error(),
},
}
for _, tt := range tests {
Expand Down Expand Up @@ -567,23 +568,23 @@ func TestGetGRPCError(t *testing.T) {
}{
{
name: "InvalidArgument",
err: corerbd.ErrInvalidArgument,
expectedErr: status.Error(codes.InvalidArgument, corerbd.ErrInvalidArgument.Error()),
err: rbderrors.ErrInvalidArgument,
expectedErr: status.Error(codes.InvalidArgument, rbderrors.ErrInvalidArgument.Error()),
},
{
name: "Aborted",
err: corerbd.ErrAborted,
expectedErr: status.Error(codes.Aborted, corerbd.ErrAborted.Error()),
err: rbderrors.ErrAborted,
expectedErr: status.Error(codes.Aborted, rbderrors.ErrAborted.Error()),
},
{
name: "FailedPrecondition",
err: corerbd.ErrFailedPrecondition,
expectedErr: status.Error(codes.FailedPrecondition, corerbd.ErrFailedPrecondition.Error()),
err: rbderrors.ErrFailedPrecondition,
expectedErr: status.Error(codes.FailedPrecondition, rbderrors.ErrFailedPrecondition.Error()),
},
{
name: "Unavailable",
err: corerbd.ErrUnavailable,
expectedErr: status.Error(codes.Unavailable, corerbd.ErrUnavailable.Error()),
err: rbderrors.ErrUnavailable,
expectedErr: status.Error(codes.Unavailable, rbderrors.ErrUnavailable.Error()),
},
{
name: "InvalidError",
Expand All @@ -597,8 +598,8 @@ func TestGetGRPCError(t *testing.T) {
},
{
name: "ErrImageNotFound",
err: util.ErrImageNotFound,
expectedErr: status.Error(codes.NotFound, util.ErrImageNotFound.Error()),
err: rbderrors.ErrImageNotFound,
expectedErr: status.Error(codes.NotFound, rbderrors.ErrImageNotFound.Error()),
},
{
name: "ErrPoolNotFound",
Expand Down
6 changes: 3 additions & 3 deletions internal/csi-addons/rbd/volumegroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
"slices"

"github.com/ceph/ceph-csi/internal/rbd"
"github.com/ceph/ceph-csi/internal/rbd/group"
rbderrors "github.com/ceph/ceph-csi/internal/rbd/errors"
"github.com/ceph/ceph-csi/internal/rbd/types"
"github.com/ceph/ceph-csi/internal/util/log"

Expand Down Expand Up @@ -194,7 +194,7 @@ func (vs *VolumeGroupServer) DeleteVolumeGroup(
// resolve the volume group
vg, err := mgr.GetVolumeGroupByID(ctx, req.GetVolumeGroupId())
if err != nil {
if errors.Is(err, group.ErrRBDGroupNotFound) {
if errors.Is(err, rbderrors.ErrGroupNotFound) {
log.ErrorLog(ctx, "VolumeGroup %q doesn't exists", req.GetVolumeGroupId())

return &volumegroup.DeleteVolumeGroupResponse{}, nil
Expand Down Expand Up @@ -433,7 +433,7 @@ func (vs *VolumeGroupServer) ControllerGetVolumeGroup(
// resolve the volume group
vg, err := mgr.GetVolumeGroupByID(ctx, req.GetVolumeGroupId())
if err != nil {
if errors.Is(err, group.ErrRBDGroupNotFound) {
if errors.Is(err, rbderrors.ErrGroupNotFound) {
log.ErrorLog(ctx, "VolumeGroup %q doesn't exists", req.GetVolumeGroupId())

return nil, status.Errorf(
Expand Down
8 changes: 4 additions & 4 deletions internal/rbd/clone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
"fmt"
"strings"

"github.com/ceph/ceph-csi/internal/util"
rbderrors "github.com/ceph/ceph-csi/internal/rbd/errors"
"github.com/ceph/ceph-csi/internal/util/k8s"
"github.com/ceph/ceph-csi/internal/util/log"

Expand Down Expand Up @@ -56,7 +56,7 @@ func (rv *rbdVolume) checkCloneImage(ctx context.Context, parentVol *rbdVolume)
err := tempClone.checkSnapExists(snap)
if err != nil {
switch {
case errors.Is(err, ErrSnapNotFound):
case errors.Is(err, rbderrors.ErrSnapNotFound):
// as the snapshot is not present, create new snapshot, clone and
// don't delete the temporary snapshot
err = createRBDClone(ctx, tempClone, rv, snap, false)
Expand All @@ -66,7 +66,7 @@ func (rv *rbdVolume) checkCloneImage(ctx context.Context, parentVol *rbdVolume)

return true, nil

case errors.Is(err, util.ErrImageNotFound):
case errors.Is(err, rbderrors.ErrImageNotFound):
// as the temp clone does not exist,check snapshot exists on parent volume
// snapshot name is same as temporary clone image
snap.RbdImageName = tempClone.RbdImageName
Expand All @@ -76,7 +76,7 @@ func (rv *rbdVolume) checkCloneImage(ctx context.Context, parentVol *rbdVolume)
// create new resources for a cleaner approach
err = parentVol.deleteSnapshot(ctx, snap)
}
if errors.Is(err, ErrSnapNotFound) {
if errors.Is(err, rbderrors.ErrSnapNotFound) {
return false, nil
}

Expand Down
29 changes: 15 additions & 14 deletions internal/rbd/controllerserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"strconv"

csicommon "github.com/ceph/ceph-csi/internal/csi-common"
rbderrors "github.com/ceph/ceph-csi/internal/rbd/errors"
"github.com/ceph/ceph-csi/internal/util"
"github.com/ceph/ceph-csi/internal/util/k8s"
"github.com/ceph/ceph-csi/internal/util/log"
Expand Down Expand Up @@ -316,10 +317,10 @@ func buildCreateVolumeResponse(
// the input error types it expected to use only for CreateVolume as we need to
// return different GRPC codes for different functions based on the input.
func getGRPCErrorForCreateVolume(err error) error {
if errors.Is(err, ErrVolNameConflict) {
if errors.Is(err, rbderrors.ErrVolNameConflict) {
return status.Error(codes.AlreadyExists, err.Error())
}
if errors.Is(err, ErrFlattenInProgress) {
if errors.Is(err, rbderrors.ErrFlattenInProgress) {
return status.Error(codes.Aborted, err.Error())
}

Expand Down Expand Up @@ -434,7 +435,7 @@ func (cs *ControllerServer) CreateVolume(

err = cs.createBackingImage(ctx, cr, req.GetSecrets(), rbdVol, parentVol, rbdSnap, req.GetParameters())
if err != nil {
if errors.Is(err, ErrFlattenInProgress) {
if errors.Is(err, rbderrors.ErrFlattenInProgress) {
return nil, status.Error(codes.Aborted, err.Error())
}

Expand Down Expand Up @@ -592,7 +593,7 @@ func (cs *ControllerServer) repairExistingVolume(ctx context.Context, req *csi.C
func flattenTemporaryClonedImages(ctx context.Context, rbdVol *rbdVolume, cr *util.Credentials) error {
snapAndChildrenInfo, err := rbdVol.listSnapAndChildren()
if err != nil {
if errors.Is(err, util.ErrImageNotFound) {
if errors.Is(err, rbderrors.ErrImageNotFound) {
return status.Error(codes.InvalidArgument, err.Error())
}

Expand Down Expand Up @@ -855,7 +856,7 @@ func checkContentSource(
rbdSnap, err := genSnapFromSnapID(ctx, snapshotID, cr, req.GetSecrets())
if err != nil {
log.ErrorLog(ctx, "failed to get backend snapshot for %s: %v", snapshotID, err)
if !errors.Is(err, ErrSnapNotFound) {
if !errors.Is(err, rbderrors.ErrSnapNotFound) {
return nil, nil, status.Error(codes.Internal, err.Error())
}

Expand All @@ -875,7 +876,7 @@ func checkContentSource(
rbdvol, err := GenVolFromVolID(ctx, volID, cr, req.GetSecrets())
if err != nil {
log.ErrorLog(ctx, "failed to get backend image for %s: %v", volID, err)
if !errors.Is(err, util.ErrImageNotFound) {
if !errors.Is(err, rbderrors.ErrImageNotFound) {
return nil, nil, status.Error(codes.Internal, err.Error())
}

Expand Down Expand Up @@ -915,7 +916,7 @@ func (cs *ControllerServer) checkErrAndUndoReserve(
return &csi.DeleteVolumeResponse{}, nil
}

if errors.Is(err, util.ErrImageNotFound) {
if errors.Is(err, rbderrors.ErrImageNotFound) {
notFoundErr := rbdVol.ensureImageCleanup(ctx)
if notFoundErr != nil {
return nil, status.Errorf(codes.Internal, "failed to cleanup image %q: %v", rbdVol, notFoundErr)
Expand Down Expand Up @@ -990,7 +991,7 @@ func (cs *ControllerServer) DeleteVolume(
return nil, status.Error(codes.InvalidArgument, pErr.Error())
}
pErr = deleteMigratedVolume(ctx, pmVolID, cr)
if pErr != nil && !errors.Is(pErr, util.ErrImageNotFound) {
if pErr != nil && !errors.Is(pErr, rbderrors.ErrImageNotFound) {
return nil, status.Error(codes.Internal, pErr.Error())
}

Expand Down Expand Up @@ -1162,7 +1163,7 @@ func (cs *ControllerServer) CreateSnapshot(
}()
if err != nil {
switch {
case errors.Is(err, util.ErrImageNotFound):
case errors.Is(err, rbderrors.ErrImageNotFound):
err = status.Errorf(codes.NotFound, "source Volume ID %s not found", req.GetSourceVolumeId())
case errors.Is(err, util.ErrPoolNotFound):
log.ErrorLog(ctx, "failed to get backend volume for %s: %v", req.GetSourceVolumeId(), err)
Expand Down Expand Up @@ -1231,7 +1232,7 @@ func (cs *ControllerServer) CreateSnapshot(
return nil, status.Error(codes.Internal, err.Error())
}
defer func() {
if err != nil && !errors.Is(err, ErrFlattenInProgress) {
if err != nil && !errors.Is(err, rbderrors.ErrFlattenInProgress) {
errDefer := undoSnapReservation(ctx, rbdSnap, cr)
if errDefer != nil {
log.WarningLog(ctx, "failed undoing reservation of snapshot: %s %v", req.GetName(), errDefer)
Expand Down Expand Up @@ -1311,7 +1312,7 @@ func cloneFromSnapshot(
}

err = vol.flattenRbdImage(ctx, false, rbdHardMaxCloneDepth, rbdSoftMaxCloneDepth)
if errors.Is(err, ErrFlattenInProgress) {
if errors.Is(err, rbderrors.ErrFlattenInProgress) {
// if flattening is in progress, return error and do not cleanup
return nil, status.Error(codes.Internal, err.Error())
} else if err != nil {
Expand Down Expand Up @@ -1398,7 +1399,7 @@ func (cs *ControllerServer) doSnapshotClone(

defer func() {
if err != nil {
if !errors.Is(err, ErrFlattenInProgress) {
if !errors.Is(err, rbderrors.ErrFlattenInProgress) {
// cleanup clone and snapshot
errCleanUp := cleanUpSnapshot(ctx, cloneRbd, rbdSnap, cloneRbd)
if errCleanUp != nil {
Expand Down Expand Up @@ -1508,7 +1509,7 @@ func (cs *ControllerServer) DeleteSnapshot(

// if the error is ErrImageNotFound, We need to cleanup the image from
// trash and remove the metadata in OMAP.
if errors.Is(err, util.ErrImageNotFound) {
if errors.Is(err, rbderrors.ErrImageNotFound) {
log.UsefulLog(ctx, "cleaning up leftovers of snapshot %s: %v", snapshotID, err)

err = cleanUpImageAndSnapReservation(ctx, rbdSnap, cr)
Expand Down Expand Up @@ -1611,7 +1612,7 @@ func (cs *ControllerServer) ControllerExpandVolume(
rbdVol, err := genVolFromVolIDWithMigration(ctx, volID, cr, req.GetSecrets())
if err != nil {
switch {
case errors.Is(err, util.ErrImageNotFound):
case errors.Is(err, rbderrors.ErrImageNotFound):
err = status.Errorf(codes.NotFound, "volume ID %s not found", volID)
case errors.Is(err, util.ErrPoolNotFound):
log.ErrorLog(ctx, "failed to get backend volume for %s: %v", volID, err)
Expand Down
Loading