Merge pull request #948 from Shruthi-1MN/fix-930

File Share ACL create and delete services updating file share status are corrected - Fix #930

pkg/api/util/db.go

@@ -109,19 +109,36 @@ func CreateFileShareAclDBEntry(ctx *c.Context, in *model.FileShareAclSpec) (*mod
 		return nil, errors.New(errMsg)
 	}
 
-	// Store the fileshare meadata into database.
 	return db.C.CreateFileShareAcl(ctx, in)
 }
 
 func DeleteFileShareAclDBEntry(ctx *c.Context, in *model.FileShareAclSpec) error {
-	// If fileshare id is invalid, it would mean that fileshare snapshot
+	// If fileshare id is invalid, it would mean that fileshare acl
 	// creation failed before the create method in storage driver was
 	// called, and delete its db entry directly.
-	if _, err := db.C.GetFileShare(ctx, in.FileShareId); err != nil {
+	validStatus := []string{model.FileShareAclAvailable, model.FileShareAclError,
+		model.FileShareAclErrorDeleting}
+	if !utils.Contained(in.Status, validStatus) {
+		errMsg := fmt.Sprintf("only the file share acl with the status available, error, error_deleting can be deleted, the fileshare status is %s", in.Status)
+		log.Error(errMsg)
+		return errors.New(errMsg)
+	}
+
+	// If fileshare id is invalid, it would mean that file share acl creation failed before the create method
+	// in storage driver was called, and delete its db entry directly.
+	_, err := db.C.GetFileShare(ctx, in.FileShareId)
+	if err != nil {
 		if err := db.C.DeleteFileShareAcl(ctx, in.Id); err != nil {
-			log.Error("when delete fileshare acl in db:", err)
+			log.Error("failed delete fileshare acl in db:", err)
 			return err
 		}
+		return nil
+	}
+
+	in.Status = model.FileShareAclDeleting
+	_, err = db.C.UpdateFileShareAcl(ctx, in)
+	if err != nil {
+		return err
 	}
 	return nil
 }

pkg/api/util/db_test.go

@@ -486,7 +486,7 @@ func TestCreateFileShareDBEntry(t *testing.T) {
 		ProfileId:   "b3585ebe-c42c-120g-b28e-f373746a71ca",
 		Status:      model.FileShareCreating,
 	}
-
+	
 	t.Run("Everything should work well", func(t *testing.T) {
 		mockClient := new(dbtest.Client)
 		mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil)
@@ -554,4 +554,131 @@ func TestCreateFileShareDBEntry(t *testing.T) {
 		expectedError := "invalid fileshare description and it has some special characters"
 		assertTestResult(t, err.Error(), expectedError)
 	})
-}
+}
+
+func TestDeleteFileShareDBEntry(t *testing.T) {
+	var fileshare = &model.FileShareSpec{
+		BaseModel: &model.BaseModel{
+			Id: "bd5b12a8-a101-11e7-941e-d77981b584d8",
+		},
+		Status:    model.FileShareAvailable,
+		ProfileId: "3769855c-a102-11e7-b772-17b880d2f537",
+		PoolId:    "3762355c-a102-11e7-b772-17b880d2f537",
+	}
+	var in = &model.FileShareSpec{
+		BaseModel: &model.BaseModel{
+			Id: "bd5b12a8-a101-11e7-941e-d77981b584d8",
+		},
+		Status:    model.FileShareInUse,
+		ProfileId: "3769855c-a102-11e7-b772-17b880d2f537",
+		PoolId:    "3762355c-a102-11e7-b772-17b880d2f537",
+	}
+	t.Run("FileShare to be deleted should not be in-use", func(t *testing.T) {
+		fileshare.Status = model.FileShareInUse
+		mockClient := new(dbtest.Client)
+		mockClient.On("ListSnapshotsByShareId", context.NewAdminContext(), fileshare.Id).Return(nil, nil)
+		mockClient.On("ListFileShareAclsByShareId", context.NewAdminContext(), fileshare.Id).Return(nil, nil)
+		mockClient.On("UpdateFileShare", context.NewAdminContext(), in).Return(nil, nil)
+		mockClient.On("DeleteFileShare", context.NewAdminContext(), fileshare.Id).Return(nil)
+		db.C = mockClient
+
+		err := DeleteFileShareDBEntry(context.NewAdminContext(), fileshare)
+		expectedError := fmt.Sprintf("only the fileshare with the status available, error, errorDeleting, can be deleted, the fileshare status is %s", in.Status)
+		assertTestResult(t, err.Error(), expectedError)
+	})
+
+	var sampleSnapshots = []*model.FileShareSnapshotSpec{&SampleShareSnapshots[0]}
+	t.Run("FileShare should not be deleted if it has dependent snapshots", func(t *testing.T) {
+		//in.Status = model.FileShareAvailable
+		fileshare.Status = model.FileShareAvailable
+		mockClient := new(dbtest.Client)
+		mockClient.On("ListSnapshotsByShareId", context.NewAdminContext(), fileshare.Id).Return(sampleSnapshots, nil)
+		db.C = mockClient
+
+		err := DeleteFileShareDBEntry(context.NewAdminContext(), fileshare)
+		expectedError := fmt.Sprintf("file share %s can not be deleted, because it still has snapshots", in.Id)
+		assertTestResult(t, err.Error(), expectedError)
+	})
+
+	var sampleAcls = []*model.FileShareAclSpec{&SampleSharesAcl[0]}
+	t.Run("FileShare should not be deleted if it has dependent acls", func(t *testing.T) {
+		//in.Status = model.FileShareAvailable
+		fileshare.Status = model.FileShareAvailable
+		mockClient := new(dbtest.Client)
+		mockClient.On("ListSnapshotsByShareId", context.NewAdminContext(), fileshare.Id).Return(nil, nil)
+		mockClient.On("ListFileShareAclsByShareId", context.NewAdminContext(), fileshare.Id).Return(sampleAcls, nil)
+		db.C = mockClient
+
+		err := DeleteFileShareDBEntry(context.NewAdminContext(), fileshare)
+		expectedError := fmt.Sprintf("file share %s can not be deleted, because it still has acls", in.Id)
+		assertTestResult(t, err.Error(), expectedError)
+	})
+
+	t.Run("FileShare deletion when it is available", func(t *testing.T) {
+		in.Status = model.FileShareDeleting
+		//fileshare.Status = model.FileShareAvailable
+		mockClient := new(dbtest.Client)
+		mockClient.On("ListSnapshotsByShareId", context.NewAdminContext(), fileshare.Id).Return(nil, nil)
+		mockClient.On("ListFileShareAclsByShareId", context.NewAdminContext(), fileshare.Id).Return(nil, nil)
+		mockClient.On("UpdateFileShare", context.NewAdminContext(), in).Return(nil, nil)
+		mockClient.On("DeleteFileShare", context.NewAdminContext(), fileshare.Id).Return(nil)
+		db.C = mockClient
+
+		err := DeleteFileShareDBEntry(context.NewAdminContext(), fileshare)
+		if err != nil {
+			t.Errorf("failed to delete fileshare, err is %v\n", err)
+		}
+	})
+}
+
+func TestDeleteFileShareAclDBEntry(t *testing.T) {
+	var in = &model.FileShareAclSpec{
+		BaseModel: &model.BaseModel{
+			Id: "d2975ebe-d82c-430f-b28e-f373746a71ca",
+		},
+		Status:           model.FileShareAclAvailable,
+		ProfileId:        "3769855c-a102-11e7-b772-17b880d2f537",
+		FileShareId:      "bd5b12a8-a101-11e7-941e-d77981b584d8",
+		Type:             "ip",
+		AccessTo:         "10.21.23.10",
+		AccessCapability: []string{"Read", "Write"},
+	}
+	var out = &model.FileShareAclSpec{
+		BaseModel: &model.BaseModel{
+			Id: "d2975ebe-d82c-430f-b28e-f373746a71ca",
+		},
+		Status:           model.FileShareAclDeleting,
+		ProfileId:        "3769855c-a102-11e7-b772-17b880d2f537",
+		FileShareId:      "bd5b12a8-a101-11e7-941e-d77981b584d8",
+		Type:             "ip",
+		AccessTo:         "10.21.23.10",
+		AccessCapability: []string{"Read", "Write"},
+	}
+
+	t.Run("FileShareAcl to be deleted should not be in-use", func(t *testing.T) {
+		in.Status = model.FileShareAclInUse
+		mockClient := new(dbtest.Client)
+		mockClient.On("GetFileShare", context.NewAdminContext(), in.FileShareId).Return(nil, nil)
+		mockClient.On("DeleteFileShareAcl", context.NewAdminContext(), in.Id).Return(nil, nil)
+		mockClient.On("UpdateFileShareAcl", context.NewAdminContext(), in).Return(nil, nil)
+		db.C = mockClient
+
+		err := DeleteFileShareAclDBEntry(context.NewAdminContext(), in)
+		expectedError := fmt.Sprintf("only the file share acl with the status available, error, error_deleting can be deleted, the fileshare status is %s", in.Status)
+		assertTestResult(t, err.Error(), expectedError)
+	})
+
+	t.Run("FileShareAcl deletion when everything works fine", func(t *testing.T) {
+		mockClient := new(dbtest.Client)
+		in.Status = model.FileShareAclAvailable
+		mockClient.On("GetFileShare", context.NewAdminContext(), in.FileShareId).Return(&SampleFileShares[0], nil)
+		mockClient.On("DeleteFileShareAcl", context.NewAdminContext(), in.Id).Return(nil, nil)
+		mockClient.On("UpdateFileShareAcl", context.NewAdminContext(), in).Return(out, nil)
+		db.C = mockClient
+
+		err := DeleteFileShareAclDBEntry(context.NewAdminContext(), in)
+		if err != nil {
+			t.Errorf("failed delete fileshare acl in db:%v\n", err)
+		}
+	})
+}

