Skip to content

Commit 69f4ca3

Browse files
Sneha-atsunnylovestiramisu
Sneha-at
authored and
sunnylovestiramisu
committed
Refactoring GetDisk calls based on comments
1 parent ad1da56 commit 69f4ca3

File tree

1 file changed

+31
-33
lines changed

1 file changed

+31
-33
lines changed

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

Lines changed: 31 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
232232
// Apply Parameters (case-insensitive). We leave validation of
233233
// the values to the cloud provider.
234234
params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraVolumeLabels)
235+
diskTypeForMetric = params.DiskType
235236
if err != nil {
236237
return nil, status.Errorf(codes.InvalidArgument, "failed to extract parameters: %v", err.Error())
237238
}
@@ -344,7 +345,6 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
344345

345346
// Verify that the volume in VolumeContentSource exists.
346347
diskFromSourceVolume, err := gceCS.CloudProvider.GetDisk(ctx, project, sourceVolKey, gceAPIVersion)
347-
diskTypeForMetric = metrics.GetDiskType(diskFromSourceVolume)
348348
if err != nil {
349349
if gce.IsGCEError(err, "notFound") {
350350
return nil, status.Errorf(codes.NotFound, "CreateVolume source volume %s does not exist", volumeContentSourceVolumeID)
@@ -401,7 +401,6 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
401401

402402
// Create the disk
403403
var disk *gce.CloudDisk
404-
diskTypeForMetric = params.DiskType
405404
switch params.ReplicationType {
406405
case replicationTypeNone:
407406
if len(zones) != 1 {
@@ -459,8 +458,6 @@ func (gceCS *GCEControllerServer) DeleteVolume(ctx context.Context, req *csi.Del
459458
}
460459

461460
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
462-
disk, _ := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
463-
diskTypeForMetric = metrics.GetDiskType(disk)
464461
if err != nil {
465462
if gce.IsGCENotFoundError(err) {
466463
klog.Warningf("DeleteVolume treating volume as deleted because cannot find volume %v: %v", volumeID, err.Error())
@@ -473,7 +470,8 @@ func (gceCS *GCEControllerServer) DeleteVolume(ctx context.Context, req *csi.Del
473470
return nil, status.Errorf(codes.Aborted, common.VolumeOperationAlreadyExistsFmt, volumeID)
474471
}
475472
defer gceCS.volumeLocks.Release(volumeID)
476-
473+
disk, _ := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
474+
diskTypeForMetric = metrics.GetDiskType(disk)
477475
err = gceCS.CloudProvider.DeleteDisk(ctx, project, volKey)
478476
if err != nil {
479477
return nil, common.LoggedError("Failed to delete disk: ", err)
@@ -492,18 +490,17 @@ func (gceCS *GCEControllerServer) ControllerPublishVolume(ctx context.Context, r
492490
}
493491
}()
494492
// Only valid requests will be accepted
495-
project, volKey, err := gceCS.validateControllerPublishVolumeRequest(ctx, req)
493+
_, _, err = gceCS.validateControllerPublishVolumeRequest(ctx, req)
496494
if err != nil {
497495
return nil, err
498496
}
499-
diskToPublish, _ := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
500-
diskTypeForMetric = metrics.GetDiskType(diskToPublish)
497+
501498
backoffId := gceCS.errorBackoff.backoffId(req.NodeId, req.VolumeId)
502499
if gceCS.errorBackoff.blocking(backoffId) {
503500
return nil, status.Errorf(codes.Unavailable, "ControllerPublish not permitted on node %q due to backoff condition", req.NodeId)
504501
}
505502

506-
resp, err := gceCS.executeControllerPublishVolume(ctx, req)
503+
resp, err, diskTypeForMetric := gceCS.executeControllerPublishVolume(ctx, req)
507504
if err != nil {
508505
klog.Infof("For node %s adding backoff due to error for volume %s: %v", req.NodeId, req.VolumeId, err.Error())
509506
gceCS.errorBackoff.next(backoffId)
@@ -551,10 +548,11 @@ func parseMachineType(machineTypeUrl string) string {
551548
return machineType
552549
}
553550

554-
func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (*csi.ControllerPublishVolumeResponse, error) {
551+
func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (*csi.ControllerPublishVolumeResponse, error, string) {
552+
diskToPublish := ""
555553
project, volKey, err := gceCS.validateControllerPublishVolumeRequest(ctx, req)
556554
if err != nil {
557-
return nil, err
555+
return nil, err, diskToPublish
558556
}
559557

560558
volumeID := req.GetVolumeId()
@@ -569,35 +567,36 @@ func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Con
569567
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
570568
if err != nil {
571569
if gce.IsGCENotFoundError(err) {
572-
return nil, status.Errorf(codes.NotFound, "ControllerPublishVolume could not find volume with ID %v: %v", volumeID, err.Error())
570+
return nil, status.Errorf(codes.NotFound, "ControllerPublishVolume could not find volume with ID %v: %v", volumeID, err.Error()), diskToPublish
573571
}
574-
return nil, common.LoggedError("ControllerPublishVolume error repairing underspecified volume key: ", err)
572+
return nil, common.LoggedError("ControllerPublishVolume error repairing underspecified volume key: ", err), diskToPublish
575573
}
576574

577575
// Acquires the lock for the volume on that node only, because we need to support the ability
578576
// to publish the same volume onto different nodes concurrently
579577
lockingVolumeID := fmt.Sprintf("%s/%s", nodeID, volumeID)
580578
if acquired := gceCS.volumeLocks.TryAcquire(lockingVolumeID); !acquired {
581-
return nil, status.Errorf(codes.Aborted, common.VolumeOperationAlreadyExistsFmt, lockingVolumeID)
579+
return nil, status.Errorf(codes.Aborted, common.VolumeOperationAlreadyExistsFmt, lockingVolumeID), diskToPublish
582580
}
583581
defer gceCS.volumeLocks.Release(lockingVolumeID)
584-
_, err = gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
582+
disk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
583+
diskToPublish = metrics.GetDiskType(disk)
585584
if err != nil {
586585
if gce.IsGCENotFoundError(err) {
587-
return nil, status.Errorf(codes.NotFound, "Could not find disk %v: %v", volKey.String(), err.Error())
586+
return nil, status.Errorf(codes.NotFound, "Could not find disk %v: %v", volKey.String(), err.Error()), diskToPublish
588587
}
589-
return nil, status.Errorf(codes.Internal, "Failed to getDisk: %v", err.Error())
588+
return nil, status.Errorf(codes.Internal, "Failed to getDisk: %v", err.Error()), diskToPublish
590589
}
591590
instanceZone, instanceName, err := common.NodeIDToZoneAndName(nodeID)
592591
if err != nil {
593-
return nil, status.Errorf(codes.NotFound, "could not split nodeID: %v", err.Error())
592+
return nil, status.Errorf(codes.NotFound, "could not split nodeID: %v", err.Error()), diskToPublish
594593
}
595594
instance, err := gceCS.CloudProvider.GetInstanceOrError(ctx, instanceZone, instanceName)
596595
if err != nil {
597596
if gce.IsGCENotFoundError(err) {
598-
return nil, status.Errorf(codes.NotFound, "Could not find instance %v: %v", nodeID, err.Error())
597+
return nil, status.Errorf(codes.NotFound, "Could not find instance %v: %v", nodeID, err.Error()), diskToPublish
599598
}
600-
return nil, status.Errorf(codes.Internal, "Failed to get instance: %v", err.Error())
599+
return nil, status.Errorf(codes.Internal, "Failed to get instance: %v", err.Error()), diskToPublish
601600
}
602601

603602
readWrite := "READ_WRITE"
@@ -607,21 +606,21 @@ func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Con
607606

608607
deviceName, err := common.GetDeviceName(volKey)
609608
if err != nil {
610-
return nil, status.Errorf(codes.Internal, "error getting device name: %v", err.Error())
609+
return nil, status.Errorf(codes.Internal, "error getting device name: %v", err.Error()), diskToPublish
611610
}
612611

613612
attached, err := diskIsAttachedAndCompatible(deviceName, instance, volumeCapability, readWrite)
614613
if err != nil {
615-
return nil, status.Errorf(codes.AlreadyExists, "Disk %v already published to node %v but incompatible: %v", volKey.Name, nodeID, err.Error())
614+
return nil, status.Errorf(codes.AlreadyExists, "Disk %v already published to node %v but incompatible: %v", volKey.Name, nodeID, err.Error()), diskToPublish
616615
}
617616
if attached {
618617
// Volume is attached to node. Success!
619618
klog.V(4).Infof("ControllerPublishVolume succeeded for disk %v to instance %v, already attached.", volKey, nodeID)
620-
return pubVolResp, nil
619+
return pubVolResp, nil, diskToPublish
621620
}
622621
instanceZone, instanceName, err = common.NodeIDToZoneAndName(nodeID)
623622
if err != nil {
624-
return nil, status.Errorf(codes.InvalidArgument, "could not split nodeID: %v", err.Error())
623+
return nil, status.Errorf(codes.InvalidArgument, "could not split nodeID: %v", err.Error()), diskToPublish
625624
}
626625
err = gceCS.CloudProvider.AttachDisk(ctx, project, volKey, readWrite, attachableDiskTypePersistent, instanceZone, instanceName)
627626
if err != nil {
@@ -630,18 +629,18 @@ func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Con
630629
// If we encountered an UnsupportedDiskError, rewrite the error message to be more user friendly.
631630
// The error message from GCE is phrased around disk create on VM creation, not runtime attach.
632631
machineType := parseMachineType(instance.MachineType)
633-
return nil, status.Errorf(codes.InvalidArgument, "'%s' is not a compatible disk type with the machine type %s, please review the GCP online documentation for available persistent disk options", udErr.DiskType, machineType)
632+
return nil, status.Errorf(codes.InvalidArgument, "'%s' is not a compatible disk type with the machine type %s, please review the GCP online documentation for available persistent disk options", udErr.DiskType, machineType), diskToPublish
634633
}
635-
return nil, status.Errorf(codes.Internal, "Failed to Attach: %v", err.Error())
634+
return nil, status.Errorf(codes.Internal, "Failed to Attach: %v", err.Error()), diskToPublish
636635
}
637636

638637
err = gceCS.CloudProvider.WaitForAttach(ctx, project, volKey, instanceZone, instanceName)
639638
if err != nil {
640-
return nil, status.Errorf(codes.Internal, "Errored during WaitForAttach: %v", err.Error())
639+
return nil, status.Errorf(codes.Internal, "Errored during WaitForAttach: %v", err.Error()), diskToPublish
641640
}
642641

643642
klog.V(4).Infof("ControllerPublishVolume succeeded for disk %v to instance %v", volKey, nodeID)
644-
return pubVolResp, nil
643+
return pubVolResp, nil, diskToPublish
645644
}
646645

647646
func (gceCS *GCEControllerServer) ControllerUnpublishVolume(ctx context.Context, req *csi.ControllerUnpublishVolumeRequest) (*csi.ControllerUnpublishVolumeResponse, error) {
@@ -656,14 +655,13 @@ func (gceCS *GCEControllerServer) ControllerUnpublishVolume(ctx context.Context,
656655
if err != nil {
657656
return nil, err
658657
}
659-
diskToUnpublish, _ := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
660-
diskTypeForMetric = metrics.GetDiskType(diskToUnpublish)
661658
// Only valid requests will be queued
662659
backoffId := gceCS.errorBackoff.backoffId(req.NodeId, req.VolumeId)
663660
if gceCS.errorBackoff.blocking(backoffId) {
664661
return nil, status.Errorf(codes.Unavailable, "ControllerUnpublish not permitted on node %q due to backoff condition", req.NodeId)
665662
}
666-
663+
diskToUnpublish, _ := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
664+
diskTypeForMetric = metrics.GetDiskType(diskToUnpublish)
667665
resp, err := gceCS.executeControllerUnpublishVolume(ctx, req)
668666
if err != nil {
669667
klog.Infof("For node %s adding backoff due to error for volume %s", req.NodeId, req.VolumeId)
@@ -1263,15 +1261,15 @@ func (gceCS *GCEControllerServer) ControllerExpandVolume(ctx context.Context, re
12631261
if err != nil {
12641262
return nil, status.Errorf(codes.InvalidArgument, "ControllerExpandVolume Volume ID is invalid: %v", err.Error())
12651263
}
1266-
sourceDisk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
1267-
diskTypeForMetric = metrics.GetDiskType(sourceDisk)
12681264
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
12691265
if err != nil {
12701266
if gce.IsGCENotFoundError(err) {
12711267
return nil, status.Errorf(codes.NotFound, "ControllerExpandVolume could not find volume with ID %v: %v", volumeID, err.Error())
12721268
}
12731269
return nil, common.LoggedError("ControllerExpandVolume error repairing underspecified volume key: ", err)
12741270
}
1271+
sourceDisk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
1272+
diskTypeForMetric = metrics.GetDiskType(sourceDisk)
12751273
resizedGb, err := gceCS.CloudProvider.ResizeDisk(ctx, project, volKey, reqBytes)
12761274
if err != nil {
12771275
return nil, common.LoggedError("ControllerExpandVolume failed to resize disk: ", err)

0 commit comments

Comments
 (0)