Add CSI driver for gluster block by kotreshhr · Pull Request #150 · gluster/gluster-csi-driver

kotreshhr · 2019-01-16T11:48:53Z

This patch intends to add glusterblock csi driver which
exports a loopback device formatted as xfs. The current
implementaion expects a mount point as a parameter, where
the block devices will be created.

The original patch was taken from [1]

[1] master...Madhu-1:lo-block

Co-Authored-by: Poornima G pgurusid@redhat.com
Co-Authored-by: Kotresh HR khiremat@redhat.com
Signed-off-by: Poornima G pgurusid@redhat.com

Describe what this PR does
Provide some context for the reviewer

Is there anything that requires special attention?
Do you have any questions? Did you do something clever?

Related issues:
Mention any github issues relevant to this PR

centos-ci · 2019-01-16T11:51:01Z

Can one of the admins verify this patch?

Madhu-1 · 2019-01-16T12:04:57Z

add to whitelist

Madhu-1 · 2019-01-16T12:07:12Z

cmd/glusterblock/main.go

+	"fmt"
+	"os"
+
+	gfd "github.com/gluster/gluster-csi-driver/pkg/glusterblock"


gfd (gluster file driver) to fbd (gluster block driver)

can we rename glusterblock as glusterloopback

can we call it 'glustervirtualblock' (gvb) instead? I am thinking of avoiding loop from code.

Madhu-1 · 2019-01-16T12:07:57Z

cmd/glusterblock/main.go

+
+	cmd := &cobra.Command{
+		Use:   "glusterblock-csi-driver",
+		Short: "GlusterFS Block CSI driver",


GlusterFS Block CSI driver or Gluster loop back CSI driver?

glusterblock-csi-driver to glusteloopback-csi-driver ?

again, as said, lets avoid 'loop', and fine to call 'virtual'

Madhu-1 · 2019-01-16T12:10:03Z

cmd/glusterfs/main.go


 	gfd "github.com/gluster/gluster-csi-driver/pkg/glusterfs"
-	"github.com/gluster/gluster-csi-driver/pkg/glusterfs/utils"
+	"github.com/gluster/gluster-csi-driver/pkg/glusterfs/config"


what the intention behind renaming this folder name?

There were common code between glusterfs and glusterblock and were moved under "pkg/utils/common-utils.go" with package name as "utils". Hence moved config.go into respective config/config.go with package name "config"

Madhu-1 · 2019-01-16T12:10:59Z

deploy/gluster-block/kubernetes/csi-deployment.yaml

@@ -0,0 +1,295 @@
+# Copyright 2018 The Gluster CSI Authors.


this template needs to be updated to use the latest sidecar CSI images

IMO create below directory struct

example/kubernets/glusterfs/ ---> for glsuterfs
example/kubernetes/glsuter-loopback/ ----> for loopback

Madhu-1 · 2019-01-16T12:15:27Z

pkg/glusterblock/Dockerfile

@@ -0,0 +1,87 @@
+# Copyright 2018 The Gluster CSI Authors.


can we use single dockerfile for both, i feel a lot of code duplication here, can we make use of arguments during docker build?

Dpendencies are different for each file. It will be messy with if blocks. I feel having it separate is cleaner.

Madhu-1 · 2019-01-16T12:33:20Z

pkg/glusterblock/nodeserver.go

+}
+
+// NodeGetCapabilities returns the supported capabilities of the node server
+func (ns *NodeServer) NodeGetCapabilities(ctx context.Context, req *csi.NodeGetCapabilitiesRequest) (*csi.NodeGetCapabilitiesResponse, error) {


this need to be updated check #149

Madhu-1 · 2019-01-16T12:33:43Z

pkg/glusterfs/config/config.go

@@ -0,0 +1,17 @@
+package config


can be moved to common place to avoid code duplication

config of glusterfs csi and glusterblock csi is little different hence kept separate.

Madhu-1 · 2019-01-16T12:34:09Z

pkg/glusterfs/controllerserver.go


 // CreateSnapshot create snapshot of an existing PV
-func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequest) (*csi.CreateSnapshotResponse, error) {
+func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequest) (*csi.CreateSnapshotResponse, error) { //nolint: gocyclo


//nolint: gocyclo needs to be removed

Madhu-1 · 2019-01-16T12:35:31Z

pkg/utils/common-utils.go

+}
+
+// MountVolume mounts the given source at the target
+func MountVolume(srcPath string, targetPath string, fstype string, mo []string) error {


can this be used for glusterfs driver also?

Currently not being used but yes, it can be used.

Madhu-1 · 2019-01-16T12:35:55Z

pkg/utils/common-utils.go

+}
+
+// Config struct fills the parameters of request or user input
+type Config struct {


this is not required i think (avoid code duplication)

JohnStrunk · 2019-01-16T16:35:54Z

Does this supersede #105?

amarts · 2019-01-16T16:54:33Z

Does this supersede #105?

Yes, changed due to gluster/glusterd2#1439

Madhu-1 · 2019-01-21T04:21:54Z

examples/kubernetes/gluster-virtblock/csi-deployment.yaml

+          securityContext:
+            privileged: true
+            capabilities:
+              add: ["CAP_MKNOD"]


why provisioner need these capabilities? ( AFAIK this does only create and delete volume)

Good catch. Done

Madhu-1 · 2019-01-21T04:24:11Z

pkg/gluster-virtblock/controllerserver.go

+}
+
+// ParseCreateVolRequest parse incoming volume create request and fill ProvisionerConfig.
+func (cs *ControllerServer) ParseCreateVolRequest(req *csi.CreateVolumeRequest) (*ProvisionerConfig, error) {


can we remove this function as it's not doing any functionality?

and remove this one also

Madhu-1 · 2019-01-21T04:25:41Z

pkg/gluster-virtblock/controllerserver.go

+
+// CreateVolume creates and starts the volume
+func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) {
+	glog.V(2).Infof("request received %+v", req)


you need to remove logging for secrets from the request, please check #148

Madhu-1 · 2019-01-21T04:28:24Z

pkg/gluster-virtblock/controllerserver.go

+	volumeReq := bapi.BlockVolumeCreateRequest{
+		BlockVolumeInfo: &blockVolInfo,
+	}
+


I think one more validation is missing you need to reject volume creation from snapshot here.

Madhu-1 · 2019-01-21T04:29:47Z

pkg/gluster-virtblock/driver.go

+
+const (
+	glusterBlockCSIDriverName    = "org.gluster.glustervirtblock"
+	glusterBlockCSIDriverVersion = "0.0.9"


need to update driver version to 1.0.0

Madhu-1 · 2019-01-21T04:34:34Z

@JohnStrunk @humblec PTAL

humblec · 2019-01-21T06:34:15Z

build.sh

 		/profile.cov >profile.cov
 fi
+
+DRIVER="glustervirtblock-csi-driver"


The driver name looks bit confusing to me, if we meant this is a virtual block driver we could work on better names. Virt block Vs virtual block , I prefer latter one

As discussed in gcs channel, keeping it as glustervirtblock.

humblec · 2019-01-21T06:35:20Z

cmd/gluster-virtblock/main.go

+)
+
+func init() {
+	// #nosec


what is #nosec

It's an annotation to skip golint warning as error check is not done for now.

humblec · 2019-01-21T06:36:06Z

cmd/gluster-virtblock/main.go

+
+	cmd := &cobra.Command{
+		Use:   "gluster-virtual-block-csi-driver",
+		Short: "GlusterFS Virtual Block CSI driver",


There is no point in keeping GlusterFS string here, Gluster fit better

humblec · 2019-01-21T06:39:31Z

pkg/gluster-virtblock/controllerserver.go

+// ProvisionerConfig is the combined configuration of gluster cli vol create request and CSI driver specific input
+type ProvisionerConfig struct {
+	gdVolReq *api.VolCreateReq
+	//csiConf  *CsiDrvParam


may be more apt to have block volume request param

Didn't get it, could you elaborate?

please remove this structure, this is not needed at all

humblec · 2019-01-21T06:41:58Z

@kotreshhr @poornimag do we have any design doc which we can point on block driver ? if yes, please put a pointer here in this PR for reference.

humblec · 2019-01-21T06:44:52Z

cmd/gluster-virtblock/main.go

+	"os"
+
+	gvb "github.com/gluster/gluster-csi-driver/pkg/gluster-virtblock"
+	"github.com/gluster/gluster-csi-driver/pkg/gluster-virtblock/config"


I feel , either virtualblock or vb suite better than virtblock.

Also considering glusterfs and gluster vb driver use same config params of a GD2 cluster, config an outside library for both to consume once its imported.

I will do this as part code refactor patch.

humblec · 2019-01-21T06:48:13Z

examples/kubernetes/gluster-virtblock/csi-deployment.yaml

+            - name: CSI_ENDPOINT
+              value: unix://plugin/csi.sock
+            - name: REST_URL
+              value: http://glusterd2-client.gcs:24007


Whats this url all about ? Is it a constant value for any GD2 cluster?

It is just like the url in glusterfs-csi deployment file [1]. If it's deployed manually, this has to be changed which will be mentioned in README. If the deployment is part of gcs, gcs will take care of updating this.

[1]

gluster-csi-driver/examples/kubernetes/csi-deployment.yaml

Line 153 in 28876cf

value: http://192.168.121.182:24007

humblec · 2019-01-21T06:52:09Z

pkg/gluster-virtblock/nodeserver.go

+	if !found {
+		source := gs + ":" + volume
+		hostPath = path.Join(ns.Config.MntPathPrefix, volume)
+		err = utils.MountVolume(source, hostPath, "glusterfs", nil)


What are the permissions of this volume ? how do we make sure the acl is properly configured between different app pod mounts ?

Right now, it's mounted with default options. And with it host volume mount is accessible to different pods as per testing.

humblec · 2019-01-21T06:55:21Z

pkg/gluster-virtblock/nodeserver.go

+		if err != nil {
+			return nil, err
+		}
+		ns.Config.Mounts[volume] = hostPath


How do we ensure the reference counts ? also how do we make sure there is no (stale) supervol mount exist when reference count of block volumes on it is "0".

The logic of creating multiple block hosting volume is in gd2. If it can't be created in first one, second one is created. Volume context will return the correct block hosting volume and is being used. The actual deletion of block volume context, @poornimag or @oshankkumar would better answer that.

what humble meant was if you delete all block volumes why you want to keep the mount of BHV on node-plugin?

@humblec if you have multiple BHV (maybe we will end up in stale BHV mounts) that can be fixed later

Yeah its not ref counted yet. There will be stale host volume mounts if all the blocks are deleted. Yeah it can be sent as an incremental path.

humblec · 2019-01-21T06:56:36Z

@kotreshhr @poornimag we also need a README for sure :)

Madhu-1 · 2019-01-24T09:17:46Z

Gopkg.lock

-    "pkg/util/feature"
+    "pkg/util/feature",
  ]
+  pruneopts = "NUT"


@kotreshhr which dep version are you using?

[root@kotresh gluster-csi-driver $]dep version
dep:
version : v0.5.0
build date : 2018-07-26
git hash : 224a564
go version : go1.10.3
go compiler : gc
platform : linux/amd64
features : ImportDuringSolve=false

Madhu-1 · 2019-01-24T09:19:38Z

README.md

+### 1. RW0 Volume Claim (Gluster Virtual Block CSI driver)
+
+```
+[root@localhost]#kubectl create -f csi-deployment.yaml


in which directory csi-deployment.yaml is present please update

Madhu-1 · 2019-01-24T09:21:18Z

README.md

+metadata:
+  name: glustervirtblock-csi
+  annotations:
+    storageclass.kubernetes.io/is-default-class: "true"


do we need to make this storage class as default one?

@humblec @JohnStrunk ^^

I think we can't have two default storage class in the same cluster, please verify this.

Madhu-1 · 2019-01-24T09:23:51Z

README.md

+local-storage                    kubernetes.io/no-provisioner   29h
+```
+
+Unset glusterfs-csi as default CSI driver if it's default


ah, this is not required please make gluster-block storage class as non default one

Madhu-1 · 2019-01-24T09:25:33Z

README.md

+  name: glusterblock-csi-pv
+  annotations:
+    storageClassName: glustervirtblock-csi
+spec:


instead of storageClassName in annotation, use spec.StorageClassName check https://github.com/gluster/gluster-csi-driver/pull/154/files#diff-04c6e90faac2675aa89e2176d2eec7d8R119

Madhu-1 · 2019-01-24T09:26:26Z

README.md

+    requests:
+      storage: 100Mi
+
+[root@localhost]# kubectl create -f pvc.yaml


cat and create should be in separate block

Madhu-1 · 2019-01-24T09:28:08Z

README.md

+spec:
+  containers:
+  - name: smallfile
+    image: quay.io/ekuric/smallfile:latest


what this container image does, cant we use existing redis image?

Madhu-1 · 2019-01-24T09:29:19Z

README.md

+    persistentVolumeClaim:
+      claimName: glusterblock-csi-pv
+
+[root@localhost cluster]#kubectl create -f app.yaml


please keep [root@localhost] constant across readme

Madhu-1 · 2019-01-24T09:30:06Z

README.md

+NAME        READY   STATUS    RESTARTS   AGE
+gluster-0   1/1     Running   0          38s
+
+[root@localhost]# kubectl describe pods/gluster-0


instead of describe just get the kubectl get po if the pod is in running state than its fine

This patch intends to add gluster-virtblock csi driver which exports a block device provided by gluster. The current implementaion expects a mount point as a parameter, where the block devices will be created. Co-Authored-by: Poornima G <pgurusid@redhat.com> Co-Authored-by: Kotresh HR <khiremat@redhat.com> Signed-off-by: Kotresh HR <khiremat@redhat.com>

amarts

LGTM. I personally think, we should go ahead and get this moving, and make further improvement as followup PRs

Madhu-1

LGTM, need to do code cleanup once this PR is merged.

JohnStrunk

I'm ok with this. There will need to be a follow-up to clean up the build process.

Madhu-1 requested changes Jan 16, 2019

View reviewed changes

JohnStrunk mentioned this pull request Jan 16, 2019

Add CSI driver for gluster block (loopback device) #105

Closed

JohnStrunk added GCS/1.0 in progress labels Jan 17, 2019

kotreshhr force-pushed the loopback-csi-driver branch 5 times, most recently from 65a1cbc to 60beba9 Compare January 21, 2019 04:15

Madhu-1 requested changes Jan 21, 2019

View reviewed changes

humblec reviewed Jan 21, 2019

View reviewed changes

kotreshhr force-pushed the loopback-csi-driver branch 3 times, most recently from cbfdedc to 7179dc4 Compare January 24, 2019 06:41

Madhu-1 reviewed Jan 24, 2019

View reviewed changes

Madhu-1 requested changes Jan 24, 2019

View reviewed changes

kotreshhr force-pushed the loopback-csi-driver branch from 7179dc4 to 3712dda Compare January 24, 2019 10:48

kotreshhr force-pushed the loopback-csi-driver branch from 3712dda to 0e1f281 Compare January 25, 2019 06:45

amarts reviewed Jan 25, 2019

View reviewed changes

amarts approved these changes Jan 25, 2019

View reviewed changes

Madhu-1 approved these changes Jan 28, 2019

View reviewed changes

Merge branch 'master' into loopback-csi-driver

b04c56c

ghost assigned Madhu-1 Jan 28, 2019

JohnStrunk approved these changes Jan 28, 2019

View reviewed changes

JohnStrunk merged commit acec1ee into gluster:master Jan 28, 2019

ghost removed the in progress label Jan 28, 2019

JohnStrunk mentioned this pull request Jan 28, 2019

Auto-built containers don't have git version info #98

Closed

Conversation

kotreshhr commented Jan 16, 2019

Uh oh!

centos-ci commented Jan 16, 2019

Uh oh!

Madhu-1 commented Jan 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnStrunk commented Jan 16, 2019

Uh oh!

amarts commented Jan 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

humblec Jan 21, 2019 •

edited

Loading