volgen: Code based Volfile generation by aravindavk · Pull Request #497 · gluster/glusterd2

aravindavk · 2017-12-20T09:25:43Z

Sub volumes support is added to Volinfo. Code based volfile generation
is implemented(Still exploring the possible approach to do that based on
input json/yml/toml)

All volfiles available in $SRC/glusterd2/volgen2/volfile_*.go

Features:

Easy to add condition while generating volfile(For example, bitrot
volfile only contains local bricks information)
Easy to generate Cluster level, Volume level and Brick level volfiles
Adding new volfile support is easy(Add one file)
Full access to Cluster Info/Volume Info/Brick Info
All supported volfiles are available now
All generated volfiles are stored in etcd
REST endpoint available to trigger regeneration of volfiles(Selective
regeneration not yet available)

Limitations:

Currently limited to code based volfile generation. If new volfile
added or modified, Glusterd2 need to be recompiled. I tried template
based approach to support cluster volfiles but it is getting over
complecated. If this is not blocker now, we can proceed with this
approach.

Future:

Template file(json/yml/toml) support can be added on top of
this if required.

Fixes: #388
Signed-off-by: Aravinda VK avishwan@redhat.com

amarts · 2017-12-27T10:56:49Z

Currently limited to code based volfile generation. If new volfile added or modified, Glusterd2 need to be recompiled. I tried template based approach to support cluster volfiles but it is getting over complecated. If this is not blocker now, we can proceed with this approach.

Ack!

kshlm

Overall, I like the new volfile and entry types and methods associated with them. Much cleaner than what I had done. Makes it easier to write custom generator funtions than before. I see no problems inusing this new framework to generate graphs using templates, but that can be done a little later.

One major concern I have right now is that, there is a lot of code duplication. There are several patterns in use, which can all be simplified into standalone functions that can be reused. This will also help in later work to use templates.

I've mainly looked at the volgen and volinfo changes, and a little of the volume create request in this review. I've not looked at the changes done to other commands.

kshlm · 2018-01-03T12:03:02Z

-		logger.WithError(err).Error("could not prepare node list")
-		restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err.Error(), api.ErrCodeDefault)
-		return
+	var nodesMap = make(map[string]int)


You could just update nodesFromBricks to accept []Brickinfo, and pass the bricks variable you created earlier.

kshlm · 2018-01-04T05:33:15Z

