Add loopback block provider by poornimag · Pull Request #1439 · gluster/glusterd2

poornimag · 2018-12-27T05:02:02Z

Signed-off-by: Poornima G pgurusid@redhat.com

centos-ci · 2018-12-27T05:02:04Z

Can one of the admins verify this patch?

poornimag · 2018-12-27T05:03:32Z

This patch is dependent on pr: #1357 This pr can be merged only after merging the original PR

oshankkumar · 2018-12-27T10:00:23Z

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go

+}
+
+// GlusterBlock implements block Provider interface. It represents a gluster-block
+type GlusterBlock struct {


can we have a different name for loop back provider

oshankkumar · 2018-12-27T10:02:58Z

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go

+	mounts map[string]string
+}
+
+func newGlusterBlock() (blockprovider.Provider, error) {


amarts · 2018-12-28T03:51:25Z

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go

+	}
+
+	blockFileName := hostDir + "/" + name
+	cmd := exec.Command("truncate", fmt.Sprintf("-s %d", size), blockFileName) //nolint: gosec


check if we need to add some % (or a fixed value) to this size, so we give space for mkfs.xfs ?

Ahh right ok

Use utils.ExecuteCommandRun for including stderr info in error (pkg/utils)

this is pending

Madhu-1 · 2019-01-03T06:49:46Z

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go

+
+// BlockVolume implements blockprovider.BlockVolume interface.
+// It holds information about a gluster-block volume
+type BlockVolume struct {


i think we don't need all fields in this struct

Madhu-1 · 2019-01-03T06:50:00Z

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go

+}
+
+// HostAddresses returns host addresses of a gluster block vol
+func (gv *BlockVolume) HostAddresses() []string { return gv.hosts }


this functions are also not needed

Then it would complain that the interface methods are not implemented? How else to do this?

then IMO this will create an issue if we add different input structures for different provisioners, isn't it?

Yes, So we should have kept the structures different for each provider? The structure is union of all the fields of each provider. And the caller ends up calling these functions anyways as the req structure of block vol has these fields. How to go about this?

I think we need to refactor the code to achieve this.

Required for this patch?

we can skip this cleanup, please create an issue to do the cleanup. so that we won't miss it

@Madhu-1 we can take this task to define a better interface for BlockVolume to support all block providers in some other patch, for now this should be good to go.

@Madhu-1 sure I'll do that

Madhu-1 · 2019-01-03T06:53:21Z

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go

+	}
+	defer clusterLocks.UnLock(context.Background())
+
+	volInfo, err := volume.GetVolume(hostVolume)


#1357 (comment) fixing this helps to reduce duplicate code here.

atinmu · 2019-01-10T15:23:23Z

Fixes #1476

aravindavk

Looks good. one minor comment.

aravindavk · 2019-01-14T07:03:23Z

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go

+		return nil, fmt.Errorf("failed to truncate block file %s: %+v", blockFileName, err)
+	}
+
+	cmd = exec.Command("mkfs.xfs", "-f", blockFileName) //nolint: gosec


Use utils.ExecuteCommandRun for including stderr info in error

aravindavk · 2019-01-14T07:03:48Z

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go

+	}
+
+	blockFileName := hostDir + "/" + name
+	cmd := exec.Command("truncate", fmt.Sprintf("-s %d", size), blockFileName) //nolint: gosec


Use utils.ExecuteCommandRun for including stderr info in error (pkg/utils)

Madhu-1 · 2019-01-14T13:41:34Z

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go

+
+	log "github.com/sirupsen/logrus"
+	config "github.com/spf13/viper"
+	"k8s.io/kubernetes/pkg/util/mount"


@poornimag do we have Gopkg.toml changes for this one?

if you are updating the dependencies using dep ensure, Please make sure version of dep to be v0.5.0

Madhu-1 · 2019-01-14T13:52:34Z

add to whitelist

Madhu-1 · 2019-01-14T13:56:09Z

@poornimag CI failure

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go:17:2: cannot find package "k8s.io/kubernetes/pkg/util/mount" in any of:
19:24:38 	/root/go/src/github.com/gluster/glusterd2/vendor/k8s.io/kubernetes/pkg/util/mount (vendor tree)
19:24:38 	/usr/local/go/src/k8s.io/kubernetes/pkg/util/mount (from $GOROOT)
19:24:38 	/root/go/src/k8s.io/kubernetes/pkg/util/mount (from $GOPATH)
19:24:38 make: *** [build/glusterd2] Error 1
19:24:38 Build step 'Execute shell' marked build as failure

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go

Signed-off-by: Poornima G <pgurusid@redhat.com>

Madhu-1 · 2019-01-16T08:31:18Z

retest this please

Madhu-1

LGTM

ghost assigned poornimag Dec 27, 2018

ghost added the in progress label Dec 27, 2018

poornimag changed the title ~~Add loopback block provider <WIP~~ Add loopback block provider <WIP> Dec 27, 2018

oshankkumar reviewed Dec 27, 2018

View reviewed changes

amarts reviewed Dec 28, 2018

View reviewed changes

Madhu-1 reviewed Jan 3, 2019

View reviewed changes

poornimag force-pushed the gluster-block branch 2 times, most recently from 53dcdcd to bd94f7a Compare January 10, 2019 10:56

poornimag changed the title ~~Add loopback block provider <WIP>~~ Add loopback block provider Jan 10, 2019

aravindavk reviewed Jan 14, 2019

View reviewed changes

poornimag force-pushed the gluster-block branch from bd94f7a to 41e2011 Compare January 14, 2019 13:34

ghost assigned Madhu-1 Jan 14, 2019

Madhu-1 reviewed Jan 14, 2019

View reviewed changes

oshankkumar reviewed Jan 14, 2019

View reviewed changes

plugins/blockvolume/blockprovider/gluster-loopback/glusterloopback.go Outdated Show resolved Hide resolved

poornimag force-pushed the gluster-block branch 2 times, most recently from 314d966 to bed1c19 Compare January 16, 2019 05:22

Poornima G added 2 commits January 16, 2019 12:49

Add loopback block provider

3446501

Signed-off-by: Poornima G <pgurusid@redhat.com>

Add resclient api for block volume

d3131b0

Signed-off-by: Poornima G <pgurusid@redhat.com>

poornimag force-pushed the gluster-block branch from bed1c19 to d3131b0 Compare January 16, 2019 07:20

Merge branch 'master' into gluster-block

d261392

ghost assigned aravindavk Jan 16, 2019

aravindavk approved these changes Jan 16, 2019

View reviewed changes

Madhu-1 approved these changes Jan 16, 2019

View reviewed changes

Merge branch 'master' into gluster-block

22c7446

aravindavk merged commit 4103a89 into gluster:master Jan 16, 2019

ghost removed the in progress label Jan 16, 2019

amarts mentioned this pull request Jan 16, 2019

Add CSI driver for gluster block gluster/gluster-csi-driver#150

Merged

amarts mentioned this pull request Jan 24, 2019

Support RWO with Loopback devices #1476

Closed

Conversation

poornimag commented Dec 27, 2018

Uh oh!

centos-ci commented Dec 27, 2018

Uh oh!

poornimag commented Dec 27, 2018

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!

oshankkumar Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

atinmu commented Jan 10, 2019

Uh oh!

aravindavk left a comment

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!

Madhu-1 commented Jan 14, 2019

Uh oh!

Madhu-1 commented Jan 14, 2019

Uh oh!

Uh oh!

Madhu-1 commented Jan 16, 2019

Uh oh!

Madhu-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

oshankkumar Jan 14, 2019 •

edited

Loading