Repository: ceph/ceph-csi
Pull request #2101 (2021-05-26 09:41:51)

Continue thick-provisioning of RBD images on CreateVolume restart
# Describe what this PR does #

In case CreateVolume gets interrupted (by provisioner pod restart, or similar) the slow thick-provisioning is aborted. The RBD image will not have the `thick-provisioned` metadata set. Restarting CreateVolume will detect the RBD image, and return the volume which is not completely thick-provisioned.

By validating that the CreateVolume request intends to thick-provision the volume, it is possible to detect the need for the `thick-provisioned` metadata in the RBD image. When the metadata is missing, start allocating the image again.

## Is there anything that requires special attention ##

If the RBD image has watchers (`isInUse()` returns true), assume that the allocating of the image is still in progress. Because thick-provisioning is slow, Kubernetes tends to time-out the initial CreateVolume request(s). It is expected that the `not going to restart thick-provisioning of in-use image` is reported when Kubernetes restarts the CreateVolume request.

## Related issues ##

Fixes: https://bugzilla.redhat.com/1961647

---

<details>
<summary>Show available bot commands</summary>

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

- `/retest ci/centos/<job-name>`: retest the `<job-name>` after unrelated
  failure (please report the failure too!)
- `/retest all`: run this in case the CentOS CI failed to start/report any test
  progress or results

</details>


nixpanic: > I know it's tricky do you have any plan to add E2E for this case?

I am not sure how to make this a reliable test, ideas welcome.
nixpanic: @idryomov: when a watch is lost (initially added once the image was opened), can the opened image still be used for writing? I expected that the rbd-image-handle would become invalid, and writing returns a failure.

If there is a better recommendation than to check watches on RBD images, than we can surely implement it that way. Until there is a usable alternative, we probably will need to stick with the watchers for now.
idryomov: > @idryomov: when a watch is lost (initially added once the image was opened), can the opened image still be used for writing? I expected that the rbd-image-handle would become invalid, and writing returns a failure.

Yes.  A lost watch would be reestablished in due course.  Also, it may not know that the watch is lost for a while, etc.

> 
> If there is a better recommendation than to check watches on RBD images, than we can surely implement it that way. Until there is a usable alternative, we probably will need to stick with the watchers for now.

Ultimately it has to be something that would fence other potential writers at the Ceph level (`ceph osd blocklist`) before taking over.

It seems to be another instance of https://github.com/ceph/ceph-csi/issues/578 and similar issues.  Whereas before the only concern was the entity that mapped the image (since it was the only potential writer in the system), now that the provisioner can also write to the image it needs to be treated as a potential writer and be subjected to the same safeguards.

humblec: 
> Ultimately it has to be something that would fence other potential writers at the Ceph level (`ceph osd blocklist`) before taking over.
> 
> It seems to be another instance of #578 and similar issues. Whereas before the only concern was the entity that mapped the image (since it was the only potential writer in the system), now that the provisioner can also write to the image it needs to be treated as a potential writer and be subjected to the same safeguards.

Sure, ultimately `fencing` could help us here. Till we have such mechanism, I think we have to survive with `watchers` . Considering this PR improves the situation of thick provisioning to a good extent, it looks like we will get this in and implement more safe guards accordingly in near future.  
LGTM. 

@idryomov do you have final comments on this PR ? if not, we will go ahead and merge this :+1: . Thanks for reviewing!
idryomov: Nothing substantial.

Honestly, the entire scheme with k8s timing out create requests one after another and firing retries at random provisioner instances while zeroing is in progress seems very clumsy to me.  I just hope that thick provisioning isn't going to be widely used because otherwise the current implementation will bite pretty soon.
humblec: > Nothing substantial.
> 
> Honestly, the entire scheme with k8s timing out create requests one after another and firing retries at random provisioner instances while zeroing is in progress seems very clumsy to me. 
> 
I was wondering if we could resume from where previous attempt halted ( if we can get progression complete : https://github.com/ceph/ceph-csi/pull/2101#discussion_r640450199)  it could surface this problem to an extent. @idryomov is it something we can get supporte/improve  from Ceph side ? especially for operations like `--thick provision`..etc?   

>I just hope that thick provisioning isn't going to be widely used because otherwise the current implementation will bite pretty soon.

As per current reports from the field, It looks like this is going to be used in a heavy extent. This is becoming default choice for VM usecases. 

idryomov: > I was wondering if we could resume from where previous attempt halted ( if we can get progression complete : [#2101 (comment)](https://github.com/ceph/ceph-csi/pull/2101#discussion_r640450199)) it could surface this problem to an extent. @idryomov is it something we can get supporte/improve from Ceph side ? especially for operations like `--thick provision`..etc?

Theoretically you could infer where zeroing got interrupted by looking at the provisioned size (i.e. how many `rbd_data.<image id>` objects are there) or store a marker alongside your `thick-provisioned` metadata after every X gigabytes are zeroed.  But this is just as a potential optimization, not a solution for the data corruption scenario.
> 
> > I just hope that thick provisioning isn't going to be widely used because otherwise the current implementation will bite pretty soon.
> 
> As per current reports from the field, It looks like this is going to be used in a heavy extent. This is becoming default choice for VM usecases.

I wonder why?  Do you have the details or some links I could peruse?


humblec: 
> > > I just hope that thick provisioning isn't going to be widely used because otherwise the current implementation will bite pretty soon.
> > 
> > 
> > As per current reports from the field, It looks like this is going to be used in a heavy extent. This is becoming default choice for VM usecases.
> 
> I wonder why? Do you have the details or some links I could peruse?

@oritwas @mykaul if possible, can you please share the details of thick provision use cases? 
nixpanic: > As per current reports from the field, It looks like this is going to be used in a heavy extent. This is becoming default choice for VM usecases.

My impression is that this is mainly provided for users that need more accurate accounting for storage consumption. When an environment is sufficiently planned for the expected workloads, thin-provisioning would surely be the recommendation. It might be difficult to estimate actual storage consumption for VM workloads, but any deployment should make sure that sufficient capacity is available, thick-provisioned or thin.

Thick-provisioning can be used as a safety-net when an environment does not have sufficient storage capacity for the planned workload. In that case thick-provisioning can prevent workloads from starting and having a storage cluster running out of space (where recovering becomes very difficult).
humblec: As discussed in today's call, lets go ahead with this interim optimization now and catch up the improvements  on the way..  

Patch:
diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go
index 5eb283868..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()
 
@@ -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 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 {


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