Skip to content

Commit

Permalink
Merge pull request #562 from k8s-infra-cherrypick-robot/cherry-pick-5…
Browse files Browse the repository at this point in the history 
…23-to-release-1.5

[release-1.5] Use CodeForError when we call filestore API
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jul 18, 2023
2 parents 8cc3158 + 8416199 commit 7c59a5b
Show file tree
Hide file tree
Showing 6 changed files with 125 additions and 76 deletions.
20 changes: 16 additions & 4 deletions pkg/cloud_provider/file/file.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -625,8 +625,8 @@ func GetInstanceNameFromURI(uri string) (project, location, name string, err err
}

func IsNotFoundErr(err error) bool {
apiErr, ok := err.(*googleapi.Error)
if !ok {
var apiErr *googleapi.Error
if !errors.As(err, &apiErr) {
return false
}

Expand Down Expand Up @@ -718,7 +718,7 @@ func existingErrorCode(err error) *codes.Code {
return nil
}

// CodeForError returns a pointer to the grpc error code that maps to the http
// 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:
Expand All @@ -729,7 +729,10 @@ func existingErrorCode(err error) *codes.Code {
// 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 {
func codeForError(err error) *codes.Code {
if err == nil {
return nil
}
if errCode := existingErrorCode(err); errCode != nil {
return errCode
}
Expand All @@ -743,6 +746,15 @@ func CodeForError(err error) *codes.Code {
return util.ErrCodePtr(codes.Internal)
}

// Status error returns the error as a grpc status error, and
// sets the grpc error code according to CodeForError.
func StatusError(err error) error {
if err == nil {
return nil
}
return status.Error(*codeForError(err), err.Error())
}

// This function returns the backup URI, the region that was picked to be the backup resource location and error.
func CreateBackupURI(obj *ServiceInstance, backupName string, backupLocation string) (string, string, error) {
region, err := deduceRegion(obj, backupLocation)
Expand Down
33 changes: 32 additions & 1 deletion pkg/cloud_provider/file/file_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -581,10 +581,15 @@ func TestCodeForError(t *testing.T) {
err: status.Error(codes.Aborted, "aborted error"),
expectedErrCode: util.ErrCodePtr(codes.Aborted),
},
{
name: "nil error",
err: nil,
expectedErrCode: nil,
},
}

for _, test := range cases {
errCode := CodeForError(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 All @@ -594,6 +599,32 @@ func TestCodeForError(t *testing.T) {
}
}

func TestStatusError(t *testing.T) {
cases := []struct {
name string
err error
expectedErr error
}{
{
name: "404 googleapi error",
err: &googleapi.Error{Code: http.StatusNotFound},
expectedErr: status.Error(codes.NotFound, ""),
},
{
name: "nil error",
err: nil,
expectedErr: nil,
},
}

for _, test := range cases {
err := StatusError(test.err)
if (test.expectedErr == nil) != (err == nil) {
t.Errorf("test %v failed: got %v, expected %v", test.name, err, test.expectedErr)
}
}
}

func TestIsUserError(t *testing.T) {
cases := []struct {
name string
Expand Down
44 changes: 23 additions & 21 deletions pkg/csi_driver/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,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())
return nil, status.Error(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}
sourceSnapshotId = id
}
Expand All @@ -223,7 +223,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) {
return nil, status.Error(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}

if filer != nil {
Expand Down Expand Up @@ -262,7 +262,7 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu
// In case of abort, the CIDR IP is released and available for reservation
defer s.config.ipAllocator.ReleaseIPRange(reservedIPRange)
if err != nil {
return nil, err
return nil, file.StatusError(err)
}

// Adding the reserved IP range to the instance object
Expand All @@ -272,7 +272,7 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu
// Add labels.
labels, err := extractLabels(param, s.config.driver.config.Name)
if err != nil {
return nil, err
return nil, file.StatusError(err)
}
newFiler.Labels = labels

Expand All @@ -285,7 +285,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())
return nil, status.Error(*file.CodeForError(createErr), createErr.Error())
return nil, file.StatusError(createErr)
}
}
resp := &csi.CreateVolumeResponse{Volume: s.fileInstanceToCSIVolume(filer, modeInstance, sourceSnapshotId)}
Expand Down Expand Up @@ -364,7 +364,10 @@ func (s *controllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolu
duration := time.Since(start)
s.config.metricsManager.RecordOperationMetrics(err, methodDeleteVolume, modeMultishare, duration)
klog.Infof("Deletevolume response %+v error %v, for request: %+v", response, err, req)
return response, err
if err != nil {
return response, file.StatusError(err)
}
return response, nil
}

filer, _, err := getFileInstanceFromID(volumeID)
Expand All @@ -385,7 +388,7 @@ func (s *controllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolu
if file.IsNotFoundErr(err) {
return &csi.DeleteVolumeResponse{}, nil
}
return nil, status.Error(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}

if filer.State == "DELETING" {
Expand All @@ -395,7 +398,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(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}

klog.Infof("DeleteVolume succeeded for volume %v", volumeID)
Expand All @@ -422,7 +425,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) {
return nil, status.Error(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}
if newFiler == nil {
return nil, status.Errorf(codes.NotFound, "volume %v doesn't exist", volumeID)
Expand Down Expand Up @@ -682,7 +685,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 {
return nil, status.Error(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}
if filer.State != "READY" {
return nil, fmt.Errorf("lolume %q is not yet ready, current state %q", volumeID, filer.State)
Expand All @@ -698,7 +701,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(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}

if hasPendingOps {
Expand All @@ -708,7 +711,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(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}

klog.Infof("Controller expand volume succeeded for volume %v, new size(bytes): %v", volumeID, newfiler.Volume.SizeBytes)
Expand Down Expand Up @@ -877,12 +880,12 @@ func (s *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSn
backupInfo, err := s.config.fileService.GetBackup(ctx, backupUri)
if err != nil {
if !file.IsNotFoundErr(err) {
return nil, status.Error(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}
} else {
backupSourceCSIHandle, err := util.BackupVolumeSourceToCSIVolumeHandle(backupInfo.SourceInstance, backupInfo.SourceShare)
if err != nil {
return nil, status.Errorf(codes.Internal, "Cannot determine volume handle from back source instance %s, share %s", backupInfo.SourceInstance, backupInfo.SourceShare)
return nil, status.Errorf(codes.InvalidArgument, "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", backupSourceCSIHandle, volumeID)
Expand All @@ -896,7 +899,8 @@ func (s *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSn
}
tp, err := util.ParseTimestamp(backupInfo.Backup.CreateTime)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse create timestamp for backup %v", backupInfo.Backup.Name)
err = fmt.Errorf("failed to parse create timestamp for backup %v: %w", backupInfo.Backup.Name, err)
return nil, file.StatusError(err)
}
klog.V(4).Infof("CreateSnapshot success for volume %v, Backup Id: %v", volumeID, backupInfo.Backup.Name)
return &csi.CreateSnapshotResponse{
Expand All @@ -913,13 +917,11 @@ func (s *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSn
backupObj, err := s.config.fileService.CreateBackup(ctx, filer, req.Name, util.GetBackupLocation(req.GetParameters()))
if err != nil {
klog.Errorf("Create snapshot for volume Id %s failed: %v", volumeID, err.Error())
if err != nil {
return nil, status.Error(*file.CodeForError(err), err.Error())
}
return nil, file.StatusError(err)
}
tp, err := util.ParseTimestamp(backupObj.CreateTime)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
return nil, file.StatusError(err)
}
resp := &csi.CreateSnapshotResponse{
Snapshot: &csi.Snapshot{
Expand Down Expand Up @@ -958,7 +960,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(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}

if backupInfo.Backup.State == "DELETING" {
Expand All @@ -967,7 +969,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(*file.CodeForError(err), err.Error())
return nil, file.StatusError(err)
}

return &csi.DeleteSnapshotResponse{}, nil
Expand Down
Loading

0 comments on commit 7c59a5b

Please sign in to comment.