Skip to content

Commit

Permalink
Merge pull request #574 from judemars/cherry-pick-521-to-release-1.4
Browse files Browse the repository at this point in the history 
Cherry pick 521, 527 and 532 to release 1.4
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Aug 7, 2023
2 parents 0477988 + 8afa24d commit 21e4d52
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 64 deletions.
50 changes: 38 additions & 12 deletions pkg/cloud_provider/file/file.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"google.golang.org/api/googleapi"
"google.golang.org/api/option"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/klog/v2"
"sigs.k8s.io/gcp-filestore-csi-driver/pkg/util"
Expand Down Expand Up @@ -516,7 +517,8 @@ func (manager *gcfsServiceManager) GetBackup(ctx context.Context, backupUri stri
func (manager *gcfsServiceManager) CreateBackup(ctx context.Context, obj *ServiceInstance, backupName string, backupLocation string) (*filev1beta1.Backup, error) {
backupUri, region, err := CreateBackupURI(obj, backupName, backupLocation)
if err != nil {
return nil, err
klog.Errorf("Failed to create backup URI from given name %s and location %s, error: %v", backupName, backupLocation, err.Error())
return nil, status.Error(codes.InvalidArgument, err.Error())
}

backupSource := fmt.Sprintf("projects/%s/locations/%s/instances/%s", obj.Project, obj.Location, obj.Name)
Expand Down Expand Up @@ -634,15 +636,15 @@ func IsNotFoundErr(err error) bool {
return false
}

// IsUserError returns a pointer to the grpc error code that maps to the http
// isUserError returns a pointer to the grpc error code that maps to the http
// error code for the passed in user googleapi error. Returns nil if the
// given error is not a googleapi error caused by the user. The following
// http error codes are considered user errors:
// (1) http 400 Bad Request, returns grpc InvalidArgument,
// (2) http 403 Forbidden, returns grpc PermissionDenied,
// (3) http 404 Not Found, returns grpc NotFound
// (4) http 429 Too Many Requests, returns grpc ResourceExhausted
func IsUserError(err error) *codes.Code {
func isUserError(err error) *codes.Code {
// Upwrap the error
var apiErr *googleapi.Error
if !errors.As(err, &apiErr) {
Expand Down Expand Up @@ -686,7 +688,7 @@ func containsUserErrStr(err error) *codes.Code {
// isContextError returns a pointer to the grpc error code DeadlineExceeded
// if the passed in error contains the "context deadline exceeded" string and returns
// the grpc error code Canceled if the error contains the "context canceled" string.
func IsContextError(err error) *codes.Code {
func isContextError(err error) *codes.Code {
if err == nil {
return nil
}
Expand All @@ -701,22 +703,46 @@ func IsContextError(err error) *codes.Code {
return nil
}

// PollOpErrorCode returns a pointer to the grpc error code that maps to the http
// error code for passed in googleapi error. Returns grpc DeadlineExceeded if the
// given error contains the "context deadline exceeded" string. Returns the grpc Internal error
// code if the passed in error is neither a user error or a deadline exceeded error.
func PollOpErrorCode(err error) *codes.Code {
if errCode := IsUserError(err); errCode != nil {
// existingErrorCode returns a pointer to the grpc error code for the passed in error.
// Returns nil if the error is nil, or if the error cannot be converted to a grpc status.
func existingErrorCode(err error) *codes.Code {
if err == nil {
return nil
}

if status, ok := status.FromError(err); ok {
return util.ErrCodePtr(status.Code())
}
return nil
}

// CodeForError returns a pointer to the grpc error code that maps to the http
// error code for the passed in user googleapi error or context error. Returns
// codes.Internal if the given error is not a googleapi error caused by the user.
// The following http error codes are considered user errors:
// (1) http 400 Bad Request, returns grpc InvalidArgument,
// (2) http 403 Forbidden, returns grpc PermissionDenied,
// (3) http 404 Not Found, returns grpc NotFound
// (4) http 429 Too Many Requests, returns grpc ResourceExhausted
// The following errors are considered context errors:
// (1) "context deadline exceeded", returns grpc DeadlineExceeded,
// (2) "context canceled", returns grpc Canceled
func CodeForError(err error) *codes.Code {
if errCode := existingErrorCode(err); errCode != nil {
return errCode
}
if errCode := isUserError(err); errCode != nil {
return errCode
}
if errCode := IsContextError(err); errCode != nil {
if errCode := isContextError(err); errCode != nil {
return errCode
}

return util.ErrCodePtr(codes.Internal)
}

// This function returns the backup URI, the region that was picked to be the backup resource location and error.
func CreateBackupURI(obj *ServiceInstance, backupName, backupLocation string) (string, string, error) {
func CreateBackupURI(obj *ServiceInstance, backupName string, backupLocation string) (string, string, error) {
region, err := deduceRegion(obj, backupLocation)
if err != nil {
return "", "", err
Expand Down
14 changes: 10 additions & 4 deletions pkg/cloud_provider/file/file_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"google.golang.org/api/googleapi"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"sigs.k8s.io/gcp-filestore-csi-driver/pkg/util"
)

Expand Down Expand Up @@ -524,7 +525,7 @@ func TestIsContextError(t *testing.T) {
}

for _, test := range cases {
errCode := IsContextError(test.err)
errCode := isContextError(test.err)
if (test.expectedErrCode == nil) != (errCode == nil) {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
Expand All @@ -534,7 +535,7 @@ func TestIsContextError(t *testing.T) {
}
}

func TestPollOpErrorCode(t *testing.T) {
func TestCodeForError(t *testing.T) {
cases := []struct {
name string
err error
Expand Down Expand Up @@ -575,10 +576,15 @@ func TestPollOpErrorCode(t *testing.T) {
err: fmt.Errorf("got error: %w", &googleapi.Error{Code: http.StatusNotFound}),
expectedErrCode: util.ErrCodePtr(codes.NotFound),
},
{
name: "status error with Aborted error code",
err: status.Error(codes.Aborted, "aborted error"),
expectedErrCode: util.ErrCodePtr(codes.Aborted),
},
}

for _, test := range cases {
errCode := PollOpErrorCode(test.err)
errCode := CodeForError(test.err)
if (test.expectedErrCode == nil) != (errCode == nil) {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
Expand Down Expand Up @@ -647,7 +653,7 @@ func TestIsUserError(t *testing.T) {
}

for _, test := range cases {
errCode := IsUserError(test.err)
errCode := isUserError(test.err)
if (test.expectedErrCode == nil) != (errCode == nil) {
t.Errorf("test %v failed: got %v, expected %v", test.name, errCode, test.expectedErrCode)
}
Expand Down
53 changes: 17 additions & 36 deletions pkg/csi_driver/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,7 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu
_, err = s.config.fileService.GetBackup(ctx, id)
if err != nil {
klog.Errorf("Failed to get volume %v source snapshot %v: %v", name, id, err.Error())
if errCode := file.IsUserError(err); errCode != nil {
return nil, status.Error(*errCode, err.Error())
}
return nil, status.Errorf(codes.Internal, "Failed to get snapshot %v: %v", id, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}
sourceSnapshotId = id
}
Expand All @@ -201,10 +198,7 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu
filer, err := s.config.fileService.GetInstance(ctx, newFiler)
// No error is returned if the instance is not found during CreateVolume.
if err != nil && !file.IsNotFoundErr(err) {
if errCode := file.IsUserError(err); errCode != nil {
return nil, status.Error(*errCode, err.Error())
}
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}

if filer != nil {
Expand Down Expand Up @@ -266,10 +260,7 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu
}
if createErr != nil {
klog.Errorf("Create volume for volume Id %s failed: %v", volumeID, createErr.Error())
if errCode := file.IsUserError(createErr); errCode != nil {
return nil, status.Error(*errCode, createErr.Error())
}
return nil, status.Error(codes.Internal, createErr.Error())
return nil, status.Error(*file.CodeForError(createErr), createErr.Error())
}
}
resp := &csi.CreateVolumeResponse{Volume: s.fileInstanceToCSIVolume(filer, modeInstance, sourceSnapshotId)}
Expand Down Expand Up @@ -363,10 +354,7 @@ func (s *controllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolu
if file.IsNotFoundErr(err) {
return &csi.DeleteVolumeResponse{}, nil
}
if errCode := file.IsUserError(err); errCode != nil {
return nil, status.Error(*errCode, err.Error())
}
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}

if filer.State == "DELETING" {
Expand All @@ -376,7 +364,7 @@ func (s *controllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolu
err = s.config.fileService.DeleteInstance(ctx, filer)
if err != nil {
klog.Errorf("Delete volume for volume Id %s failed: %v", volumeID, err.Error())
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}

klog.Infof("DeleteVolume succeeded for volume %v", volumeID)
Expand All @@ -403,10 +391,7 @@ func (s *controllerServer) ValidateVolumeCapabilities(ctx context.Context, req *
filer.Project = s.config.cloud.Project
newFiler, err := s.config.fileService.GetInstance(ctx, filer)
if err != nil && !file.IsNotFoundErr(err) {
if errCode := file.IsUserError(err); errCode != nil {
return nil, status.Error(*errCode, err.Error())
}
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}
if newFiler == nil {
return nil, status.Errorf(codes.NotFound, "volume %v doesn't exist", volumeID)
Expand Down Expand Up @@ -660,10 +645,7 @@ func (s *controllerServer) ControllerExpandVolume(ctx context.Context, req *csi.
filer.Project = s.config.cloud.Project
filer, err = s.config.fileService.GetInstance(ctx, filer)
if err != nil {
if errCode := file.IsUserError(err); errCode != nil {
return nil, status.Error(*errCode, err.Error())
}
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}
if filer.State != "READY" {
return nil, fmt.Errorf("lolume %q is not yet ready, current state %q", volumeID, filer.State)
Expand All @@ -679,7 +661,7 @@ func (s *controllerServer) ControllerExpandVolume(ctx context.Context, req *csi.

hasPendingOps, err := s.config.fileService.HasOperations(ctx, filer, "update", false /* done */)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}

if hasPendingOps {
Expand All @@ -689,7 +671,7 @@ func (s *controllerServer) ControllerExpandVolume(ctx context.Context, req *csi.
filer.Volume.SizeBytes = reqBytes
newfiler, err := s.config.fileService.ResizeInstance(ctx, filer)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}

klog.Infof("Controller expand volume succeeded for volume %v, new size(bytes): %v", volumeID, newfiler.Volume.SizeBytes)
Expand Down Expand Up @@ -849,14 +831,16 @@ func (s *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSn
}

// Check for existing snapshot
backupUri, _, err := file.CreateBackupURI(filer, req.Name, util.GetBackupLocation(req.GetParameters()))
backupLocation := util.GetBackupLocation(req.GetParameters())
backupUri, _, err := file.CreateBackupURI(filer, req.Name, backupLocation)
if err != nil {
return nil, err
klog.Errorf("Failed to create backup URI from given name %s and location %s, error: %v", req.Name, backupLocation, err.Error())
return nil, status.Error(codes.InvalidArgument, err.Error())
}
backupInfo, err := s.config.fileService.GetBackup(ctx, backupUri)
if err != nil {
if !file.IsNotFoundErr(err) {
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}
} else {
backupSourceCSIHandle, err := util.BackupVolumeSourceToCSIVolumeHandle(backupInfo.SourceInstance, backupInfo.SourceShare)
Expand Down Expand Up @@ -893,10 +877,7 @@ func (s *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSn
if err != nil {
klog.Errorf("Create snapshot for volume Id %s failed: %v", volumeID, err.Error())
if err != nil {
if errCode := file.IsUserError(err); errCode != nil {
return nil, status.Error(*errCode, err.Error())
}
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}
}
tp, err := util.ParseTimestamp(backupObj.CreateTime)
Expand Down Expand Up @@ -940,7 +921,7 @@ func (s *controllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteSn
klog.Infof("Volume snapshot with ID %v not found", id)
return &csi.DeleteSnapshotResponse{}, nil
}
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}

if backupInfo.Backup.State == "DELETING" {
Expand All @@ -949,7 +930,7 @@ func (s *controllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteSn

if err = s.config.fileService.DeleteBackup(ctx, id); err != nil {
klog.Errorf("Delete snapshot for backup Id %s failed: %v", id, err.Error())
return nil, status.Error(codes.Internal, err.Error())
return nil, status.Error(*file.CodeForError(err), err.Error())
}

return &csi.DeleteSnapshotResponse{}, nil
Expand Down
18 changes: 6 additions & 12 deletions pkg/csi_driver/multishare_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,7 @@ func (m *MultishareController) CreateVolume(ctx context.Context, req *csi.Create
// lock released. poll for op.
err = m.waitOnWorkflow(ctx, workflow)
if err != nil {
errCode := file.PollOpErrorCode(err)
return nil, status.Errorf(*errCode, "Create Volume failed, operation %q poll error: %v", workflow.opName, err)
return nil, status.Errorf(*file.CodeForError(err), "Create Volume failed, operation %q poll error: %v", workflow.opName, err)
}

klog.Infof("Poll for operation %s (type %s) completed", workflow.opName, workflow.opType.String())
Expand All @@ -141,8 +140,7 @@ func (m *MultishareController) CreateVolume(ctx context.Context, req *csi.Create
// lock released. poll for share create op.
err = m.waitOnWorkflow(ctx, shareCreateWorkflow)
if err != nil {
errCode := file.PollOpErrorCode(err)
return nil, status.Errorf(*errCode, "%s operation %q poll error: %v", shareCreateWorkflow.opType.String(), shareCreateWorkflow.opName, err)
return nil, status.Errorf(*file.CodeForError(err), "%s operation %q poll error: %v", shareCreateWorkflow.opType.String(), shareCreateWorkflow.opName, err)
}
return m.getShareAndGenerateCSICreateVolumeResponse(ctx, instanceScPrefix, newShare)
}
Expand Down Expand Up @@ -201,8 +199,7 @@ func (m *MultishareController) DeleteVolume(ctx context.Context, req *csi.Delete
if workflow != nil {
err = m.waitOnWorkflow(ctx, workflow)
if err != nil {
errCode := file.PollOpErrorCode(err)
return nil, status.Errorf(*errCode, "%s operation %q poll error: %v", workflow.opType.String(), workflow.opName, err)
return nil, status.Errorf(*file.CodeForError(err), "%s operation %q poll error: %v", workflow.opType.String(), workflow.opName, err)
}
}

Expand Down Expand Up @@ -236,8 +233,7 @@ func (m *MultishareController) startAndWaitForInstanceDeleteOrShrink(ctx context
}
err = m.waitOnWorkflow(ctx, workflow)
if err != nil {
errCode := file.PollOpErrorCode(err)
return status.Errorf(*errCode, "%s operation %q poll error: %v", workflow.opType.String(), workflow.opName, err)
return status.Errorf(*file.CodeForError(err), "%s operation %q poll error: %v", workflow.opType.String(), workflow.opName, err)
}
return nil
}
Expand Down Expand Up @@ -300,8 +296,7 @@ func (m *MultishareController) ControllerExpandVolume(ctx context.Context, req *

err = m.waitOnWorkflow(ctx, workflow)
if err != nil {
errCode := file.PollOpErrorCode(err)
return nil, status.Errorf(*errCode, "wait on %s operation %q failed with error: %v", workflow.opType.String(), workflow.opName, err)
return nil, status.Errorf(*file.CodeForError(err), "wait on %s operation %q failed with error: %v", workflow.opType.String(), workflow.opName, err)
}
klog.Infof("Wait for operation %s (type %s) completed", workflow.opName, workflow.opType.String())

Expand All @@ -319,8 +314,7 @@ func (m *MultishareController) ControllerExpandVolume(ctx context.Context, req *

err = m.waitOnWorkflow(ctx, workflow)
if err != nil {
errCode := file.PollOpErrorCode(err)
return nil, status.Errorf(*errCode, "wait on share expansion op %q failed with error: %v", workflow.opName, err)
return nil, status.Errorf(*file.CodeForError(err), "wait on share expansion op %q failed with error: %v", workflow.opName, err)
}

return m.getShareAndGenerateCSIControllerExpandVolumeResponse(ctx, share, reqBytes)
Expand Down

0 comments on commit 21e4d52

Please sign in to comment.