Repository: kubernetes-sigs/gcp-filestore-csi-driver
Pull request #454 (2023-03-16 19:42:10)

cherry pick #447 and #445 into release-1.4
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line:
>
> /kind api-change
> /kind bug
> /kind cleanup
> /kind design
> /kind documentation
> /kind failing-test
> /kind feature
> /kind flake

**What this PR does / why we need it**:

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
N/A
```



Patch:
diff --git a/cloudbuild.yaml b/cloudbuild.yaml
index d1b3b968..33cda234 100644
--- a/cloudbuild.yaml
+++ b/cloudbuild.yaml
@@ -6,7 +6,7 @@
 # To promote release images, see https://github.com/kubernetes/k8s.io/tree/main/k8s.gcr.io/images/k8s-staging-cloud-provider-gcp.
 
 # This must be specified in seconds. If omitted, defaults to 600s (10 mins).
-timeout: 1200s
+timeout: 1800s
 # This prevents errors if you don't use both _GIT_TAG and _PULL_BASE_REF,
 # or any new substitutions added in the future.
 options:
diff --git a/pkg/cloud_provider/file/fake.go b/pkg/cloud_provider/file/fake.go
index 8cf82a86..d76457d5 100644
--- a/pkg/cloud_provider/file/fake.go
+++ b/pkg/cloud_provider/file/fake.go
@@ -164,8 +164,9 @@ func (manager *fakeServiceManager) CreateBackup(ctx context.Context, obj *Servic
 
 	backupSource := fmt.Sprintf("projects/%s/locations/%s/instances/%s", obj.Project, obj.Location, obj.Name)
 	if backupInfo, ok := manager.backups[backupUri]; ok {
-		if backupInfo.SourceVolumeHandle != backupSource {
-			return nil, fmt.Errorf("Mismatch in source volume handle for existing snapshot")
+		if backupInfo.SourceInstance != backupSource && backupInfo.SourceShare != obj.Volume.Name {
+			// TODO: format the right info
+			return nil, fmt.Errorf("Mismatch in source for existing snapshot %v", backupInfo)
 		}
 		return backupInfo.Backup, nil
 	}
@@ -179,8 +180,9 @@ func (manager *fakeServiceManager) CreateBackup(ctx context.Context, obj *Servic
 		CapacityGb:      defaultCapacityGb,
 	}
 	manager.backups[backupUri] = &BackupInfo{
-		Backup:             backupToCreate,
-		SourceVolumeHandle: backupSource,
+		Backup:         backupToCreate,
+		SourceInstance: backupSource,
+		SourceShare:    obj.Volume.Name,
 	}
 	return backupToCreate, nil
 }
diff --git a/pkg/cloud_provider/file/file.go b/pkg/cloud_provider/file/file.go
index 0018c29c..8712e175 100644
--- a/pkg/cloud_provider/file/file.go
+++ b/pkg/cloud_provider/file/file.go
@@ -108,8 +108,9 @@ type Network struct {
 }
 
 type BackupInfo struct {
-	Backup             *filev1beta1.Backup
-	SourceVolumeHandle string
+	Backup         *filev1beta1.Backup
+	SourceInstance string
+	SourceShare    string
 }
 
 type Service interface {
@@ -506,8 +507,9 @@ func (manager *gcfsServiceManager) GetBackup(ctx context.Context, backupUri stri
 		return nil, err
 	}
 	return &BackupInfo{
-		Backup:             backup,
-		SourceVolumeHandle: backup.SourceInstance,
+		Backup:         backup,
+		SourceInstance: backup.SourceInstance,
+		SourceShare:    backup.SourceFileShare,
 	}, nil
 }
 
diff --git a/pkg/csi_driver/controller.go b/pkg/csi_driver/controller.go
index 353cf696..29e91d2d 100644
--- a/pkg/csi_driver/controller.go
+++ b/pkg/csi_driver/controller.go
@@ -862,12 +862,12 @@ func (s *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSn
 			return nil, status.Error(codes.Internal, err.Error())
 		}
 	} else {
-		backupSourceCSIHandle, err := util.BackupVolumeSourceToCSIVolumeHandle(backupInfo.SourceVolumeHandle)
+		backupSourceCSIHandle, err := util.BackupVolumeSourceToCSIVolumeHandle(backupInfo.SourceInstance, backupInfo.SourceShare)
 		if err != nil {
-			return nil, status.Errorf(codes.Internal, "Cannot determine volume handle from back source %s", backupInfo.SourceVolumeHandle)
+			return nil, status.Errorf(codes.Internal, "Cannot determine volume handle from back source instance %s, share %s", backupInfo.SourceInstance, backupInfo.SourceShare)
 		}
 		if backupSourceCSIHandle != volumeID {
-			return nil, status.Errorf(codes.AlreadyExists, "Backup already exists with a different source volume %s, input source volume %s", backupInfo.SourceVolumeHandle, volumeID)
+			return nil, status.Errorf(codes.AlreadyExists, "Backup already exists with a different source volume %s, input source volume %s", backupSourceCSIHandle, volumeID)
 		}
 		// Check if backup is in the process of getting created.
 		if backupInfo.Backup.State == "CREATING" || backupInfo.Backup.State == "FINALIZING" {
diff --git a/pkg/util/util.go b/pkg/util/util.go
index 2e16a287..1a2b0ccc 100644
--- a/pkg/util/util.go
+++ b/pkg/util/util.go
@@ -199,12 +199,12 @@ func GetBackupLocation(params map[string]string) string {
 	return location
 }
 
-func BackupVolumeSourceToCSIVolumeHandle(backupVolumeSource string) (string, error) {
-	splitId := strings.Split(backupVolumeSource, "/")
+func BackupVolumeSourceToCSIVolumeHandle(sourceInstance, sourceShare string) (string, error) {
+	splitId := strings.Split(sourceInstance, "/")
 	if len(splitId) != volumeTotalElements {
-		return "", fmt.Errorf("Failed to get id components. Expected 'projects/{project}/location/{zone}/instances/{name}'. Got: %s", backupVolumeSource)
+		return "", fmt.Errorf("Failed to get id components. Expected 'projects/{project}/location/{zone}/instances/{name}'. Got: %s", sourceInstance)
 	}
-	return fmt.Sprintf("modeInstance/%s/%s/vol1", splitId[3], splitId[5]), nil
+	return fmt.Sprintf("modeInstance/%s/%s/%s", splitId[3], splitId[5], sourceShare), nil
 }
 
 // Multishare util functions.


Test patch:
diff --git a/pkg/csi_driver/controller_test.go b/pkg/csi_driver/controller_test.go
index 177c8348..3e0189f6 100644
--- a/pkg/csi_driver/controller_test.go
+++ b/pkg/csi_driver/controller_test.go
@@ -1048,22 +1048,99 @@ func ValidateExpectedError(t *testing.T, errResp <-chan error, operationUnblocke
 }
 
 func TestCreateSnapshot(t *testing.T) {
+	type BackupInfo struct {
+		s              *file.ServiceInstance
+		backupName     string
+		backupLocation string
+	}
+	backupName := "mybackup"
+	project := "test-project"
+	zone := "us-central1-c"
+	region := "us-central1"
+	instanceName := "myinstance"
+	shareName := "myshare"
 	cases := []struct {
-		name      string
-		req       *csi.CreateSnapshotRequest
-		expectErr bool
+		name          string
+		req           *csi.CreateSnapshotRequest
+		initialBackup *BackupInfo
+		expectErr     bool
 	}{
+		// Failure test cases
 		{
 			name: "Create snapshot request for multishare backed volumes",
 			req: &csi.CreateSnapshotRequest{
-				SourceVolumeId: "modeMultishare/mysc/myproject/location/instancename/sharename",
+				SourceVolumeId: "modeMultishare/mysc/test-project/location/myinstance/myshare",
 			},
 			expectErr: true,
 		},
+		{
+			name: "Existing backup found, with different volume Id (source zonal filestore instance), error expected",
+			req: &csi.CreateSnapshotRequest{
+				SourceVolumeId: "modeInstance/us-central1-c/myinstance1/myshare",
+				Name:           backupName,
+				Parameters: map[string]string{
+					util.VolumeSnapshotTypeKey: "backup",
+				},
+			},
+			initialBackup: &BackupInfo{
+				s: &file.ServiceInstance{
+					Project:  project,
+					Location: zone,
+					Name:     instanceName,
+					Volume: file.Volume{
+						Name: shareName,
+					},
+				},
+				backupName:     backupName,
+				backupLocation: region,
+			},
+			expectErr: true,
+		},
+		// Success test cases
+		{
+			name: "Existing backup found, with same source volume Id (source zonal filestore instance)",
+			req: &csi.CreateSnapshotRequest{
+				SourceVolumeId: "modeInstance/us-central1-c/myinstance/myshare",
+				Name:           backupName,
+				Parameters: map[string]string{
+					util.VolumeSnapshotTypeKey: "backup",
+				},
+			},
+			initialBackup: &BackupInfo{
+				s: &file.ServiceInstance{
+					Project:  project,
+					Location: zone,
+					Name:     instanceName,
+					Volume: file.Volume{
+						Name: shareName,
+					},
+				},
+				backupName:     backupName,
+				backupLocation: region,
+			},
+		},
 	}
 	for _, test := range cases {
-		cs := initTestController(t)
-		_, err := cs.CreateSnapshot(context.TODO(), test.req)
+		fileService, err := file.NewFakeService()
+		if err != nil {
+			t.Fatalf("failed to initialize GCFS service: %v", err)
+		}
+
+		cloudProvider, err := cloud.NewFakeCloud()
+		if err != nil {
+			t.Fatalf("Failed to get cloud provider: %v", err)
+		}
+		cs := newControllerServer(&controllerServerConfig{
+			driver:      initTestDriver(t),
+			fileService: fileService,
+			cloud:       cloudProvider,
+			volumeLocks: util.NewVolumeLocks(),
+		})
+
+		if test.initialBackup != nil {
+			fileService.CreateBackup(context.TODO(), test.initialBackup.s, test.initialBackup.backupName, test.initialBackup.backupLocation)
+		}
+		_, err = cs.CreateSnapshot(context.TODO(), test.req)
 		if !test.expectErr && err != nil {
 			t.Errorf("test %q failed: %v", test.name, err)
 		}
diff --git a/test/run_e2e.sh b/test/run_e2e.sh
index d37a6439..f95403eb 100755
--- a/test/run_e2e.sh
+++ b/test/run_e2e.sh
@@ -5,4 +5,4 @@ set -x
 
 readonly PKGDIR=sigs.k8s.io/gcp-filestore-csi-driver
 
-go test --mod=vendor --timeout 20m --v=true "${PKGDIR}/test/e2e/tests" --logtostderr --run-in-prow=true --delete-instances=true
+go test --mod=vendor --timeout 35m --v=true "${PKGDIR}/test/e2e/tests" --logtostderr --run-in-prow=true --delete-instances=true