-	if req.Arbiter != 0 {
-		if req.Replica != 3 || req.Arbiter != 1 {
-			return nil, errors.New("For arbiter configuration, replica count must be 3 and arbiter count must be 1. The 3rd brick of the replica will be the arbiter")
+	for idx, subvolreq := range req.Subvols {


The assumption here with the request is that the subvols are only 2 levels deep, which works for existing cases. But it will be nice if we were to support an arbitrary levels, which will help us implement a tiered volume much more easily in the future. This shouldn't need too much changes to the request format.

I do have that support in Subvol struct, but not checked that in all other places now.

I need to consider, subvol.Bricks if subvol.Subvols is nil. I will work on this part later.

I do have that support in Subvol struct, but not checked that in all other places now.

Yup. I noticed that.

kshlm · 2018-01-04T05:36:01Z

 	}

+	var bricks []brick.Brickinfo
+	for _, subvol := range volinfo.Subvols {


This pattern is repeated many times to get the bricks list of a volume. Please have a common method on the *Volinfo to get a list of bricks. This will also help us easily extend the volume subvol structure later on.

just realized now that, it is used in only two places volume-create and volume-expand. VolumeCreateReq is (VolumeCreateReq->Subvols->Bricks) and VolumeExpandReq is (VolumeExpandReq->Bricks). Not easy to have common func. I moved the code to brickutils but as two func.(duplicate code not avoided but now available in single place)

kshlm · 2018-01-04T05:47:36Z

+	)
+
+	last := volfile.RootEntry.
+		Add("protocol/server", vol, b).


This can very easily be converted to depend on the template file we have. But we can do that later.

kshlm · 2018-01-04T05:48:56Z

+	volfile.FileName = vol.Name
+
+	dht := volfile.RootEntry.Add("debug/io-stats", vol, nil).SetName(vol.Name).
+		Add("performance/io-threads", vol, nil).


All client volfiles have this section with these common xlators. There should be a single function to add these xlators that is used where required. And this can also be easily generated from a template.

I will introduce AddMany and accept list.

No I didn't mean this. What I want is a single function that adds this particular list of xlators in this particular order. That single function can be reused elsewhere.

kshlm · 2018-01-04T05:51:43Z

+
+	for subvolIdx, subvol := range vol.Subvols {
+		if subvol.Type == volume.SubvolReplicate {
+			name := fmt.Sprintf("%s-replicate-%d", vol.Name, subvolIdx)


We should not be programatically generating the names for replicate/ec/dist (cluster) xlators each time we generate volfiles. We should do this once when the volume is created, or when a new brick set is added to the volume and save it with the volinfo, and use the saved names here. This will allow us to also accept subvol names during volume create, which will help with migration from 3.x to 4.0.

Nice idea.

I do have subvol name like s1, s2.. I will change the subvol name as <volname>-<subvoltype>-<idx> in subvol struct itself

Rather than index, give it unique names, uuids. Index will become hard to keep track of after a few expansions/shrinks of the the volume.

Yes please! This is one of my primary ask. (Hence I was asking for ability to pass the distribute subvolume name in API itself (to help with backward compatibility)).

kshlm · 2018-01-04T05:53:30Z

+		Add("cluster/distribute", vol, nil)
+
+	for subvolIdx, subvol := range vol.Subvols {
+		if subvol.Type == volume.SubvolReplicate {


Why only replicate? Wouldn't EC also require this?

Also, bricks are being added to the graph only when this condition is true. So distribute volumes don't have any bricks added to the graph.

I didn't considered EC since EC support patch was in review. I will look into this condition again

kshlm · 2018-01-04T05:56:15Z

+		Add("performance/write-behind", vol, nil).
+		Add("cluster/distribute", vol, nil)
+
+	for subvolIdx, subvol := range vol.Subvols {


This should be made a common function as well. We will need to attach the cluster graph to other graphs several times.

Not done as single function since bitd volfile differs with respect to local bricks. I will give it a try now.

aravindavk · 2018-01-04T08:43:06Z

@kshlm Thanks for the comments, I will address the comments and refresh the patch soon.

aravindavk · 2018-01-09T05:50:18Z

@kshlm please review

kshlm

Need some more details on how the volfiles are actually being generated for volume create and other apis.

A few more comments about refactoring code for better readability.

And my request from the previous review about a common function to add the client xlator stack is yet to be done. If this isn't something you want to do now, please let me know.

kshlm · 2018-01-09T07:24:39Z

 			DoFunc: "vol-create.Validate",
 			Nodes:  nodes,
 		},
-		{


When exactly are volfiles generated? I see the new volfile endpoint, but I don't understand how exactly it works.

In $SRC/glusterd2/commands/volumes/common.go:storeVolume, Incremental generation of volfiles is not yet implemented. All the Volfiles will be (re)generated when volinfo is stored.(Create/Expand/Set etc)

kshlm · 2018-01-09T07:26:37Z

-		if !uuid.Equal(b.NodeID, gdctx.MyUUID) {
-			continue
-		}
+	for _, b := range vol.GetBricks(true) {


Just a small nit. The volume variable is named vol here but volinfo in the startAllBricks() function above. Isn't consistent.

Not changed the names in existing code. I will update this.

kshlm · 2018-01-09T07:35:47Z

+}
+
+// Generate generates all the volfiles(Cluster/Volume/Brick)
+func Generate() error {


This function is huge, and can be split into individual functions for generating volume volfiles, brick volfiles and cluster volfiles.

And my request from the previous review about a common function to add the client xlator stack is yet to be done. If this isn't something you want to do now, please let me know.

Only client graph and gfproxy are using common list of xlators for now. But that may differ in future. I don't see much repeatation there.

kshlm · 2018-01-09T07:45:10Z

+	Type         string
+	VolumeID     uuid.UUID
+	BrickID      uuid.UUID
+	ExtraOptions map[string]string


What's the difference between ExtraOptions and ExtraData? A comment here will help.

I will add the details

kshlm · 2018-01-09T07:45:10Z

+	"github.com/pborman/uuid"
+)
+
+var varStrRE = regexp.MustCompile(`\{\{\s*(\S+)\s*\}\}`)


Move the var string code into its own file. Keeps things much more readable.

kshlm · 2018-01-09T07:46:39Z

+
+// getValue returns value if found for provided graph.xlator.keys in the options map
+// XXX: Not possibly the best place for this
+func getValue(graph, xl string, keys []string, opts map[string]string) (string, string, bool) {


getValue and getOptions can be moved into a seperate files (utils?).

kshlm · 2018-01-09T07:59:53Z

 }

+// GetBricks returns a list of Bricks
+func (v *Volinfo) GetBricks(onlyLocal bool) []brick.Brickinfo {


It'd be nicer if this were two functions GetBricks and GetLocalBricks. Makes it much more understandable when calling from elsewhere, as you don't have any trouble understanding the true/false being passed.

This current GetBricks(bool) function can become and unexported getBricks(bool) and the newer exported GetBricks() and GetLocalBricks() functions can call it.

kshlm · 2018-01-09T08:22:18Z

 type VolExpandReq struct {
-	ReplicaCount int      `json:"replica,omitempty"`
-	Bricks       []string `json:"bricks"`
+	ReplicaCount int        `json:"replica,omitempty"`


This is okay for now. But the expected API to add bricks which aligns with the new create API, is to be able to accept a list of subvols with the new bricks/subvols. For eg.,

[ { "SubvolID": "00adc43c-6eeb-47d9-912a-12daf6cde2c7", "Bricks": [ { "NodeID": "8d1c03a7-0ac8-4072-a8c2-83f31c92ccd2", "Path": "/new/brick" } ], "Subvols": [] }, { "SubvolID": "3378e563-705d-4eb0-a9a0-cb9bf2301634", "Bricks": [ { "NodeID": "e8986e52-bb46-4bbc-91b6-0c45b0ef1872", "Path": "/new/brick" } ], "Subvols": [] } ]

This will make it slightly harder for our cli, but makes the expand api way more powerful.

This is for supporting current syntax for Volume Expand. We need to have discussion on all the Sub volume based commands. Like Add new subvol, Delete Subvol, Expand a subvol etc.

Sub volumes support is added to Volinfo. Code based volfile generation is implemented(Still exploring the possible approach to do that based on input json/yml/toml) All volfiles available in `$SRC/glusterd2/volgen2/volfile_*.go` Features: - Easy to add condition while generating volfile(For example, bitrot volfile only contains local bricks information) - Easy to generate Cluster level, Volume level and Brick level volfiles - Adding new volfile support is easy(Add one file) - Full access to Cluster Info/Volume Info/Brick Info - All supported volfiles are available now - All generated volfiles are stored in etcd - REST endpoint available to trigger regeneration of volfiles(Selective regeneration not yet available) Limitations: - Currently limited to code based volfile generation. If new volfile added or modified, Glusterd2 need to be recompiled. I tried template based approach to support cluster volfiles but it is getting over complecated. If this is not blocker now, we can proceed with this approach. Future: - Template file(json/yml/toml) support can be added on top of this if required. Fixes: #388 Signed-off-by: Aravinda VK <avishwan@redhat.com>

Signed-off-by: Aravinda VK <avishwan@redhat.com>

Merged duplicate code, simplified interface for volfiles. Now it is easy to support both interface code and template for volfile generation Signed-off-by: Aravinda VK <avishwan@redhat.com>

Signed-off-by: Aravinda VK <avishwan@redhat.com>

Added subvolume Name and ID in volume-expand Signed-off-by: Aravinda VK <avishwan@redhat.com>

Signed-off-by: Aravinda VK <avishwan@redhat.com>

Also rebased after disperse volume integration Signed-off-by: Aravinda VK <avishwan@redhat.com>

aravindavk · 2018-01-09T15:41:10Z

@kshlm updated the code and rebased again. Please review

aravindavk requested review from amarts, atinmu, kshlm and prashanthpai December 26, 2017 14:00

aravindavk mentioned this pull request Dec 27, 2017

gfproxy: Add plugin for daemon management #413

Open

kshlm suggested changes Jan 4, 2018

View reviewed changes

kshlm suggested changes Jan 9, 2018

View reviewed changes

aravindavk added 7 commits January 9, 2018 15:17

Fixed CLI issue with new format and DistReplicate fix

dc21ba7

Signed-off-by: Aravinda VK <avishwan@redhat.com>

volgen2: More code cleanups

c2deaff

Merged duplicate code, simplified interface for volfiles. Now it is easy to support both interface code and template for volfile generation Signed-off-by: Aravinda VK <avishwan@redhat.com>

volgen2: Added ID for subvolume

16bfe26

Signed-off-by: Aravinda VK <avishwan@redhat.com>

volgen2: Fixed Volfile generation after volume expand

ed3d9a7

Added subvolume Name and ID in volume-expand Signed-off-by: Aravinda VK <avishwan@redhat.com>

volgen: Rebased

9dfa25c

Signed-off-by: Aravinda VK <avishwan@redhat.com>

volgen2: Added code comments and code improvements

e9936cd

Also rebased after disperse volume integration Signed-off-by: Aravinda VK <avishwan@redhat.com>

kshlm approved these changes Jan 10, 2018

View reviewed changes

kshlm merged commit 8ef3beb into gluster:master Jan 10, 2018

aravindavk deleted the volgen2 branch January 11, 2018 06:27

Conversation

aravindavk commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amarts commented Dec 27, 2017

Uh oh!

kshlm 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!

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!

aravindavk commented Jan 4, 2018

Uh oh!

aravindavk commented Jan 9, 2018

Uh oh!

kshlm 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!

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!

aravindavk commented Dec 20, 2017 •

edited

Loading