From 7cbad9305f71b70040509ed854f1d65c7c624d4f Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Wed, 26 May 2021 10:29:32 +0200 Subject: [PATCH] rbd: repair thick-provisioned images on CreateVolume restart Signed-off-by: Niels de Vos --- internal/rbd/controllerserver.go | 49 +++++++++++++++---- internal/rbd/controllerserver_test.go | 69 +++++++++++++++++++++++++++ internal/rbd/rbd_util.go | 37 ++++++++++++++ 3 files changed, 146 insertions(+), 9 deletions(-) create mode 100644 internal/rbd/controllerserver_test.go diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go index fda9fce24..1e2f50309 100644 --- a/internal/rbd/controllerserver.go +++ b/internal/rbd/controllerserver.go @@ -19,7 +19,6 @@ package rbd import ( "context" "errors" - "fmt" "strconv" csicommon "github.com/ceph/ceph-csi/internal/csi-common" @@ -118,13 +117,7 @@ func (cs *ControllerServer) parseVolCreateRequest(ctx context.Context, req *csi. return nil, status.Error(codes.InvalidArgument, err.Error()) } - tp := "thickProvision" - thick := req.GetParameters()[tp] - if thick != "" { - if rbdVol.ThickProvision, err = strconv.ParseBool(thick); err != nil { - return nil, fmt.Errorf("failed to parse %q: %w", tp, err) - } - } + rbdVol.ThickProvision = cs.isThickProvisionRequest(req) rbdVol.RequestName = req.GetName() @@ -322,7 +315,23 @@ func flattenParentImage(ctx context.Context, rbdVol *rbdVolume, cr *util.Credent // when the process of creating a volume was interrupted. func (cs *ControllerServer) repairExistingVolume(ctx context.Context, req *csi.CreateVolumeRequest, cr *util.Credentials, rbdVol *rbdVolume, rbdSnap *rbdSnapshot) (*csi.CreateVolumeResponse, error) { - if rbdSnap != nil { + vcs := req.GetVolumeContentSource() + + switch { + // normal CreateVolume without VolumeContentSource + case vcs == nil: + // continue/restart allocating the volume in case it + // should be thick-provisioned + if cs.isThickProvisionRequest(req) { + err := rbdVol.RepairThickProvision() + if err != nil { + return nil, status.Error(codes.Internal, err.Error()) + } + } + + // rbdVol is a restore from snapshot, rbdSnap is passed + case vcs.GetSnapshot() != nil: + // restore from snapshot imploes rbdSnap != nil // check if image depth is reached limit and requires flatten err := checkFlatten(ctx, rbdVol, cr) if err != nil { @@ -333,6 +342,10 @@ func (cs *ControllerServer) repairExistingVolume(ctx context.Context, req *csi.C if err != nil { return nil, err } + + // rbdVol is a clone from an other volume + case vcs.GetVolume() != nil: + // TODO: is there really nothing to repair on image clone? } return buildCreateVolumeResponse(req, rbdVol), nil @@ -1129,3 +1142,21 @@ func (cs *ControllerServer) ControllerExpandVolume(ctx context.Context, req *csi NodeExpansionRequired: nodeExpansion, }, nil } + +// isThickProvisionRequest returns true in case the request contains the +// `thickProvision` option set to `true`. +func (cs *ControllerServer) isThickProvisionRequest(req *csi.CreateVolumeRequest) bool { + tp := "thickProvision" + + thick, ok := req.GetParameters()[tp] + if !ok || thick == "" { + return false + } + + thickBool, err := strconv.ParseBool(thick) + if err != nil { + return false + } + + return thickBool +} diff --git a/internal/rbd/controllerserver_test.go b/internal/rbd/controllerserver_test.go new file mode 100644 index 000000000..32905a53f --- /dev/null +++ b/internal/rbd/controllerserver_test.go @@ -0,0 +1,69 @@ +/* +Copyright 2021 The Ceph-CSI Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package rbd + +import ( + "testing" + + "github.com/container-storage-interface/spec/lib/go/csi" +) + +func TestIsThickProvisionRequest(t *testing.T) { + cs := &ControllerServer{} + req := &csi.CreateVolumeRequest{ + Name: "fake", + Parameters: map[string]string{ + "unkownOption": "not-set", + }, + } + + // pass disabled/invalid values for "thickProvision" option + if cs.isThickProvisionRequest(req) { + t.Error("request is not for thick-provisioning") + } + + req.Parameters["thickProvision"] = "" + if cs.isThickProvisionRequest(req) { + t.Errorf("request is not for thick-provisioning: %s", req.Parameters["thickProvision"]) + } + + req.Parameters["thickProvision"] = "false" + if cs.isThickProvisionRequest(req) { + t.Errorf("request is not for thick-provisioning: %s", req.Parameters["thickProvision"]) + } + + req.Parameters["thickProvision"] = "off" + if cs.isThickProvisionRequest(req) { + t.Errorf("request is not for thick-provisioning: %s", req.Parameters["thickProvision"]) + } + + req.Parameters["thickProvision"] = "no" + if cs.isThickProvisionRequest(req) { + t.Errorf("request is not for thick-provisioning: %s", req.Parameters["thickProvision"]) + } + + req.Parameters["thickProvision"] = "**true**" + if cs.isThickProvisionRequest(req) { + t.Errorf("request is not for thick-provisioning: %s", req.Parameters["thickProvision"]) + } + + // only "true" should enable thick provisioning + req.Parameters["thickProvision"] = "true" + if !cs.isThickProvisionRequest(req) { + t.Errorf("request should be for thick-provisioning: %s", req.Parameters["thickProvision"]) + } +} diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go index 1b98cbc25..ab6e313c1 100644 --- a/internal/rbd/rbd_util.go +++ b/internal/rbd/rbd_util.go @@ -1441,6 +1441,43 @@ func (rv *rbdVolume) isThickProvisioned() (bool, error) { return thick, nil } +// RepairThickProvision writes zero bytes to the volume so that it will be +// completely allocated. In case the volume is already marked as +// thick-provisioned, nothing will be done. +func (rv *rbdVolume) RepairThickProvision() error { + // if the image has the thick-provisioned metadata, it has been fully + // allocated + done, err := rv.isThickProvisioned() + if err != nil { + return fmt.Errorf("failed to repair thick-provisioning of %q: %w", rv, err) + } else if done { + return nil + } + + // in case there are watchers, assume allocating is still happening in + // the background (by an other process?) + background, err := rv.isInUse() + if err != nil { + return fmt.Errorf("failed to get users of %q: %w", rv, err) + } else if background { + return fmt.Errorf("not going to restart thick-provisioning of in-use image %q", rv) + } + + // TODO: can this be improved by starting at the offset where + // allocating was aborted/restarted? + err = rv.allocate(0) + if err != nil { + return fmt.Errorf("failed to continue thick-provisioning of %q: %w", rv, err) + } + + err = rv.setThickProvisioned() + if err != nil { + return fmt.Errorf("failed to continue thick-provisioning of %q: %w", rv, err) + } + + return nil +} + func (rv *rbdVolume) listSnapshots() ([]librbd.SnapInfo, error) { image, err := rv.open() if err != nil {