Added initial creator for loopback based files #18

ShyamsundarR · 2018-02-20T18:46:19Z

Added creator for generating XFS formatted files to use as
loopback mounted PVs.

Updates #14

Signed-off-by: ShyamsundarR [email protected]

Added creator for generating XFS formatted files to use as loopback mounted PVs. Signed-off-by: ShyamsundarR <[email protected]>

Tested using local setup and scripts (which need to get in as a separate commit later). - Init fail/pass tests - Invalid unmount - Valid unmount - Valid mount - Multiple mounts - Single unmount post multiple unmounts Signed-off-by: ShyamsundarR <[email protected]>

Signed-off-by: ShyamsundarR <[email protected]>

amarts

👍

Couldn't yet get to run the scripts, just a glance on the code looks fine.

Untested! Also 1 shellcheck warning remains. Signed-off-by: ShyamsundarR <[email protected]>

ShyamsundarR · 2018-02-26T20:24:17Z

blockstore/creator/creator.sh

+    fi
+    # Create a sparse file of required volume size
+    # 2097152 = 1024 * 1024 * 1024 (GB to bytes) / 512 (obs in dd)
+    seek_end=$((volsize_gb*2097152))


Suggestion by @JohnStrunk to use 1 byte and normal 1GB math, than this 512 byte conversion, still to process.

dd bs=1 count=1 if=/dev/zero of="${blockfqpath}" seek="$((volsize_gb * 1024 * 1024 *1024))"
Don't need bs=512 since it's not actually a block device.

Ack, will update in the next commit.

ShyamsundarR · 2018-02-26T20:25:38Z

blockstore/flex-volume/install_plugin.yml

+        name: "{{ install_loc }}"
+        state: directory
+
+# TODO: Check package installation for which, jq and util-linux


Intention is to check and add util-linux package as part of the ansible play, than check for jq presence, as this avoids copying jq and such.

ShyamsundarR · 2018-02-26T20:26:17Z

blockstore/pv-recycler-pod/recycler.sh

+        echo "Failed parsing PV $pv (unable to get backing file capacity)"
+        return
+    fi
+    capacity=$(echo "$capacity_str" | sed -r "s/Gi//")


Assumption is that the capacity is always in "Gi" units, no backing document found to understand possible range of values yet!

Pretty sure other suffixes are allowed... Mi, Ti. Not sure about others, and a quick search has turned up nothing.

Marked TODO for tackling later when we move to larger PV sizes (or find enough documentation to write this cleanly)

ShyamsundarR · 2018-02-26T20:26:48Z

blockstore/pv-recycler-pod/recycler.sh

+    local scrub="${vol_root}/${subdir}/${blockfile}"
+    echo "= $(date) = Working on ${pv}"
+    echo "  Scrubbing $scrub"
+    test -e "$scrub" && rm -f "$scrub" && test -z "$(ls -A "${vol_root}/${subdir}")"


shellcheck warning exists here

I think you can just if ! rm -f "$scrub"; then

If we start with lock files per device, then we may need to ensure rm -rf, than rm -f. But can change this for now. Corrected in the next commit.

Signed-off-by: ShyamsundarR <[email protected]>

ShyamsundarR · 2018-02-27T01:11:54Z

blockstore/pv-recycler-pod/allow_root_user.txt

@@ -0,0 +1 @@
+oc adm policy add-scc-to-user anyuid system:serviceaccount:glusterfs:volrecycler-sa


@JohnStrunk I think this is incorrect, the name in the end "volrecycler-sa" may need a different specialization for block devices?

You should be able to use the same exact service account across all subvol types. It just grants the recycler pod(s) permissions to modify the PVs.

JohnStrunk

...And even a test script 💯

JohnStrunk · 2018-03-05T18:56:38Z

blockstore/creator/creator.sh

+        echo "Unable to create $dir"
+        exit 2
+    fi
+    if ! chmod 777 "$dir"; then


I'm not sure we need to open up the perms here. It exists on subvol because this last level directory needs to be writable by the random uid assigned to the pod where this acts as the PV. In this case, nothing but uid=0 touches this directory and the blockfile... creator is root, recycler is root, and so is kubelet when it calls the plugin.

