Repository: ceph/ceph-csi
Pull request #2116 (2021-06-01 14:42:47)

Continue thick-provisioning of RBD images on CreateVolume restart (backport #2101)
This is an automatic backport of pull request #2101 done by [Mergify](https://mergify.io).


---


<details>
<summary>Mergify commands and options</summary>

<br />

More conditions and actions can be found in the [documentation](https://docs.mergify.io/).

You can also trigger Mergify actions by commenting on this pull request:

- `@Mergifyio refresh` will re-evaluate the rules
- `@Mergifyio rebase` will rebase this PR on its base branch
- `@Mergifyio update` will merge the base branch into this PR
- `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch

Additionally, on Mergify [dashboard](https://dashboard.mergify.io/) you can:

- look at your merge queues
- generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.io/
</details>



Patch:
diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go
index 43acce512..468f067c9 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()
 
@@ -257,23 +250,8 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 	found, err := rbdVol.Exists(ctx, parentVol)
 	if err != nil {
 		return nil, getGRPCErrorForCreateVolume(err)
-	}
-
-	if found {
-		if rbdSnap != nil {
-			// check if image depth is reached limit and requires flatten
-			err = checkFlatten(ctx, rbdVol, cr)
-			if err != nil {
-				return nil, err
-			}
-
-			err = rbdSnap.repairEncryptionConfig(&rbdVol.rbdImage)
-			if err != nil {
-				return nil, err
-			}
-		}
-
-		return buildCreateVolumeResponse(req, rbdVol), nil
+	} else if found {
+		return cs.repairExistingVolume(ctx, req, cr, rbdVol, rbdSnap)
 	}
 
 	err = validateRequestedVolumeSize(rbdVol, parentVol, rbdSnap, cr)
@@ -332,6 +310,47 @@ func flattenParentImage(ctx context.Context, rbdVol *rbdVolume, cr *util.Credent
 	return nil
 }
 
+// repairExistingVolume checks the existing volume or snapshot and makes sure
+// that the state is corrected to what was requested. It is needed to call this
+// 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) {
+	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 {
+			return nil, err
+		}
+
+		err = rbdSnap.repairEncryptionConfig(&rbdVol.rbdImage)
+		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
+}
+
 // check snapshots on the rbd image, as we have limit from krbd that an image
 // cannot have more than 510 snapshot at a given point of time. If the
 // snapshots are more than the `maxSnapshotsOnImage` Add a task to flatten all
@@ -1123,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/rbd_util.go b/internal/rbd/rbd_util.go
index f15ba61aa..cf3e624e1 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 {


Test patch:
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"])
+	}
+}