diff --git a/driver/controller.go b/driver/controller.go index eaf221b..1cd4f26 100644 --- a/driver/controller.go +++ b/driver/controller.go @@ -281,13 +281,24 @@ func (d *Driver) ControllerPublishVolume(ctx context.Context, req *csi.Controlle }, nil } -// ControllerUnpublishVolume detaches storage from a node via UpCloud Storage service. +// ControllerUnpublishVolume is a reverse operation of ControllerPublishVolume. +// +// The Plugin SHOULD perform the work that is necessary for making the volume ready to be consumed by a different node. +// The Plugin MUST NOT assume that this RPC will be executed on the node where the volume was previously used. +// +// This RPC is typically called by the CO when the workload using the volume is being moved to a different node, +// or all the workload using the volume on a node has finished. +// +// If the volume corresponding to the volume_id or the node corresponding to node_id cannot be found by the Plugin +// and the volume can be safely regarded as ControllerUnpublished from the node, the plugin SHOULD return 0 OK. func (d *Driver) ControllerUnpublishVolume(ctx context.Context, req *csi.ControllerUnpublishVolumeRequest) (*csi.ControllerUnpublishVolumeResponse, error) { if req.VolumeId == "" { return nil, status.Error(codes.InvalidArgument, "volume ID must be provided") } - log := logWithServerContext(ctx, d.log).WithField(logVolumeIDKey, req.GetVolumeId()) - + log := logWithServerContext(ctx, d.log).WithFields(logrus.Fields{ + logVolumeIDKey: req.GetVolumeId(), + logNodeIDKey: req.GetNodeId(), + }) log.Info("getting storage by uuid") // check if volume exist before trying to detach it _, err := d.svc.getStorageByUUID(ctx, req.GetVolumeId()) @@ -298,7 +309,7 @@ func (d *Driver) ControllerUnpublishVolume(ctx context.Context, req *csi.Control return nil, err } - // TODO: If node ID is not set, the SP MUST unpublish the volume from all nodes it is published to. + // TODO: If node ID is not set, the SP MUST unpublish the volume from all nodes it is published to (ref. ControllerUnpublishVolumeRequest.NodeId). log.Info("getting server by hostname") server, err := d.svc.getServerByHostname(ctx, req.GetNodeId()) if err != nil { @@ -308,6 +319,10 @@ func (d *Driver) ControllerUnpublishVolume(ctx context.Context, req *csi.Control log.Info("detaching volume") err = d.svc.detachStorage(ctx, req.VolumeId, server.UUID) if err != nil { + if errors.Is(err, errUpCloudServerStorageNotFound) { + log.Info("volume was already detached from the node") + return &csi.ControllerUnpublishVolumeResponse{}, nil + } return nil, err } diff --git a/driver/service.go b/driver/service.go index 5173387..9a3940d 100644 --- a/driver/service.go +++ b/driver/service.go @@ -16,8 +16,9 @@ const ( ) var ( - errUpCloudStorageNotFound = errors.New("upcloud: storage not found") - errUpCloudServerNotFound = errors.New("upcloud: server not found") + errUpCloudStorageNotFound = errors.New("upcloud: storage not found") + errUpCloudServerNotFound = errors.New("upcloud: server not found") + errUpCloudServerStorageNotFound = errors.New("upcloud: server storage not found") ) type service interface { //nolint:interfacebloat // Split this to smaller piece when it makes sense code wise @@ -163,8 +164,7 @@ func (u *upCloudService) detachStorage(ctx context.Context, storageUUID, serverU return nil } } - // TODO: review error - Might this happen if ControllerUnpublishVolume is called twice on same storage UUID? - return fmt.Errorf("this shouldnt happen. serverUUID %s storageUUID %s context: %+v", serverUUID, storageUUID, sd) + return errUpCloudServerStorageNotFound } func (u *upCloudService) listStorage(ctx context.Context, zone string) ([]upcloud.Storage, error) {