Allow nodeID to be empty as defined in CSI spec

Author: hrak

pkg/cloud/cloud.go

@@ -21,7 +21,7 @@ type Interface interface {
 	CreateVolume(ctx context.Context, diskOfferingID, zoneID, name string, sizeInGB int64) (string, error)
 	DeleteVolume(ctx context.Context, id string) error
 	AttachVolume(ctx context.Context, volumeID, vmID string) (string, error)
-	DetachVolume(ctx context.Context, volumeID string) error
+	DetachVolume(ctx context.Context, volumeID string, vmID string) error
 }
 
 // Volume represents a CloudStack volume.

pkg/cloud/fake/fake.go

@@ -100,4 +100,6 @@ func (f *fakeConnector) AttachVolume(ctx context.Context, volumeID, vmID string)
 	return "1", nil
 }
 
-func (f *fakeConnector) DetachVolume(ctx context.Context, volumeID string) error { return nil }
+func (f *fakeConnector) DetachVolume(ctx context.Context, volumeID string, vmID string) error {
+	return nil
+}

pkg/cloud/volumes.go

@@ -111,11 +111,15 @@ func (c *client) AttachVolume(ctx context.Context, volumeID, vmID string) (strin
 	return strconv.FormatInt(r.Deviceid, 10), nil
 }
 
-func (c *client) DetachVolume(ctx context.Context, volumeID string) error {
+func (c *client) DetachVolume(ctx context.Context, volumeID string, vmID string) error {
 	p := c.Volume.NewDetachVolumeParams()
 	p.SetId(volumeID)
+	if vmID != "" {
+		p.SetVirtualmachineid(vmID)
+	}
 	ctxzap.Extract(ctx).Sugar().Infow("CloudStack API call", "command", "DetachVolume", "params", map[string]string{
-		"id": volumeID,
+		"id":               volumeID,
+		"virtualmachineid": vmID,
 	})
 	_, err := c.Volume.DetachVolume(p)
 	return err

pkg/driver/controller.go

@@ -252,7 +252,7 @@ func (cs *controllerServer) ControllerPublishVolume(ctx context.Context, req *cs
 	}
 
 	if _, err := cs.connector.GetVMByID(ctx, nodeID); err == cloud.ErrNotFound {
-		return nil, status.Errorf(codes.NotFound, "VM %v not found", volumeID)
+		return nil, status.Errorf(codes.NotFound, "VM %v not found", nodeID)
 	} else if err != nil {
 		// Error with CloudStack
 		return nil, status.Errorf(codes.Internal, "Error %v", err)
@@ -285,15 +285,6 @@ func (cs *controllerServer) ControllerUnpublishVolume(ctx context.Context, req *
 		return nil, status.Error(codes.InvalidArgument, "Volume ID missing in request")
 	}
 	volumeID := req.GetVolumeId()
-
-	// TODO: according to the spec, node_id is allowed to be empty:
-	//
-	// If the value is set, the SP MUST unpublish the volume from
-	// the specified node. If the value is unset, the SP MUST unpublish
-	// the volume from all nodes it is published to.
-	if req.GetNodeId() == "" {
-		return nil, status.Error(codes.InvalidArgument, "Node ID missing in request")
-	}
 	nodeID := req.GetNodeId()
 
 	// Check volume
@@ -304,8 +295,8 @@ func (cs *controllerServer) ControllerUnpublishVolume(ctx context.Context, req *
 	} else if err != nil {
 		// Error with CloudStack
 		return nil, status.Errorf(codes.Internal, "Error %v", err)
-	} else if vol.VirtualMachineID != nodeID {
-		// Nothing to do
+	} else if nodeID != "" && vol.VirtualMachineID != nodeID {
+		// Volume is present but not attached to this particular nodeID
 		return &csi.ControllerUnpublishVolumeResponse{}, nil
 	}
 
@@ -317,7 +308,7 @@ func (cs *controllerServer) ControllerUnpublishVolume(ctx context.Context, req *
 		return nil, status.Errorf(codes.Internal, "Error %v", err)
 	}
 
-	err := cs.connector.DetachVolume(ctx, volumeID)
+	err := cs.connector.DetachVolume(ctx, volumeID, nodeID)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "Cannot detach volume %s: %s", volumeID, err.Error())
 	}