Ack, will update in the next commit.

JohnStrunk · 2018-03-05T18:57:34Z

blockstore/creator/creator.sh

+        echo "Unable to create file ${blockfile}"
+        exit 2
+    fi
+    if ! chmod 777 "$blockfqpath"; then


permissions here too.

Ack, will update in the next commit.

JohnStrunk · 2018-03-05T19:00:32Z

blockstore/creator/creator.sh

+    fi
+    # Create a sparse file of required volume size
+    # 2097152 = 1024 * 1024 * 1024 (GB to bytes) / 512 (obs in dd)
+    seek_end=$((volsize_gb*2097152))


dd bs=1 count=1 if=/dev/zero of="${blockfqpath}" seek="$((volsize_gb * 1024 * 1024 *1024))"
Don't need bs=512 since it's not actually a block device.

JohnStrunk · 2018-03-05T19:03:47Z

blockstore/creator/gluster-block-subvol-sc.yml

+metadata:
+  name: gluster-block-subvol
+  annotations:
+    storageclass.kubernetes.io/is-default-class: "true"


Probably don't want this. If a user wants this as the default storageclass, they can patch it once it's been added. Having it automatically set itself as cluster default raises the possibility of accidentally having 2 that are set as default. In that case, neither acts as default and default provisioning breaks.

This yml is a helper/reminder for anyone who wants to add this as the default, right? Normally we do not need to add this as a storage class, as we use the Flexvol scheme. Isn't that right?

Assuming so, I would assume we leave this as is, for interested users to set this as a default. If my assumptions are wrong, then I would need to change it.

Yes good point. Since we use static provisioning, there is no need for the StorageClass object unless one wants it to become the default class. (not related to flex)

JohnStrunk · 2018-03-05T19:05:22Z

blockstore/flex-volume/glfs-block-subvol

+    DEBUG=1
+# if DEBUG is 2 output all commands (verbose debug mode)
+elif [ "${DEBUG}" == 2 ]; then
+    set -x


This may break the flex interface as it interacts w/ the script via stdout. (not actually sure if -x goes to stderr or whether flex monitors stderr)

This was more for my debugging, will remove it in the next commit.

JohnStrunk · 2018-03-05T19:16:41Z

blockstore/flex-volume/install_plugin.yml

+        name: "{{ install_loc }}"
+        state: directory
+
+# TODO: Check package installation for which, jq and util-linux


JohnStrunk · 2018-03-05T19:17:56Z

blockstore/pv-recycler-pod/recycler.sh

+        echo "Unable to create file ${blockfile}"
+        return 2
+    fi
+    if ! chmod 777 "$blockfqpath"; then


permissions like above

Ack, corrected in the next commit.

JohnStrunk · 2018-03-05T19:18:17Z

blockstore/pv-recycler-pod/recycler.sh

+    # Create a sparse file of required volume size
+    # 2097152 = 1024 * 1024 * 1024 (GB to bytes) / 512 (obs in dd)
+    local seek_end
+    seek_end=$((volsize_gb*2097152))


math like above

Ack, corrected in the next commit.

JohnStrunk · 2018-03-05T19:23:51Z

blockstore/pv-recycler-pod/recycler.sh

+    local scrub="${vol_root}/${subdir}/${blockfile}"
+    echo "= $(date) = Working on ${pv}"
+    echo "  Scrubbing $scrub"
+    test -e "$scrub" && rm -f "$scrub" && test -z "$(ls -A "${vol_root}/${subdir}")"


I think you can just if ! rm -f "$scrub"; then

JohnStrunk · 2018-03-05T19:36:54Z

blockstore/pv-recycler-pod/recycler.sh

+        echo "Failed parsing PV $pv (unable to get backing file capacity)"
+        return
+    fi
+    capacity=$(echo "$capacity_str" | sed -r "s/Gi//")


Pretty sure other suffixes are allowed... Mi, Ti. Not sure about others, and a quick search has turned up nothing.

