From 9e55f015dea8948d91fba5e031abdf391397cfbe Mon Sep 17 00:00:00 2001
From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Date: Thu, 2 Sep 2021 00:13:37 +0530
Subject: [PATCH] rbd: avoid supplying map options on unmap

Thanks to the random unmap failure on my local machine:

I0901 17:08:37.841890 2617035 cephcmds.go:55] ID: 11 Req-ID:
0001-0024-fed5480a-f00f-417a-a51d-31d8a8144c03-0000000000000003-024983f3-0b47-11ec-8fcb-e671f0b9f58e
an error (exit status 22) occurred while running rbd args: [unmap
rbd-pool/csi-vol-024983f3-0b47-11ec-8fcb-e671f0b9f58e --device-type nbd
--options try-netlink --options reattach-timeout=300 --options
io-timeout=0]

Noticed the map args are also getting passed to/as unmap args, which is not
correct. We have separate things for mapOptions and unmapOptions. This PR
makes sure that the map args are not passed at the time of unmap.

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
---
 internal/rbd/rbd_attach.go | 64 +++++++++++++++++++++++++++-----------
 internal/rbd/rbd_util.go   | 11 +++++++
 2 files changed, 57 insertions(+), 18 deletions(-)

diff --git a/internal/rbd/rbd_attach.go b/internal/rbd/rbd_attach.go
index d5b4c6062..350e57393 100644
--- a/internal/rbd/rbd_attach.go
+++ b/internal/rbd/rbd_attach.go
@@ -243,19 +243,11 @@ func attachRBDImage(ctx context.Context, volOptions *rbdVolume, device string, c
 	return devicePath, err
 }
 
-func appendDeviceTypeAndOptions(cmdArgs []string, isNbd, isThick bool, userOptions string) []string {
-	accessType := accessTypeKRbd
-	if isNbd {
-		accessType = accessTypeNbd
-	}
+func appendNbdDeviceTypeAndOptions(cmdArgs []string, isThick bool, userOptions string) []string {
+	cmdArgs = append(cmdArgs, "--device-type", accessTypeNbd)
 
-	cmdArgs = append(cmdArgs, "--device-type", accessType)
-	if !isNbd {
-		// Enable mapping and unmapping images from a non-initial network
-		// namespace (e.g. for Multus CNI).  The network namespace must be
-		// owned by the initial user namespace.
-		cmdArgs = append(cmdArgs, "--options", "noudev")
-	} else {
+	isUnmap := CheckSliceContains(cmdArgs, "unmap")
+	if !isUnmap {
 		if !strings.Contains(userOptions, useNbdNetlink) {
 			cmdArgs = append(cmdArgs, "--options", useNbdNetlink)
 		}
@@ -265,12 +257,40 @@ func appendDeviceTypeAndOptions(cmdArgs []string, isNbd, isThick bool, userOptio
 		if !strings.Contains(userOptions, setNbdIOTimeout) {
 			cmdArgs = append(cmdArgs, "--options", fmt.Sprintf("%s=%d", setNbdIOTimeout, defaultNbdIOTimeout))
 		}
+
+		if isThick {
+			// When an image is thick-provisioned, any discard/unmap/trim
+			// requests should not free extents.
+			cmdArgs = append(cmdArgs, "--options", "notrim")
+		}
 	}
-	if isThick {
-		// When an image is thick-provisioned, any discard/unmap/trim
-		// requests should not free extents.
-		cmdArgs = append(cmdArgs, "--options", "notrim")
+
+	if userOptions != "" {
+		// userOptions is appended after, possibly overriding the above
+		// default options.
+		cmdArgs = append(cmdArgs, "--options", userOptions)
 	}
+
+	return cmdArgs
+}
+
+func appendKRbdDeviceTypeAndOptions(cmdArgs []string, isThick bool, userOptions string) []string {
+	cmdArgs = append(cmdArgs, "--device-type", accessTypeKRbd)
+
+	isUnmap := CheckSliceContains(cmdArgs, "unmap")
+	if !isUnmap {
+		if isThick {
+			// When an image is thick-provisioned, any discard/unmap/trim
+			// requests should not free extents.
+			cmdArgs = append(cmdArgs, "--options", "notrim")
+		}
+	}
+
+	// Enable mapping and unmapping images from a non-initial network
+	// namespace (e.g. for Multus CNI).  The network namespace must be
+	// owned by the initial user namespace.
+	cmdArgs = append(cmdArgs, "--options", "noudev")
+
 	if userOptions != "" {
 		// userOptions is appended after, possibly overriding the above
 		// default options.
@@ -338,7 +358,11 @@ func createPath(ctx context.Context, volOpt *rbdVolume, device string, cr *util.
 		mapArgs = appendRbdNbdCliOptions(mapArgs, volOpt.MapOptions)
 	} else {
 		mapArgs = append(mapArgs, "map", imagePath)
-		mapArgs = appendDeviceTypeAndOptions(mapArgs, isNbd, isThick, volOpt.MapOptions)
+		if isNbd {
+			mapArgs = appendNbdDeviceTypeAndOptions(mapArgs, isThick, volOpt.MapOptions)
+		} else {
+			mapArgs = appendKRbdDeviceTypeAndOptions(mapArgs, isThick, volOpt.MapOptions)
+		}
 	}
 
 	if volOpt.readOnly {
@@ -443,7 +467,11 @@ func detachRBDImageOrDeviceSpec(
 	}
 
 	unmapArgs := []string{"unmap", dArgs.imageOrDeviceSpec}
-	unmapArgs = appendDeviceTypeAndOptions(unmapArgs, dArgs.isNbd, false, dArgs.unmapOptions)
+	if dArgs.isNbd {
+		unmapArgs = appendNbdDeviceTypeAndOptions(unmapArgs, false, dArgs.unmapOptions)
+	} else {
+		unmapArgs = appendKRbdDeviceTypeAndOptions(unmapArgs, false, dArgs.unmapOptions)
+	}
 
 	_, stderr, err := util.ExecCommand(ctx, rbd, unmapArgs...)
 	if err != nil {
diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go
index 218a07a2a..2b57aff60 100644
--- a/internal/rbd/rbd_util.go
+++ b/internal/rbd/rbd_util.go
@@ -2011,3 +2011,14 @@ func getCephClientLogFileName(id, logDir, prefix string) string {
 
 	return fmt.Sprintf("%s/%s-%s.log", logDir, prefix, id)
 }
+
+// CheckSliceContains checks the slice for string.
+func CheckSliceContains(options []string, opt string) bool {
+	for _, o := range options {
+		if o == opt {
+			return true
+		}
+	}
+
+	return false
+}