Skip to content

Commit 95a8291

Browse files
pohlypohly
committed
clean up error handling
Wrapping errors with an "Internal" status error was at best useless (it does not tell the caller anything and the default "Unknown" error has the same effect) and at worst hid the actual status (when wrapping createVolume). Therefore most of those wrappers get removed. The handling of "not found" also was inconsistent. Some places checked the map directly, others made assumptions about the error returned by the helper functions. Now the helper functions themselves generate a gRPC status error which can be returned directly.
1 parent 9f929d0 commit 95a8291

File tree

3 files changed

+33
-34
lines changed

3 files changed

+33
-34
lines changed

‎pkg/hostpath/controllerserver.go‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ func (hp *hostPath) CreateVolume(ctx context.Context, req *csi.CreateVolumeReque
143143
kind := req.GetParameters()[storageKind]
144144
vol, err := hp.createVolume(volumeID, req.GetName(), capacity, requestedAccessType, false /* ephemeral */, kind)
145145
if err != nil {
146-
return nil, status.Errorf(codes.Internal, "failed to create volume %v: %v", volumeID, err)
146+
return nil, fmt.Errorf("failed to create volume %v: %w", volumeID, err)
147147
}
148148
glog.V(4).Infof("created volume %s at path %s", vol.VolID, vol.VolPath)
149149

@@ -203,7 +203,7 @@ func (hp *hostPath) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeReque
203203

204204
volId := req.GetVolumeId()
205205
if err := hp.deleteVolume(volId); err != nil {
206-
return nil, status.Errorf(codes.Internal, "failed to delete volume %v: %v", volId, err)
206+
return nil, fmt.Errorf("failed to delete volume %v: %w", volId, err)
207207
}
208208
glog.V(4).Infof("volume %v successfully deleted", volId)
209209

@@ -232,7 +232,7 @@ func (hp *hostPath) ValidateVolumeCapabilities(ctx context.Context, req *csi.Val
232232
defer hp.mutex.Unlock()
233233

234234
if _, err := hp.getVolumeByID(req.GetVolumeId()); err != nil {
235-
return nil, status.Error(codes.NotFound, req.GetVolumeId())
235+
return nil, err
236236
}
237237

238238
for _, cap := range req.GetVolumeCapabilities() {
@@ -343,9 +343,9 @@ func (hp *hostPath) ControllerGetVolume(ctx context.Context, req *csi.Controller
343343
hp.mutex.Lock()
344344
defer hp.mutex.Unlock()
345345

346-
volume, ok := hp.volumes[req.GetVolumeId()]
347-
if !ok {
348-
return nil, status.Error(codes.NotFound, "The volume not found")
346+
volume, err := hp.getVolumeByID(req.GetVolumeId())
347+
if err != nil {
348+
return nil, err
349349
}
350350

351351
healthy, msg := hp.doHealthCheckInControllerSide(req.GetVolumeId())
@@ -412,9 +412,9 @@ func (hp *hostPath) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotR
412412
}
413413

414414
volumeID := req.GetSourceVolumeId()
415-
hostPathVolume, ok := hp.volumes[volumeID]
416-
if !ok {
417-
return nil, status.Error(codes.Internal, "volumeID is not exist")
415+
hostPathVolume, err := hp.getVolumeByID(volumeID)
416+
if err != nil {
417+
return nil, err
418418
}
419419

420420
snapshotID := uuid.NewUUID().String()
@@ -433,7 +433,7 @@ func (hp *hostPath) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotR
433433
executor := utilexec.New()
434434
out, err := executor.Command(cmd[0], cmd[1:]...).CombinedOutput()
435435
if err != nil {
436-
return nil, status.Errorf(codes.Internal, "failed create snapshot: %v: %s", err, out)
436+
return nil, fmt.Errorf("failed create snapshot: %w: %s", err, out)
437437
}
438438

439439
glog.V(4).Infof("create volume snapshot %s", file)
@@ -614,14 +614,13 @@ func (hp *hostPath) ControllerExpandVolume(ctx context.Context, req *csi.Control
614614

615615
exVol, err := hp.getVolumeByID(volID)
616616
if err != nil {
617-
// Assume not found error
618-
return nil, status.Errorf(codes.NotFound, "Could not get volume %s: %v", volID, err)
617+
return nil, err
619618
}
620619

621620
if exVol.VolSize < capacity {
622621
exVol.VolSize = capacity
623622
if err := hp.updateVolume(volID, exVol); err != nil {
624-
return nil, status.Errorf(codes.Internal, "Could not update volume %s: %v", volID, err)
623+
return nil, fmt.Errorf("could not update volume %s: %w", volID, err)
625624
}
626625
}
627626

‎pkg/hostpath/hostpath.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ func (hp *hostPath) getVolumeByID(volumeID string) (hostPathVolume, error) {
239239
if hostPathVol, ok := hp.volumes[volumeID]; ok {
240240
return hostPathVol, nil
241241
}
242-
return hostPathVolume{}, fmt.Errorf("volume id %s does not exist in the volumes list", volumeID)
242+
return hostPathVolume{}, status.Errorf(codes.NotFound, "volume id %s does not exist in the volumes list", volumeID)
243243
}
244244

245245
func (hp *hostPath) getVolumeByName(volName string) (hostPathVolume, error) {
@@ -248,7 +248,7 @@ func (hp *hostPath) getVolumeByName(volName string) (hostPathVolume, error) {
248248
return hostPathVol, nil
249249
}
250250
}
251-
return hostPathVolume{}, fmt.Errorf("volume name %s does not exist in the volumes list", volName)
251+
return hostPathVolume{}, status.Errorf(codes.NotFound, "volume name %s does not exist in the volumes list", volName)
252252
}
253253

254254
func (hp *hostPath) getSnapshotByName(name string) (hostPathSnapshot, error) {
@@ -257,7 +257,7 @@ func (hp *hostPath) getSnapshotByName(name string) (hostPathSnapshot, error) {
257257
return snapshot, nil
258258
}
259259
}
260-
return hostPathSnapshot{}, fmt.Errorf("snapshot name %s does not exist in the snapshots list", name)
260+
return hostPathSnapshot{}, status.Errorf(codes.NotFound, "snapshot name %s does not exist in the snapshots list", name)
261261
}
262262

263263
// getVolumePath returns the canonical path for hostpath volume
@@ -406,7 +406,7 @@ func (hp *hostPath) loadFromSnapshot(size int64, snapshotId, destPath string, mo
406406
return status.Errorf(codes.NotFound, "cannot find snapshot %v", snapshotId)
407407
}
408408
if snapshot.ReadyToUse != true {
409-
return status.Errorf(codes.Internal, "snapshot %v is not yet ready to use.", snapshotId)
409+
return fmt.Errorf("snapshot %v is not yet ready to use", snapshotId)
410410
}
411411
if snapshot.SizeBytes > size {
412412
return status.Errorf(codes.InvalidArgument, "snapshot %v size %v is greater than requested volume size %v", snapshotId, snapshot.SizeBytes, size)
@@ -428,7 +428,7 @@ func (hp *hostPath) loadFromSnapshot(size int64, snapshotId, destPath string, mo
428428
out, err := executor.Command(cmd[0], cmd[1:]...).CombinedOutput()
429429
glog.V(4).Infof("Command Finish: %v", string(out))
430430
if err != nil {
431-
return status.Errorf(codes.Internal, "failed pre-populate data from snapshot %v: %v: %s", snapshotId, err, out)
431+
return fmt.Errorf("failed pre-populate data from snapshot %v: %w: %s", snapshotId, err, out)
432432
}
433433
return nil
434434
}
@@ -460,7 +460,7 @@ func loadFromFilesystemVolume(hostPathVolume hostPathVolume, destPath string) er
460460
srcPath := hostPathVolume.VolPath
461461
isEmpty, err := hostPathIsEmpty(srcPath)
462462
if err != nil {
463-
return status.Errorf(codes.Internal, "failed verification check of source hostpath volume %v: %v", hostPathVolume.VolID, err)
463+
return fmt.Errorf("failed verification check of source hostpath volume %v: %w", hostPathVolume.VolID, err)
464464
}
465465

466466
// If the source hostpath volume is empty it's a noop and we just move along, otherwise the cp call will fail with a a file stat error DNE
@@ -469,7 +469,7 @@ func loadFromFilesystemVolume(hostPathVolume hostPathVolume, destPath string) er
469469
executor := utilexec.New()
470470
out, err := executor.Command("cp", args...).CombinedOutput()
471471
if err != nil {
472-
return status.Errorf(codes.Internal, "failed pre-populate data from volume %v: %v: %s", hostPathVolume.VolID, err, out)
472+
return fmt.Errorf("failed pre-populate data from volume %v: %s: %w", hostPathVolume.VolID, out, err)
473473
}
474474
}
475475
return nil
@@ -481,7 +481,7 @@ func loadFromBlockVolume(hostPathVolume hostPathVolume, destPath string) error {
481481
executor := utilexec.New()
482482
out, err := executor.Command("dd", args...).CombinedOutput()
483483
if err != nil {
484-
return status.Errorf(codes.Internal, "failed pre-populate data from volume %v: %v: %s", hostPathVolume.VolID, err, out)
484+
return fmt.Errorf("failed pre-populate data from volume %v: %w: %s", hostPathVolume.VolID, err, out)
485485
}
486486
return nil
487487
}

‎pkg/hostpath/nodeserver.go‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ func (hp *hostPath) NodePublishVolume(ctx context.Context, req *csi.NodePublishV
8787
// Get loop device from the volume path.
8888
loopDevice, err := volPathHandler.GetLoopDevice(vol.VolPath)
8989
if err != nil {
90-
return nil, status.Error(codes.Internal, fmt.Sprintf("failed to get the loop device: %v", err))
90+
return nil, fmt.Errorf("failed to get the loop device: %w", err)
9191
}
9292

9393
mounter := mount.New("")
@@ -96,18 +96,18 @@ func (hp *hostPath) NodePublishVolume(ctx context.Context, req *csi.NodePublishV
9696
_, err = os.Lstat(targetPath)
9797
if os.IsNotExist(err) {
9898
if err = makeFile(targetPath); err != nil {
99-
return nil, status.Error(codes.Internal, fmt.Sprintf("failed to create target path: %s: %v", targetPath, err))
99+
return nil, fmt.Errorf("failed to create target path: %w", err)
100100
}
101101
}
102102
if err != nil {
103-
return nil, status.Errorf(codes.Internal, "failed to check if the target block file exists: %v", err)
103+
return nil, fmt.Errorf("failed to check if the target block file exists: %w", err)
104104
}
105105

106106
// Check if the target path is already mounted. Prevent remounting.
107107
notMount, err := mount.IsNotMountPoint(mounter, targetPath)
108108
if err != nil {
109109
if !os.IsNotExist(err) {
110-
return nil, status.Errorf(codes.Internal, "error checking path %s for mount: %s", targetPath, err)
110+
return nil, fmt.Errorf("error checking path %s for mount: %w", targetPath, err)
111111
}
112112
notMount = true
113113
}
@@ -119,7 +119,7 @@ func (hp *hostPath) NodePublishVolume(ctx context.Context, req *csi.NodePublishV
119119

120120
options := []string{"bind"}
121121
if err := mount.New("").Mount(loopDevice, targetPath, "", options); err != nil {
122-
return nil, status.Error(codes.Internal, fmt.Sprintf("failed to mount block device: %s at %s: %v", loopDevice, targetPath, err))
122+
return nil, fmt.Errorf("failed to mount block device: %s at %s: %w", loopDevice, targetPath, err)
123123
}
124124
} else if req.GetVolumeCapability().GetMount() != nil {
125125
if vol.VolAccessType != mountAccess {
@@ -130,11 +130,11 @@ func (hp *hostPath) NodePublishVolume(ctx context.Context, req *csi.NodePublishV
130130
if err != nil {
131131
if os.IsNotExist(err) {
132132
if err = os.MkdirAll(targetPath, 0750); err != nil {
133-
return nil, status.Error(codes.Internal, err.Error())
133+
return nil, fmt.Errorf("create target path: %w", err)
134134
}
135135
notMnt = true
136136
} else {
137-
return nil, status.Error(codes.Internal, err.Error())
137+
return nil, fmt.Errorf("check target path: %w", err)
138138
}
139139
}
140140

@@ -172,7 +172,7 @@ func (hp *hostPath) NodePublishVolume(ctx context.Context, req *csi.NodePublishV
172172
errList.WriteString(fmt.Sprintf(" :%s", rmErr.Error()))
173173
}
174174
}
175-
return nil, status.Error(codes.Internal, fmt.Sprintf("failed to mount device: %s at %s: %s", path, targetPath, errList.String()))
175+
return nil, fmt.Errorf("failed to mount device: %s at %s: %s", path, targetPath, errList.String())
176176
}
177177
}
178178

@@ -206,26 +206,26 @@ func (hp *hostPath) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpubl
206206
// Unmount only if the target path is really a mount point.
207207
if notMnt, err := mount.IsNotMountPoint(mount.New(""), targetPath); err != nil {
208208
if !os.IsNotExist(err) {
209-
return nil, status.Error(codes.Internal, err.Error())
209+
return nil, fmt.Errorf("check target path: %w", err)
210210
}
211211
} else if !notMnt {
212212
// Unmounting the image or filesystem.
213213
err = mount.New("").Unmount(targetPath)
214214
if err != nil {
215-
return nil, status.Error(codes.Internal, err.Error())
215+
return nil, fmt.Errorf("unmount target path: %w", err)
216216
}
217217
}
218218
// Delete the mount point.
219219
// Does not return error for non-existent path, repeated calls OK for idempotency.
220220
if err = os.RemoveAll(targetPath); err != nil {
221-
return nil, status.Error(codes.Internal, err.Error())
221+
return nil, fmt.Errorf("remove target path: %w", err)
222222
}
223223
glog.V(4).Infof("hostpath: volume %s has been unpublished.", targetPath)
224224

225225
if vol.Ephemeral {
226226
glog.V(4).Infof("deleting volume %s", volumeID)
227227
if err := hp.deleteVolume(volumeID); err != nil && !os.IsNotExist(err) {
228-
return nil, status.Error(codes.Internal, fmt.Sprintf("failed to delete volume: %s", err))
228+
return nil, fmt.Errorf("failed to delete volume: %w", err)
229229
}
230230
}
231231

@@ -318,7 +318,7 @@ func (hp *hostPath) NodeGetVolumeStats(ctx context.Context, in *csi.NodeGetVolum
318318
glog.V(3).Infof("Healthy state: %+v Volume: %+v", volume.VolName, healthy)
319319
available, capacity, used, err := getPVCapacity(in.GetVolumeId())
320320
if err != nil {
321-
return nil, status.Errorf(codes.Internal, "Get volume capacity failed: %+v", err)
321+
return nil, fmt.Errorf("get volume capacity failed: %w", err)
322322
}
323323

324324
glog.V(3).Infof("Capacity: %+v Used: %+v Available: %+v", capacity, used, available)

0 commit comments

Comments
 (0)