Addressed provided review comments. - Couple of instances added TODO to the code - Added SELinux capability as a task list item to ensure it is tested before enabling the same During testing in a k8s cluster, on CentOS7 found and fixed a couple of script errors and assumptions. - losetup does not have -n option in CentOS - PV yml needs to be quoted whereever possible Signed-off-by: ShyamsundarR <[email protected]>

This completes creator and the flex volume provider. Signed-off-by: ShyamsundarR <[email protected]>

Further, - Updated existing recycler ansible to handle block as well - Added ansible plays to test recycler - This should ideally be useful to test the non-block case as well. - Fixed bugs found during testing Signed-off-by: ShyamsundarR <[email protected]>

JohnStrunk · 2018-03-09T20:51:19Z

blockstore/flex-volume/install_plugin.yml

-        dest: "{{ install_loc }}/jq"
-        mode: 0755
+    - name: Install required dependencies
+      yum:


instead of yum:, consider package: so it auto-chooses which package manager to use.

Yes, I had a vague remembrance of this, but could not find it when writing this up. Will do in the next patch.

ShyamsundarR added 3 commits February 20, 2018 12:34

Added initial creator for loopback based files

03d3c1d

Added creator for generating XFS formatted files to use as loopback mounted PVs. Signed-off-by: ShyamsundarR <[email protected]>

Moved block based flex vol work to its own directory

4a7088a

Signed-off-by: ShyamsundarR <[email protected]>

amarts reviewed Feb 26, 2018

View reviewed changes

blockstore: added recycler shell script

75d03e1

Untested! Also 1 shellcheck warning remains. Signed-off-by: ShyamsundarR <[email protected]>

ShyamsundarR commented Feb 26, 2018

View reviewed changes

Added Dockerfile to create block recycler image

ecba25e

Signed-off-by: ShyamsundarR <[email protected]>

ShyamsundarR commented Feb 27, 2018

View reviewed changes

JohnStrunk reviewed Mar 5, 2018

View reviewed changes

ShyamsundarR added 3 commits March 5, 2018 16:31

Addressed various TODOs and added documentation

169d53b

This completes creator and the flex volume provider. Signed-off-by: ShyamsundarR <[email protected]>

JohnStrunk reviewed Mar 9, 2018

View reviewed changes

ShyamsundarR mentioned this pull request Nov 15, 2018

loopback 'block' CSI for GCS users gluster/gcs#62

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added initial creator for loopback based files #18

Added initial creator for loopback based files #18

ShyamsundarR commented Feb 20, 2018

amarts left a comment

ShyamsundarR Feb 26, 2018

JohnStrunk Mar 5, 2018

ShyamsundarR Mar 5, 2018

ShyamsundarR Feb 26, 2018

JohnStrunk Mar 5, 2018

ShyamsundarR Feb 26, 2018

JohnStrunk Mar 5, 2018

ShyamsundarR Mar 5, 2018

ShyamsundarR Feb 26, 2018

JohnStrunk Mar 5, 2018

ShyamsundarR Mar 5, 2018

ShyamsundarR Feb 27, 2018

JohnStrunk Feb 27, 2018

JohnStrunk left a comment

JohnStrunk Mar 5, 2018

ShyamsundarR Mar 5, 2018

JohnStrunk Mar 5, 2018

ShyamsundarR Mar 5, 2018

JohnStrunk Mar 5, 2018

JohnStrunk Mar 5, 2018

ShyamsundarR Mar 5, 2018

JohnStrunk Mar 5, 2018

JohnStrunk Mar 5, 2018

ShyamsundarR Mar 5, 2018

JohnStrunk Mar 5, 2018

JohnStrunk Mar 5, 2018

ShyamsundarR Mar 5, 2018

JohnStrunk Mar 5, 2018

ShyamsundarR Mar 5, 2018

JohnStrunk Mar 5, 2018

JohnStrunk Mar 5, 2018

JohnStrunk Mar 9, 2018

ShyamsundarR Mar 12, 2018

		@@ -0,0 +1 @@
		oc adm policy add-scc-to-user anyuid system:serviceaccount:glusterfs:volrecycler-sa

Added initial creator for loopback based files #18

Are you sure you want to change the base?

Added initial creator for loopback based files #18

Conversation

ShyamsundarR commented Feb 20, 2018

amarts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnStrunk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment