Volume Group Snapshots NAS Implementation

Author: torirevilla

core/orchestrator_core.go

@@ -4191,9 +4191,11 @@ func (o *TridentOrchestrator) CreateGroupSnapshot(
 		}
 
 		if err = currentInfo.Validate(); err != nil {
-			return nil, fmt.Errorf("invalid storage target for backend %s", b.Name())
+			// If the storage target is invalid, we cannot create a group snapshot, snapshotter should not retry.
+			return nil, errors.InvalidInputError("invalid storage target for backend %s", b.Name())
 		} else if !currentInfo.IsShared(groupSnapshotTarget) {
-			return nil, fmt.Errorf("storage target is not the same for all backends")
+			// If the storage target is not shared, we cannot create a group snapshot, snapshotter should not retry.
+			return nil, errors.InvalidInputError("storage target is not the same for all backends")
 		}
 
 		// Identify and remove duplicates for any source volumes that are shared across multiple backends.
@@ -4225,6 +4227,7 @@ func (o *TridentOrchestrator) CreateGroupSnapshot(
 	}()
 
 	// Create the group snapshot
+	// TODO (TRID-16891): Do post-processing within the driver function; san-economy needs work
 	groupSnapshot, snapshots, err = groupSnapshotter.CreateGroupSnapshot(ctx, groupSnapshotConfig, groupSnapshotTarget)
 	if err != nil {
 		if errors.IsMaxLimitReachedError(err) {
@@ -4234,19 +4237,6 @@ func (o *TridentOrchestrator) CreateGroupSnapshot(
 		return nil, fmt.Errorf("failed to create group snapshot %s: %v", groupSnapshotConfig.ID(), err)
 	}
 
-	// Do post-processing here; nas and san should be no-ops; san-economy needs work
-	postProcessedSnaps, err := groupSnapshotter.PostProcessGroupSnapshot(ctx, groupSnapshotTarget, groupSnapshot)
-	if err != nil {
-		return nil, fmt.Errorf("failed to post process group snapshot %s: %v",
-			groupSnapshotConfig.ID(), err)
-	}
-	if postProcessedSnaps != nil {
-		// This would be the SAN-Economy case
-		for _, postProcessedSnap := range postProcessedSnaps {
-			groupSnapshot.SnapshotIDs = append(groupSnapshot.SnapshotIDs, postProcessedSnap.ID())
-		}
-	}
-
 	// Save references to new group snapshot and each snapshot
 	if err = o.storeClient.AddGroupSnapshot(ctx, groupSnapshot); err != nil {
 		return nil, err

frontend/csi/plugin.go

@@ -532,6 +532,8 @@ func (p *Plugin) getCSIErrorForOrchestratorError(err error) error {
 		return status.Error(codes.DeadlineExceeded, err.Error())
 	} else if ok, errPtr := errors.HasResourceExhaustedError(err); ok && errPtr != nil {
 		return status.Error(codes.ResourceExhausted, err.Error())
+	} else if errors.IsInvalidInputError(err) {
+		return status.Error(codes.InvalidArgument, err.Error())
 	} else {
 		return status.Error(codes.Unknown, err.Error())
 	}

mocks/mock_storage/mock_storage.go

@@ -106,9 +106,6 @@ type GroupSnapshotter interface {
 	CreateGroupSnapshot(
 		ctx context.Context, config *GroupSnapshotConfig, target *GroupSnapshotTargetInfo,
 	) (*GroupSnapshot, []*Snapshot, error)
-	PostProcessGroupSnapshot(
-		ctx context.Context, targetInfo *GroupSnapshotTargetInfo, groupSnapshot *GroupSnapshot,
-	) ([]*Snapshot, error)
 }
 
 // StateGetter provides a common interface for backends that support polling backend for state information.
@@ -1020,18 +1017,6 @@ func (b *StorageBackend) CreateGroupSnapshot(ctx context.Context, config *GroupS
 	return snapshotter.CreateGroupSnapshot(ctx, config, target)
 }
 
-func (b *StorageBackend) PostProcessGroupSnapshot(
-	ctx context.Context, targetInfo *GroupSnapshotTargetInfo, groupSnapshot *GroupSnapshot,
-) ([]*Snapshot, error) {
-	snapshotter, ok := b.driver.(GroupSnapshotter)
-	if !ok {
-		return nil, errors.UnsupportedError(
-			fmt.Sprintf("group snapshot is not supported for backend of type %v", b.driver.Name()))
-	}
-
-	return snapshotter.PostProcessGroupSnapshot(ctx, targetInfo, groupSnapshot)
-}
-
 const (
 	BackendRename = iota
 	InvalidVolumeAccessInfoChange

storage/backend.go

@@ -1165,13 +1165,6 @@ func (d *StorageDriver) CreateGroupSnapshot(
 	return storage.NewGroupSnapshot(config, snapshotIDs, creationTime), groupedSnapshots, nil
 }
 
-func (d *StorageDriver) PostProcessGroupSnapshot(ctx context.Context, targetInfo *storage.GroupSnapshotTargetInfo,
-	groupSnapshot *storage.GroupSnapshot,
-) ([]*storage.Snapshot, error) {
-	// no-op for fake driver
-	return nil, nil
-}
-
 func (d *StorageDriver) Get(_ context.Context, name string) error {
 	_, ok := d.Volumes[name]
 	if !ok {

storage_drivers/fake/fake.go

@@ -4058,6 +4058,141 @@ func createFlexvolSnapshot(
 	}, nil
 }
 
+// GetGroupSnapshotTarget returns a set of information about the target of a group snapshot.
+// This information is used to gather information in a consistent way across storage drivers.
+func GetGroupSnapshotTarget(
+	ctx context.Context, volConfigs []*storage.VolumeConfig, config *drivers.OntapStorageDriverConfig,
+	client api.OntapAPI,
+) (*storage.GroupSnapshotTargetInfo, error) {
+	fields := LogFields{
+		"Method": "GetGroupSnapshotTarget",
+		"Type":   "ontap_common",
+	}
+	Logd(ctx, config.StorageDriverName,
+		config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> GetGroupSnapshotTarget")
+	defer Logd(ctx, config.StorageDriverName,
+		config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< GetGroupSnapshotTarget")
+
+	targetType := PersonalityUnified
+	if config.Flags != nil && config.Flags[FlagPersonality] != "" {
+		targetType = config.Flags[FlagPersonality]
+	}
+
+	targetUUID := client.GetSVMUUID()
+
+	// Construct a set of unique source volume IDs to volume names to configs for the group snapshot.
+	targetVolumes := make(storage.GroupSnapshotTargetVolumes, 0)
+	for _, volumeConfig := range volConfigs {
+		volumeName := volumeConfig.Name
+		internalVolName := volumeConfig.InternalName
+
+		// If the specified volume doesn't exist, return error
+		if volExists, err := client.VolumeExists(ctx, internalVolName); err != nil {
+			return nil, fmt.Errorf("error checking for existing volume: %v", err)
+		} else if !volExists {
+			return nil, errors.NotFoundError("volume %s does not exist", internalVolName)
+		}
+
+		if targetVolumes[internalVolName] == nil {
+			targetVolumes[internalVolName] = make(map[string]*storage.VolumeConfig)
+		}
+
+		// For other drivers such as the SAN-Eco driver, this will be a flexvol name -> pv name -> volume config.
+		targetVolumes[internalVolName][volumeName] = volumeConfig
+	}
+
+	return storage.NewGroupSnapshotTargetInfo(targetType, targetUUID, targetVolumes), nil
+}
+
+func CreateGroupSnapshot(
+	ctx context.Context, groupSnapshotConfig *storage.GroupSnapshotConfig, target *storage.GroupSnapshotTargetInfo,
+	driverConfig *drivers.OntapStorageDriverConfig, client api.OntapAPI, sizeGetter func(context.Context, string) (int, error),
+) (*storage.GroupSnapshot, []*storage.Snapshot, error) {
+	fields := LogFields{
+		"Method": "CreateGroupSnapshot",
+		"Type":   "ontap_common",
+	}
+	Logd(ctx, driverConfig.StorageDriverName,
+		driverConfig.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> CreateGroupSnapshot")
+	defer Logd(ctx, driverConfig.StorageDriverName,
+		driverConfig.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< CreateGroupSnapshot")
+
+	// Assign an internal name at the driver level.
+	groupSnapshotConfig.InternalName = groupSnapshotConfig.ID()
+	groupName := groupSnapshotConfig.ID()
+	snapName, err := storage.ConvertGroupSnapshotID(groupName)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	// Track the individual snapshots for each internalVolumeName config in the config snapshot target.
+	groupedSnapshots := make([]*storage.Snapshot, 0)
+	snapshotIDs := make([]string, 0)
+
+	// Look at all target source internalVolumes and build a list of snapshots.
+	// The sourceVolumeID here correlates to a parent FlexVolume.
+	// The list of constituentVolumes is a set of internalVolumes that exist under the source internalVolumeName ID.
+	internalVolumes := make([]string, 0, len(target.GetVolumes()))
+	for vol := range target.GetVolumes() {
+		internalVolumes = append(internalVolumes, vol)
+	}
+
+	// After we create the group snapshot and there is no error,
+	// if we fail for another reason we need to return the group snapshot so each snapshot is cleaned up.
+	if err = client.ConsistencyGroupSnapshot(ctx, snapName, internalVolumes); err != nil {
+		return nil, nil, err
+	}
+
+	var returnErr error
+	dateCreated := ""
+	for _, internalVolumeName := range internalVolumes {
+		snap, err := client.VolumeSnapshotInfo(ctx, snapName, internalVolumeName)
+		if err != nil {
+			returnErr = errors.Join(returnErr, err)
+		}
+		if dateCreated == "" {
+			dateCreated = snap.CreateTime
+		} else if dateCreated != snap.CreateTime {
+			Logc(ctx).Debugf("Snapshots in group '%s' created at different times: '%s' vs '%s'",
+				groupName, dateCreated, snap.CreateTime)
+		}
+
+		size, err := sizeGetter(ctx, internalVolumeName)
+		if err != nil {
+			returnErr = errors.Join(returnErr, fmt.Errorf("error reading volume size: %v", err))
+			continue
+		}
+
+		var snapVolumeName string
+		for volName := range target.GetVolumes()[internalVolumeName] {
+			snapVolumeName = volName
+
+			// Create a snapshot config and object for each constituent snapshot for the group.
+			snapConfig := &storage.SnapshotConfig{
+				Name:               snapName,
+				InternalName:       snapName,
+				VolumeInternalName: internalVolumeName,
+				VolumeName:         snapVolumeName,
+				ImportNotManaged:   false,
+				GroupSnapshotName:  groupName,
+			}
+
+			snapshot := &storage.Snapshot{
+				Config:    snapConfig,
+				Created:   snap.CreateTime,
+				SizeBytes: int64(size),
+				State:     storage.SnapshotStateOnline,
+			}
+
+			groupedSnapshots = append(groupedSnapshots, snapshot)
+			snapshotIDs = append(snapshotIDs, snapshot.ID())
+		}
+	}
+
+	// Create the group snapshot object. This is a logical grouping of the constituent snapshots.
+	return storage.NewGroupSnapshot(groupSnapshotConfig, snapshotIDs, dateCreated), groupedSnapshots, returnErr
+}
+
 // cloneFlexvol creates a volume clone
 func cloneFlexvol(
 	ctx context.Context, cloneVolConfig *storage.VolumeConfig, labels string, split bool, config *drivers.OntapStorageDriverConfig,

storage_drivers/ontap/ontap_common.go

@@ -1280,7 +1280,7 @@ func (d *NASStorageDriver) RestoreSnapshot(
 	return RestoreSnapshot(ctx, snapConfig, &d.Config, d.API)
 }
 
-// DeleteSnapshot creates a snapshot of a volume.
+// DeleteSnapshot deletes a snapshot of a volume.
 func (d *NASStorageDriver) DeleteSnapshot(
 	ctx context.Context, snapConfig *storage.SnapshotConfig, _ *storage.VolumeConfig,
 ) error {
@@ -1311,6 +1311,34 @@ func (d *NASStorageDriver) DeleteSnapshot(
 	return nil
 }
 
+// GetGroupSnapshotTarget returns a set of information about the target of a group snapshot.
+// This information is used to gather information in a consistent way across storage drivers.
+func (d *NASStorageDriver) GetGroupSnapshotTarget(
+	ctx context.Context, volConfigs []*storage.VolumeConfig,
+) (*storage.GroupSnapshotTargetInfo, error) {
+	fields := LogFields{
+		"Method": "GetGroupSnapshotTarget",
+		"Type":   "NASStorageDriver",
+	}
+	Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> GetGroupSnapshotTarget")
+	defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< GetGroupSnapshotTarget")
+
+	return GetGroupSnapshotTarget(ctx, volConfigs, &d.Config, d.API)
+}
+
+func (d *NASStorageDriver) CreateGroupSnapshot(
+	ctx context.Context, config *storage.GroupSnapshotConfig, target *storage.GroupSnapshotTargetInfo,
+) (*storage.GroupSnapshot, []*storage.Snapshot, error) {
+	fields := LogFields{
+		"Method": "CreateGroupSnapshot",
+		"Type":   "NASStorageDriver",
+	}
+	Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> CreateGroupSnapshot")
+	defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< CreateGroupSnapshot")
+
+	return CreateGroupSnapshot(ctx, config, target, &d.Config, d.API, d.volumeUsedSize)
+}
+
 // Get tests for the existence of a volume
 func (d *NASStorageDriver) Get(ctx context.Context, name string) error {
 	fields := LogFields{"Method": "Get", "Type": "NASStorageDriver"}

storage_drivers/ontap/ontap_nas.go

@@ -5981,3 +5981,88 @@ func TestOntapStorageDriverGetMirrorTransferTime(t *testing.T) {
 	assert.NotNil(t, result, "received nil")
 	assert.NoError(t, err, "received error")
 }
+
+func TestOntapNasVolumeGroupSnapshot(t *testing.T) {
+	ctx := context.Background()
+
+	mockAPI, driver := newMockOntapNASDriver(t)
+	mockAPI.EXPECT().SVMName().AnyTimes().Return("SVM1")
+
+	groupSnapshotConfig := &storage.GroupSnapshotConfig{
+		Name:         "groupsnapshot-1234",
+		InternalName: "groupsnapshot-1234",
+		VolumeNames:  []string{"vol1", "vol2"},
+	}
+	storageVols := storage.GroupSnapshotTargetVolumes{
+		"trident_vol1": {
+			"vol1": &storage.VolumeConfig{Name: "vol1"},
+		},
+		"trident_vol2": {
+			"vol2": &storage.VolumeConfig{Name: "vol2"},
+		},
+	}
+	targetInfo := &storage.GroupSnapshotTargetInfo{
+		StorageType:    "unified",
+		StorageUUID:    "12345",
+		StorageVolumes: storageVols,
+	}
+	storageVolNames := []string{"trident_vol1", "trident_vol2"}
+	snapName, _ := storage.ConvertGroupSnapshotID(groupSnapshotConfig.Name)
+	snapInfoResult := api.Snapshot{CreateTime: "1"}
+
+	mockAPI.EXPECT().ConsistencyGroupSnapshot(ctx, snapName, gomock.InAnyOrder(storageVolNames)).Return(nil).Times(1)
+
+	mockAPI.EXPECT().VolumeSnapshotInfo(ctx, snapName, gomock.Any()).Return(snapInfoResult, nil).Times(2)
+	mockAPI.EXPECT().VolumeUsedSize(ctx, gomock.Any()).Return(1073741824, nil).Times(2)
+
+	groupSnapshot, snapshots, err := driver.CreateGroupSnapshot(ctx, groupSnapshotConfig, targetInfo)
+
+	assert.Equal(t, groupSnapshot.ID(), groupSnapshotConfig.ID())
+	assert.Equal(t, groupSnapshot.GetVolumeNames(), groupSnapshotConfig.GetVolumeNames())
+
+	for _, snap := range snapshots {
+		assert.Contains(t, groupSnapshot.GetSnapshotIDs(), snap.ID())
+	}
+
+	assert.NoError(t, err, "Group snapshot creation failed")
+}
+
+func TestOntapNasVolumeGroupTarget(t *testing.T) {
+	ctx := context.Background()
+
+	mockAPI, driver := newMockOntapNASDriver(t)
+	mockAPI.EXPECT().SVMName().AnyTimes().Return("SVM1")
+
+	volumeConfigs := []*storage.VolumeConfig{
+		{
+			Name:         "vol1",
+			InternalName: "trident_vol1",
+		},
+		{
+			Name:         "vol2",
+			InternalName: "trident_vol2",
+		},
+	}
+
+	storageVols := storage.GroupSnapshotTargetVolumes{
+		"trident_vol1": {
+			"vol1": &storage.VolumeConfig{Name: "vol1", InternalName: "trident_vol1"},
+		},
+		"trident_vol2": {
+			"vol2": &storage.VolumeConfig{Name: "vol2", InternalName: "trident_vol2"},
+		},
+	}
+	expectedTargetInfo := &storage.GroupSnapshotTargetInfo{
+		StorageType:    "Unified",
+		StorageUUID:    "12345",
+		StorageVolumes: storageVols,
+	}
+
+	mockAPI.EXPECT().GetSVMUUID().Return("12345").Times(1)
+	mockAPI.EXPECT().VolumeExists(ctx, gomock.Any()).Return(true, nil).Times(2)
+
+	targetInfo, err := driver.GetGroupSnapshotTarget(ctx, volumeConfigs)
+
+	assert.Equal(t, targetInfo, expectedTargetInfo)
+	assert.NoError(t, err, "Volume group target failed")
+}