pkg/controller/controller.go

@@ -953,13 +953,13 @@ func (c *Controller) CreateFileShareAcl(contx context.Context, opt *pb.CreateFil
 
 	fileshare, err := db.C.GetFileShare(ctx, opt.FileshareId)
 	if err != nil {
-		db.UpdateFileShareStatus(ctx, db.C, opt.Id, model.FileShareError)
+		db.UpdateFileShareAclStatus(ctx, db.C, opt.Id, model.FileShareAclError)
 		return pb.GenericResponseError(err), err
 	}
 
 	dockInfo, err := db.C.GetDockByPoolId(ctx, fileshare.PoolId)
 	if err != nil {
-		db.UpdateFileShareStatus(ctx, db.C, opt.Id, model.FileShareError)
+		db.UpdateFileShareAclStatus(ctx, db.C, opt.Id, model.FileShareAclError)
 		log.Error("when search supported dock resource:", err.Error())
 		return pb.GenericResponseError(err), err
 	}
@@ -970,12 +970,12 @@ func (c *Controller) CreateFileShareAcl(contx context.Context, opt *pb.CreateFil
 	result, err := c.fileshareController.CreateFileShareAcl((*pb.CreateFileShareAclOpts)(opt))
 	if err != nil {
 		// Change the status of the file share acl to error when the creation faild
-		defer db.UpdateFileShareStatus(ctx, db.C, fileshare.Id, model.FileShareError)
+		defer db.UpdateFileShareAclStatus(ctx, db.C, opt.Id, model.FileShareAclError)
 		log.Error("when create file share acl:", err.Error())
 		return pb.GenericResponseError(err), err
 	}
 
-	db.C.UpdateFileShareAcl(ctx, result)
+	db.C.UpdateStatus(ctx, result, model.FileShareAclAvailable)
 	return pb.GenericResponseResult(result), nil
 }
 
@@ -987,10 +987,13 @@ func (c *Controller) DeleteFileShareAcl(contx context.Context, opt *pb.DeleteFil
 
 	fileshare, err := db.C.GetFileShare(ctx, opt.FileshareId)
 	if err != nil {
+		defer db.UpdateFileShareAclStatus(ctx, db.C, opt.Id, model.FileShareAclErrorDeleting)
+		log.Error("when delete file share acl:", err.Error())
 		return pb.GenericResponseError(err), err
 	}
 	dockInfo, err := db.C.GetDockByPoolId(ctx, fileshare.PoolId)
 	if err != nil {
+		defer db.UpdateFileShareAclStatus(ctx, db.C, opt.Id, model.FileShareAclErrorDeleting)
 		log.Error("when search supported dock resource:", err.Error())
 		return pb.GenericResponseError(err), err
 	}
@@ -999,11 +1002,13 @@ func (c *Controller) DeleteFileShareAcl(contx context.Context, opt *pb.DeleteFil
 	opt.Name = fileshare.Name
 
 	if err = c.fileshareController.DeleteFileShareAcl((*pb.DeleteFileShareAclOpts)(opt)); err != nil {
-		// Change the status of the file share acl to error when the creation faild
+		// Change the status of the file share acl to error when the creation failed
+		defer db.UpdateFileShareAclStatus(ctx, db.C, opt.Id, model.FileShareAclErrorDeleting)
 		log.Error("when delete file share acl:", err.Error())
 		return pb.GenericResponseError(err), err
 	}
 	if err = db.C.DeleteFileShareAcl(ctx, opt.Id); err != nil {
+		defer db.UpdateFileShareAclStatus(ctx, db.C, opt.Id, model.FileShareAclErrorDeleting)
 		log.Error("error occurred in controller module when delete file share acl in db: ", err)
 		return pb.GenericResponseError(err), err
 	}

pkg/db/db.go

@@ -234,6 +234,11 @@ func UpdateFileShareSnapshotStatus(ctx *c.Context, client Client, snapID, status
 	return client.UpdateStatus(ctx, snap, status)
 }
 
+func UpdateFileShareAclStatus(ctx *c.Context, client Client, fileID, status string) error {
+	file, _ := client.GetFileShareAcl(ctx, fileID)
+	return client.UpdateStatus(ctx, file, status)
+}
+
 func UpdateVolumeStatus(ctx *c.Context, client Client, volID, status string) error {
 	vol, _ := client.GetVolume(ctx, volID)
 	return client.UpdateStatus(ctx, vol, status)

pkg/model/fileshare.go

@@ -39,6 +39,10 @@ type FileShareAclSpec struct {
 	// The description of the fileshare acl.
 	Description string `json:"description,omitempty"`
 
+	// The status of the fileshareacl.
+	// One of: "available", "error" etc.
+	Status string `json:"status,omitempty"`
+
 	// The uuid of the profile which the fileshare belongs to.
 	ProfileId string `json:"profileId,omitempty"`
 

pkg/model/status.go

@@ -33,6 +33,15 @@ const (
 	FileShareSnapErrorDeleting = "errorDeleting"
 )
 
+// fileshare acl status
+const (
+	FileShareAclAvailable     = "available"
+	FileShareAclDeleting      = "deleting"
+	FileShareAclError         = "error"
+	FileShareAclErrorDeleting = "errorDeleting"
+	FileShareAclInUse         = "in_Use"
+)
+
 // volume status
 const (
 	VolumeCreating       = "creating"

testutils/collection/data.go

@@ -244,7 +244,30 @@ var (
 			Description: "This is a sample Acl for testing",
 		},
 	}
-
+	SampleSharesAcl = []model.FileShareAclSpec{
+		{
+			BaseModel: &model.BaseModel{
+				Id: "d2975ebe-d82c-430f-b28e-f373746a71ca",
+			},
+			Status: "available",
+			FileShareId: "bd5b12a8-a101-11e7-941e-d77981b584d8",
+			ProfileId: "3769855c-a102-11e7-b772-17b880d2f537",
+			Type: "ip",
+			AccessTo: "10.21.23.10",
+			AccessCapability:[]string{"Read", "Write"},
+		},
+		{
+			BaseModel: &model.BaseModel{
+				Id: "1e643aca-4922-4b1a-bb98-4245054aeff4",
+			},
+			Status: "available",
+			FileShareId: "bd5b12a8-a101-11e7-941e-d77981b584d8",
+			ProfileId: "3769855c-a102-11e7-b772-17b880d2f537",
+			Type: "ip",
+			AccessTo: "101.21.23.10",
+			AccessCapability:[]string{"Read"},
+		},
+	}
 	SampleFileShareSnapshots = []model.FileShareSnapshotSpec{
 		{
 			BaseModel: &model.BaseModel{