Skip to content

Commit 01b0034

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #368 from davidz627/fix/err
Wrap all GRPC errors in status, fix semantics of NotFound errors
2 parents a383029 + c1b73b0 commit 01b0034

File tree

16 files changed

+470
-147
lines changed

16 files changed

+470
-147
lines changed

‎Gopkg.lock

+5-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Gopkg.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@
4747

4848
[[constraint]]
4949
name = "github.com/kubernetes-csi/csi-test"
50-
version = "v2.0.1"
50+
version = "2.2.0"
5151

5252
[[constraint]]
5353
branch = "master"

‎pkg/common/utils.go

+4
Original file line numberDiff line numberDiff line change
@@ -144,3 +144,7 @@ func GetDeviceName(volKey *meta.Key) (string, error) {
144144
func CreateNodeID(project, zone, name string) string {
145145
return fmt.Sprintf(nodeIDFmt, project, zone, name)
146146
}
147+
148+
func CreateZonalVolumeID(project, zone, name string) string {
149+
return fmt.Sprintf(volIDZonalFmt, project, zone, name)
150+
}

‎pkg/gce-cloud-provider/compute/fake-gce.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func (cloud *FakeCloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Contex
7575
return volumeKey, nil
7676
}
7777
}
78-
return nil, fmt.Errorf("Couldn't repair unspecified volume information")
78+
return nil, notFoundError()
7979
case meta.Regional:
8080
if volumeKey.Region != common.UnspecifiedValue {
8181
return volumeKey, nil
@@ -375,7 +375,7 @@ func (cloud *FakeCloudProvider) ResizeDisk(ctx context.Context, volKey *meta.Key
375375

376376
disk.setSizeGb(common.BytesToGb(requestBytes))
377377

378-
return requestBytes, nil
378+
return common.BytesToGb(requestBytes), nil
379379

380380
}
381381

‎pkg/gce-cloud-provider/compute/gce-compute.go

+20-5
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ func (cloud *CloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Context, v
7272
}
7373
switch volumeKey.Type() {
7474
case meta.Zonal:
75+
foundZone := ""
7576
if volumeKey.Zone == common.UnspecifiedValue {
7677
// list all zones, try to get disk in each zone
7778
zones, err := cloud.ListZones(ctx, region)
@@ -80,13 +81,27 @@ func (cloud *CloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Context, v
8081
}
8182
for _, zone := range zones {
8283
_, err := cloud.getZonalDiskOrError(ctx, zone, volumeKey.Name)
83-
if err == nil {
84-
// If there is no error we have found a disk
85-
volumeKey.Zone = zone
86-
return volumeKey, nil
84+
if err != nil {
85+
if IsGCENotFoundError(err) {
86+
// Couldn't find the disk in this zone so we keep
87+
// looking
88+
continue
89+
}
90+
// There is some miscellaneous error getting disk from zone
91+
// so we return error immediately
92+
return nil, err
8793
}
94+
if len(foundZone) > 0 {
95+
return nil, fmt.Errorf("found disk %s in more than one zone: %s and %s", volumeKey.Name, foundZone, zone)
96+
}
97+
foundZone = zone
98+
}
99+
100+
if len(foundZone) == 0 {
101+
return nil, notFoundError()
88102
}
89-
return nil, fmt.Errorf("volume zone unspecified and unable to find in any of these zones %v", zones)
103+
volumeKey.Zone = foundZone
104+
return volumeKey, nil
90105
}
91106
return volumeKey, nil
92107
case meta.Regional:

‎pkg/gce-cloud-provider/compute/gce.go

+9-2
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,14 @@ package gcecloudprovider
1717
import (
1818
"context"
1919
"fmt"
20-
"golang.org/x/oauth2/google"
21-
"gopkg.in/gcfg.v1"
2220
"net/http"
2321
"os"
2422
"runtime"
2523
"time"
2624

25+
"golang.org/x/oauth2/google"
26+
"gopkg.in/gcfg.v1"
27+
2728
"cloud.google.com/go/compute/metadata"
2829
"golang.org/x/oauth2"
2930
beta "google.golang.org/api/compute/v0.beta"
@@ -242,3 +243,9 @@ func IsGCEError(err error, reason string) bool {
242243
}
243244
return false
244245
}
246+
247+
// IsGCENotFoundError returns true if the error is a googleapi.Error with
248+
// notFound reason
249+
func IsGCENotFoundError(err error) bool {
250+
return IsGCEError(err, "notFound")
251+
}

‎pkg/gce-pd-csi-driver/controller.go

+35-13
Original file line numberDiff line numberDiff line change
@@ -224,14 +224,19 @@ func (gceCS *GCEControllerServer) DeleteVolume(ctx context.Context, req *csi.Del
224224

225225
volKey, err := common.VolumeIDToKey(volumeID)
226226
if err != nil {
227-
// Cannot find volume associated with this ID because can't even get the name or zone
228-
// This is a success according to the spec
227+
// Cannot find volume associated with this ID because VolumeID is not in
228+
// correct format, this is a success according to the Spec
229+
klog.Warningf("DeleteVolume treating volume as deleted because volume id %s is invalid: %v", volumeID, err)
229230
return &csi.DeleteVolumeResponse{}, nil
230231
}
231232

232233
volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, volKey)
233234
if err != nil {
234-
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find volume with ID %v: %v", volumeID, err))
235+
if gce.IsGCENotFoundError(err) {
236+
klog.Warningf("DeleteVolume treating volume as deleted because cannot find volume %v: %v", volumeID, err)
237+
return &csi.DeleteVolumeResponse{}, nil
238+
}
239+
return nil, status.Errorf(codes.Internal, "DeleteVolume error repairing underspecified volume key: %v", err)
235240
}
236241

237242
if acquired := gceCS.volumeLocks.TryAcquire(volumeID); !acquired {
@@ -267,12 +272,15 @@ func (gceCS *GCEControllerServer) ControllerPublishVolume(ctx context.Context, r
267272

268273
volKey, err := common.VolumeIDToKey(volumeID)
269274
if err != nil {
270-
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find volume with ID %v: %v", volumeID, err))
275+
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("ControllerPublishVolume volume ID is invalid: %v", err))
271276
}
272277

273278
volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, volKey)
274279
if err != nil {
275-
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find volume with ID %v: %v", volumeID, err))
280+
if gce.IsGCENotFoundError(err) {
281+
return nil, status.Errorf(codes.NotFound, "ControllerPublishVolume could not find volume with ID %v: %v", volumeID, err)
282+
}
283+
return nil, status.Errorf(codes.Internal, "ControllerPublishVolume error repairing underspecified volume key: %v", err)
276284
}
277285

278286
// Acquires the lock for the volume on that node only, because we need to support the ability
@@ -294,7 +302,7 @@ func (gceCS *GCEControllerServer) ControllerPublishVolume(ctx context.Context, r
294302

295303
_, err = gceCS.CloudProvider.GetDisk(ctx, volKey)
296304
if err != nil {
297-
if gce.IsGCEError(err, "notFound") {
305+
if gce.IsGCENotFoundError(err) {
298306
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find disk %v: %v", volKey.String(), err))
299307
}
300308
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown get disk error: %v", err))
@@ -305,7 +313,7 @@ func (gceCS *GCEControllerServer) ControllerPublishVolume(ctx context.Context, r
305313
}
306314
instance, err := gceCS.CloudProvider.GetInstanceOrError(ctx, instanceZone, instanceName)
307315
if err != nil {
308-
if gce.IsGCEError(err, "notFound") {
316+
if gce.IsGCENotFoundError(err) {
309317
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find instance %v: %v", nodeID, err))
310318
}
311319
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown get instance error: %v", err))
@@ -365,7 +373,7 @@ func (gceCS *GCEControllerServer) ControllerUnpublishVolume(ctx context.Context,
365373

366374
volKey, err := common.VolumeIDToKey(volumeID)
367375
if err != nil {
368-
return nil, err
376+
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("ControllerUnpublishVolume Volume ID is invalid: %v", err))
369377
}
370378

371379
// Acquires the lock for the volume on that node only, because we need to support the ability
@@ -382,7 +390,12 @@ func (gceCS *GCEControllerServer) ControllerUnpublishVolume(ctx context.Context,
382390
}
383391
instance, err := gceCS.CloudProvider.GetInstanceOrError(ctx, instanceZone, instanceName)
384392
if err != nil {
385-
return nil, err
393+
if gce.IsGCENotFoundError(err) {
394+
// Node not existing on GCE means that disk has been detached
395+
klog.Warningf("Treating volume %v as unpublished because node %v could not be found", volKey.String(), instanceName)
396+
return &csi.ControllerUnpublishVolumeResponse{}, nil
397+
}
398+
return nil, status.Error(codes.Internal, fmt.Sprintf("error getting instance: %v", err))
386399
}
387400

388401
deviceName, err := common.GetDeviceName(volKey)
@@ -420,7 +433,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
420433
}
421434
volKey, err := common.VolumeIDToKey(volumeID)
422435
if err != nil {
423-
return nil, status.Error(codes.NotFound, fmt.Sprintf("Volume ID is of improper format, got %v", volumeID))
436+
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("ValidateVolumeCapabilities Volume ID is invalid: %v", err))
424437
}
425438

426439
if acquired := gceCS.volumeLocks.TryAcquire(volumeID); !acquired {
@@ -430,7 +443,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
430443

431444
_, err = gceCS.CloudProvider.GetDisk(ctx, volKey)
432445
if err != nil {
433-
if gce.IsGCEError(err, "notFound") {
446+
if gce.IsGCENotFoundError(err) {
434447
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find disk %v: %v", volKey.Name, err))
435448
}
436449
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown get disk error: %v", err))
@@ -532,14 +545,23 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C
532545
}
533546
volKey, err := common.VolumeIDToKey(volumeID)
534547
if err != nil {
535-
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find volume with ID %v: %v", volumeID, err))
548+
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("CreateSnapshot Volume ID is invalid: %v", err))
536549
}
537550

538551
if acquired := gceCS.volumeLocks.TryAcquire(volumeID); !acquired {
539552
return nil, status.Errorf(codes.Aborted, common.VolumeOperationAlreadyExistsFmt, volumeID)
540553
}
541554
defer gceCS.volumeLocks.Release(volumeID)
542555

556+
// Check if volume exists
557+
_, err = gceCS.CloudProvider.GetDisk(ctx, volKey)
558+
if err != nil {
559+
if gce.IsGCENotFoundError(err) {
560+
return nil, status.Error(codes.NotFound, fmt.Sprintf("CreateSnapshot could not find disk %v: %v", volKey.String(), err))
561+
}
562+
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateSnapshot unknown get disk error: %v", err))
563+
}
564+
543565
// Check if snapshot already exists
544566
var snapshot *compute.Snapshot
545567
snapshot, err = gceCS.CloudProvider.GetSnapshot(ctx, req.Name)
@@ -670,7 +692,7 @@ func (gceCS *GCEControllerServer) ControllerExpandVolume(ctx context.Context, re
670692

671693
volKey, err := common.VolumeIDToKey(volumeID)
672694
if err != nil {
673-
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("ControllerExpandVolume volume ID is invalid: %v", err))
695+
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("ControllerExpandVolume Volume ID is invalid: %v", err))
674696
}
675697

676698
resizedGb, err := gceCS.CloudProvider.ResizeDisk(ctx, volKey, reqBytes)

0 commit comments

Comments
 (0)