Refactor snapshot processing & streamline data flow

Author: sujeet01

api/storage/v1alpha1/common.go

@@ -8,7 +8,7 @@ import corev1 "k8s.io/api/core/v1"
 const (
 	VolumeVolumePoolRefNameField     = "spec.volumePoolRef.name"
 	VolumeVolumeClassRefNameField    = "spec.volumeClassRef.name"
-	VolumeVolumeSnapshotRefNameField = "spec.volumeDataSource.volumeSnapshotRef.name"
+	VolumeVolumeSnapshotRefNameField = "spec.volumeSnapshotRef.name"
 
 	BucketBucketPoolRefNameField  = "spec.bucketPoolRef.name"
 	BucketBucketClassRefNameField = "spec.bucketClassRef.name"

broker/volumebroker/server/volume_create.go

@@ -18,8 +18,6 @@ import (
 	volumepoolletv1alpha1 "github.com/ironcore-dev/ironcore/poollet/volumepoollet/api/v1alpha1"
 
 	utilsmaps "github.com/ironcore-dev/ironcore/utils/maps"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -32,7 +30,7 @@ type AggregateIronCoreVolume struct {
 	AccessSecret     *corev1.Secret
 }
 
-func (s *Server) getIronCoreVolumeConfig(ctx context.Context, volume *iri.Volume) (*AggregateIronCoreVolume, error) {
+func (s *Server) getIronCoreVolumeConfig(_ context.Context, volume *iri.Volume) (*AggregateIronCoreVolume, error) {
 	var volumePoolRef *corev1.LocalObjectReference
 	if s.volumePoolName != "" {
 		volumePoolRef = &corev1.LocalObjectReference{
@@ -69,31 +67,18 @@ func (s *Server) getIronCoreVolumeConfig(ctx context.Context, volume *iri.Volume
 	)
 
 	var image string
-	image = volume.Spec.Image
-	if dataSource := volume.Spec.VolumeDataSource; dataSource != nil && dataSource.ImageDataSource != nil {
-		image = dataSource.ImageDataSource.Image
-	}
-
 	var volumeSnapshotRef *corev1.LocalObjectReference
-	if dataSource := volume.Spec.VolumeDataSource; dataSource != nil && dataSource.SnapshotDataSource != nil {
-		volumeSnapshot, err := s.findVolumeSnapshotBySnapshotID(ctx, dataSource.SnapshotDataSource.SnapshotId)
-		if err != nil {
-			return nil, fmt.Errorf("error finding volume snapshot by snapshot ID %s: %w", dataSource.SnapshotDataSource.SnapshotId, err)
-		}
-		if volumeSnapshot == nil {
-			return nil, status.Errorf(codes.NotFound, "volume snapshot with ID %s not found", dataSource.SnapshotDataSource.SnapshotId)
-		}
-		if volumeSnapshot.Status.State != storagev1alpha1.VolumeSnapshotStateReady {
-			switch volumeSnapshot.Status.State {
-			case storagev1alpha1.VolumeSnapshotStatePending:
-				return nil, status.Errorf(codes.FailedPrecondition, "volume snapshot %s is not ready (state: %s)", volumeSnapshot.Name, volumeSnapshot.Status.State)
-			case storagev1alpha1.VolumeSnapshotStateFailed:
-				return nil, status.Errorf(codes.Internal, "volume snapshot %s has failed (state: %s)", volumeSnapshot.Name, volumeSnapshot.Status.State)
-			default:
-				return nil, status.Errorf(codes.FailedPrecondition, "volume snapshot %s is not ready (state: %s)", volumeSnapshot.Name, volumeSnapshot.Status.State)
-			}
+
+	image = volume.Spec.Image // TODO: Remove this once volume.Spec.Image is deprecated
+
+	if dataSource := volume.Spec.VolumeDataSource; dataSource != nil {
+		switch {
+		case dataSource.SnapshotDataSource != nil:
+			volumeSnapshotRef = &corev1.LocalObjectReference{Name: dataSource.SnapshotDataSource.SnapshotId}
+			image = "" // TODO: Remove this once volume.Spec.Image is deprecated
+		case dataSource.ImageDataSource != nil:
+			image = dataSource.ImageDataSource.Image
 		}
-		volumeSnapshotRef = &corev1.LocalObjectReference{Name: volumeSnapshot.Name}
 	}
 
 	ironcoreVolume := &storagev1alpha1.Volume{
@@ -111,7 +96,7 @@ func (s *Server) getIronCoreVolumeConfig(ctx context.Context, volume *iri.Volume
 			Resources: corev1alpha1.ResourceList{
 				corev1alpha1.ResourceStorage: *resource.NewQuantity(volume.Spec.Resources.StorageBytes, resource.DecimalSI),
 			},
-			Image:              image, // TODO: Remove this once the image field is deprecated
+			Image:              image, // TODO: Remove this once volume.Spec.Image is deprecated
 			ImagePullSecretRef: nil,   // TODO: Fill if necessary
 			Encryption:         encryption,
 			VolumeDataSource: storagev1alpha1.VolumeDataSource{
@@ -137,21 +122,6 @@ func getOSImageIfPresent(image string) *string {
 	return &image
 }
 
-func (s *Server) findVolumeSnapshotBySnapshotID(ctx context.Context, snapshotID string) (*storagev1alpha1.VolumeSnapshot, error) {
-	volumeSnapshotList := &storagev1alpha1.VolumeSnapshotList{}
-	if err := s.client.List(ctx, volumeSnapshotList, client.InNamespace(s.namespace)); err != nil {
-		return nil, fmt.Errorf("error listing volume snapshots: %w", err)
-	}
-
-	for _, volumeSnapshot := range volumeSnapshotList.Items {
-		if volumeSnapshot.Status.SnapshotID == snapshotID {
-			return &volumeSnapshot, nil
-		}
-	}
-
-	return nil, nil
-}
-
 func (s *Server) createIronCoreVolume(ctx context.Context, log logr.Logger, volume *AggregateIronCoreVolume) (retErr error) {
 	c, cleanup := s.setupCleaner(ctx, log, &retErr)
 	defer cleanup()

broker/volumebroker/server/volume_create_test.go

@@ -14,11 +14,8 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -104,7 +101,7 @@ var _ = Describe("CreateVolume", func() {
 					},
 					VolumeDataSource: &iri.VolumeDataSource{
 						SnapshotDataSource: &iri.SnapshotDataSource{
-							SnapshotId: "test-snapshot-id",
+							SnapshotId: "test-snapshot",
 						},
 					},
 				},
@@ -124,119 +121,4 @@ var _ = Describe("CreateVolume", func() {
 		Expect(ironcoreVolume.Spec.VolumeDataSource.VolumeSnapshotRef.Name).To(Equal("test-snapshot"))
 	})
 
-	It("should return error if snapshot is not found", func(ctx SpecContext) {
-		By("creating a volume with non-existent snapshot data source")
-		res, err := srv.CreateVolume(ctx, &iri.CreateVolumeRequest{
-			Volume: &iri.Volume{
-				Metadata: &irimeta.ObjectMetadata{
-					Labels: map[string]string{
-						volumepoolletv1alpha1.VolumeUIDLabel: "foobar",
-					},
-				},
-				Spec: &iri.VolumeSpec{
-					Class: volumeClass.Name,
-					Resources: &iri.VolumeResources{
-						StorageBytes: 100,
-					},
-					VolumeDataSource: &iri.VolumeDataSource{
-						SnapshotDataSource: &iri.SnapshotDataSource{
-							SnapshotId: "non-existent-snapshot-id",
-						},
-					},
-				},
-			},
-		})
-
-		Expect(err).To(HaveOccurred())
-		Expect(res).To(BeNil())
-		Expect(status.Code(err)).To(Equal(codes.NotFound))
-	})
-
-	It("should return error if snapshot is in pending state", func(ctx SpecContext) {
-		By("creating a volume snapshot in pending state")
-		volumeSnapshot := &storagev1alpha1.VolumeSnapshot{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: ns.Name,
-				Name:      "pending-snapshot",
-			},
-			Spec: storagev1alpha1.VolumeSnapshotSpec{
-				VolumeRef: &corev1.LocalObjectReference{Name: "source-volume"},
-			},
-			Status: storagev1alpha1.VolumeSnapshotStatus{
-				State:      storagev1alpha1.VolumeSnapshotStatePending,
-				SnapshotID: "pending-snapshot-id",
-			},
-		}
-		Expect(k8sClient.Create(ctx, volumeSnapshot)).To(Succeed())
-
-		By("creating a volume with pending snapshot data source")
-		res, err := srv.CreateVolume(ctx, &iri.CreateVolumeRequest{
-			Volume: &iri.Volume{
-				Metadata: &irimeta.ObjectMetadata{
-					Labels: map[string]string{
-						volumepoolletv1alpha1.VolumeUIDLabel: "foobar",
-					},
-				},
-				Spec: &iri.VolumeSpec{
-					Class: volumeClass.Name,
-					Resources: &iri.VolumeResources{
-						StorageBytes: 100,
-					},
-					VolumeDataSource: &iri.VolumeDataSource{
-						SnapshotDataSource: &iri.SnapshotDataSource{
-							SnapshotId: "pending-snapshot-id",
-						},
-					},
-				},
-			},
-		})
-
-		Expect(err).To(HaveOccurred())
-		Expect(res).To(BeNil())
-		Expect(status.Code(err)).To(Equal(codes.FailedPrecondition))
-	})
-
-	It("should return error if snapshot is in failed state", func(ctx SpecContext) {
-		By("creating a volume snapshot in failed state")
-		volumeSnapshot := &storagev1alpha1.VolumeSnapshot{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: ns.Name,
-				Name:      "failed-snapshot",
-			},
-			Spec: storagev1alpha1.VolumeSnapshotSpec{
-				VolumeRef: &corev1.LocalObjectReference{Name: "source-volume"},
-			},
-			Status: storagev1alpha1.VolumeSnapshotStatus{
-				State:      storagev1alpha1.VolumeSnapshotStateFailed,
-				SnapshotID: "failed-snapshot-id",
-			},
-		}
-		Expect(k8sClient.Create(ctx, volumeSnapshot)).To(Succeed())
-
-		By("creating a volume with failed snapshot data source")
-		res, err := srv.CreateVolume(ctx, &iri.CreateVolumeRequest{
-			Volume: &iri.Volume{
-				Metadata: &irimeta.ObjectMetadata{
-					Labels: map[string]string{
-						volumepoolletv1alpha1.VolumeUIDLabel: "foobar",
-					},
-				},
-				Spec: &iri.VolumeSpec{
-					Class: volumeClass.Name,
-					Resources: &iri.VolumeResources{
-						StorageBytes: 100,
-					},
-					VolumeDataSource: &iri.VolumeDataSource{
-						SnapshotDataSource: &iri.SnapshotDataSource{
-							SnapshotId: "failed-snapshot-id",
-						},
-					},
-				},
-			},
-		})
-
-		Expect(err).To(HaveOccurred())
-		Expect(res).To(BeNil())
-		Expect(status.Code(err)).To(Equal(codes.Internal))
-	})
 })