gluster-block: implement expand feature interface #1702

pkalever · 2020-03-24T13:26:13Z

What does this PR achieve? Why do we need it?

Implement block volume expand interface in heketi

Notes for the reviewer

Pushing it live with the intention that I will have my code side review comments ready by that time I finish on the TODO list.

The RFE just works fine now but also check for TODO list at the end of this comment which I got from John and Talur in a demo meeting.

Before Expand:
-------------
[root@server1 ~]# heketi-cli blockvolume create --size 1
Name: blockvol_365f428f2bf4b5a37f644c239f148058
Size: 1
Volume Id: 365f428f2bf4b5a37f644c239f148058
Cluster Id: 0b5740b0e3c34e935c1cd73e8bbf3c71
Hosts: [192.168.121.42 192.168.121.68 192.168.121.155]
IQN: iqn.2016-12.org.gluster-block:afcc41a3-1c46-4102-812b-5a0767cfb16a
LUN: 0
Hacount: 3
Username:
Password:
Block Hosting Volume: 7b3e7fceae17a57e0fc9ddedcce75912

[root@server1 ~]# heketi-cli volume info 7b3e7fceae17a57e0fc9ddedcce75912 | grep "Free Size"
Free Size: 8

[root@server1 ~]# targetcli ls | grep vol_7b3e7fceae17a57e0fc9ddedcce75912
 o- blockvol_365f428f2bf4b5a37f644c239f148058 [vol_7b3e7fceae17a57e0fc9ddedcce75912@... (1.0GiB) activated]

After Expand:
------------
[root@server1 ~]# heketi-cli blockvolume expand --block-volume 365f428f2bf4b5a37f644c239f148058 --new-size 2
Name: blockvol_365f428f2bf4b5a37f644c239f148058
Size: 2
Volume Id: 365f428f2bf4b5a37f644c239f148058
Cluster Id: 0b5740b0e3c34e935c1cd73e8bbf3c71
Hosts: [192.168.121.42 192.168.121.68 192.168.121.155]
IQN: iqn.2016-12.org.gluster-block:afcc41a3-1c46-4102-812b-5a0767cfb16a
LUN: 0
Hacount: 3
Username:
Password:
Block Hosting Volume: 7b3e7fceae17a57e0fc9ddedcce75912

[root@server1 ~]# heketi-cli volume info 7b3e7fceae17a57e0fc9ddedcce75912 | grep "Free Size"
Free Size: 7

[root@server1 ~]# targetcli ls | grep vol_7b3e7fceae17a57e0fc9ddedcce75912
 o- blockvol_365f428f2bf4b5a37f644c239f148058 [vol_7b3e7fceae17a57e0fc9ddedcce75912@... (2.0GiB) activated]

TODO:

Implement Clean() and CleanDone() methods --> Done with V2
Add unit tests
defer the block volume delete in the functional test case --> Done with V2

centos-ci · 2020-03-24T13:31:04Z

Can one of the admins verify this patch?

pkalever · 2020-03-24T13:47:15Z

@phlogistonjohn @raghavendra-talur can we request the CI for running the Smoke tests? or does it already got triggered automatically?

phlogistonjohn · 2020-03-24T14:03:04Z

/ok to test

phlogistonjohn · 2020-03-24T14:05:24Z

Added 'do not merge' label for now, once TODOs are done we will remove the label.

pkalever · 2020-03-30T11:37:34Z

@phlogistonjohn @raghavendra-talur I have added new patches to this series, implementing Rollback, clean and CleanDone methods, also moved BHV delete in functional tests to a defer method. PTAL.

phlogistonjohn · 2020-03-30T14:34:42Z

Travis CI run is failing due to a go vet error. This appears to be a real issue, please investigate.

Centoc-CI run failed in TestBlockVolumeExpand. This will need investigation too.

--- FAIL: TestBlockVolumeExpand (66.95s)
    assert.go:44:
        ^MASSERT:»»»func (github.com/heketi/heketi/tests/functional/TestSmokeTest/tests.TestBlockVolumeExpand) 0x71f629
        ^M»»File /root/go/src/github.com/heketi/heketi/tests/functional/TestSmokeTest/tests/block_volume_test.go:283
        ^M»»Info: [expected volInfo.Size == 146 got: 200]

I will begin reviewing too.

phlogistonjohn

Incomplete review, but I feel there is sufficient items that need feedback so far to submit this.

phlogistonjohn · 2020-03-30T14:40:25Z

apps/glusterfs/app_block_volume.go

+			return err
+		}
+
+		if msg.Size == blockvolume.Info.Size {


These checks should be moved into the Build function of the operation. Putting the checks outside of the operation could lead to races with the checks passing at the http request level and then starting two simultaneous expand operations for the same volume.
Specifically checks that depend on the state of the volume need to happen inside the same db transaction, such that it all happens atomically.

phlogistonjohn · 2020-03-30T14:44:47Z

apps/glusterfs/block_volume_entry.go

@@ -320,6 +320,65 @@ func (v *BlockVolumeEntry) destroyFromHost(
 	return nil
 }

+func (v *BlockVolumeEntry) InfoBlockVolumeFromHost(executor executors.Executor,


The order of words in this function name is strange. Shouldn't it be something like "BlockVolumeInfoFromHost"?

phlogistonjohn · 2020-03-30T14:49:27Z

apps/glusterfs/block_volume_entry.go

+	return e
+}
+
+func (v *BlockVolumeEntry) expandBlockVolumeExec(executor executors.Executor,


What's the value-add of this function? Is the additional logging here particularly valuable or can we just call the executor function when needed and do the logging/error-handling in the callers?

Yes, for now, it only called by Exec(), I will call the executor function directly.

phlogistonjohn · 2020-03-30T14:50:03Z

apps/glusterfs/errors.go

@@ -25,6 +25,7 @@ var (
 	ErrKeyExists        = errors.New("Key already exists in the database")
 	ErrNoReplacement    = errors.New("No Replacement was found for resource requested to be removed")
 	ErrCloneBlockVol    = errors.New("Cloning of block hosting volumes is not supported")
+	ErrInvalidRequest   = errors.New("Invalid request")


Please drop this error and use one of the existing errors OR make it much much more specific.

John, I had looked at the existing errors and none of them looks like a match to give a better hint in this case, if you have any thoughts you can share here, so that I will drop it.

This description I had copied from errno.h

EINVAL Invalid argument (POSIX.1-2001).

which looked more generic, I can tune it specific to my functionality of code, but the reusability will be compromised.

Let me know what you think.

Please remove it and where it is being used generate specific error messages, similar to (or the same as) what you are currently logging. We do not have to precreate the errors.

phlogistonjohn · 2020-03-30T14:53:04Z

apps/glusterfs/operations_block_volume.go

+// with the given blockvolume entry, db connection and newsize (in GB) that the
+// blockvolume is to be expanded by.
+func NewBlockVolumeExpandOperation(
+	bvol *BlockVolumeEntry, db wdb.DB, newSize int) *BlockVolumeExpandOperation {


Net-new operations should take IDs as arguments rather than *Entry types. The reason is one of db management and being clean across db transactions and avoiding stale state. I realize that a lot of existing operations use the entry types but this has lead to quite a few interesting bugs, and probably should not have been done that way. Regardless, we should not propagate that sub-optimal pattern in new code.

phlogistonjohn · 2020-03-30T14:56:20Z

apps/glusterfs/operations_block_volume.go

+			return err
+		}
+		bve.bvol = v
+		if bve.bvol.Pending.Id != "" {


Should we have a similar check for a pending BHV? It might be the case that the BHV is being modified/deleted and so we should probably avoid any changes to the contents (and any changes to the sizes tracked on the bhv) until those changes are complete.

phlogistonjohn · 2020-03-30T15:00:50Z

apps/glusterfs/operations_block_volume.go

+		// to the db.
+		// This is only noteworthy because it is different from regular
+		// volumes which determines everything up front. Here certain
+		// values are determined by gluster-block commands.


We're probably copying the code for file volume expand a bit too closely here. This isn't the best pattern, at least as far as mutating the entry objects outside of a transaction and then expecting consistent results afterward.
A better pattern would be to capture the results needed from the system in the expand exec function in an object(s) or attributes specifically created to track what the state of the system is rather than piggyback on db entries.
This also makes it cleaner wrt removing the *Entry as an argument to initialize the operation.

John,

I have copied this piece of comments intentionally from the same file, it is part of function:

func (bvc *BlockVolumeCreateOperation) Finalize() error { ... }

I was actually unclear with what we wanted to do to fix completely.

Hence, I would like to let this PR carry as it is, if its not a serious problem, and I will definitely address it in both the places in a separate PR.

pkalever · 2020-03-31T13:40:52Z

V3:
Addressed:

Travis CI run is failing due to a go vet errors and later by go fmt errors
Centoc-CI run failed in TestBlockVolumeExpand.
The rest of the comments are addressed in a separate patch
- Moved multiple size checks to Build()
- Change function name from InfoBlockVolumeFromHost() -> BlockVolumeInfoFromHost()
- Discarded expandBlockVolumeExec() and called the executor directly from Exec()
- Used IDs as arguments rather than *Entry types at NewBlockVolumeExpandOperation()
- Added check for pending BHV along with existing pending BV in Build()

PTAL

Thanks!

pkalever · 2020-04-01T05:15:30Z

Rebased on the latest master.

phlogistonjohn · 2020-04-01T18:06:47Z

apps/glusterfs/errors.go

@@ -25,6 +25,7 @@ var (
 	ErrKeyExists        = errors.New("Key already exists in the database")
 	ErrNoReplacement    = errors.New("No Replacement was found for resource requested to be removed")
 	ErrCloneBlockVol    = errors.New("Cloning of block hosting volumes is not supported")
+	ErrInvalidRequest   = errors.New("Invalid request")


Please remove it and where it is being used generate specific error messages, similar to (or the same as) what you are currently logging. We do not have to precreate the errors.

apps/glusterfs/app_block_volume.go

phlogistonjohn · 2020-04-01T18:14:53Z

apps/glusterfs/block_volume_entry.go

+	godbc.Require(hvname != "")
+	godbc.Require(h != "")
+
+	// Request the executor to get the block volume info


redundant comment

phlogistonjohn · 2020-04-01T18:19:26Z

apps/glusterfs/operations_block_volume.go

+type ExpandCleanOpType int
+
+const (
+	OperationChangeInSize ExpandCleanOpType = iota + 1


Isn't this redundant? Can't we just get back the size and then do a comparison (currSize == dbSize)?

I'm ok with removing

phlogistonjohn · 2020-04-01T18:21:37Z

apps/glusterfs/operations_block_volume.go

+)
+
+type BlockVolExpandReclaim struct {
+	Op   ExpandCleanOpType


Ignore if we end up dropping this variable, but the name Op is very confusing to me. Correct me if I'm wrong but this is really to track if the state change occurred in the underlying volume, it's not what I'd call an "Op" maybe a 'result' or a 'status' something like that.

I'm ok with removing

phlogistonjohn · 2020-04-01T18:53:48Z

client/cli/go/cmds/block_volume.go

@@ -25,6 +25,9 @@ var (
 	bv_auth     bool
 	bv_clusters string
 	bv_ha       int
+
+	bv_newSize int


I realize the existing names have underscores in them but let us not propagate that non-idiomatic style for the new stuff. Use camelCase like typical Go code please.

phlogistonjohn · 2020-04-01T18:55:51Z

executors/cmdexec/block_volume.go

+	EXABYTE
+)
+
+func ToGigaBytes(s string) (int, error) {


Is there a way gluster-block can return its output in just bytes or GB rather than heketi code having to parse strings like this? I'm specifically concerned about rounding errors, and things like that. If we need to have this function we absolutely need thorough unit tests for just this function. Similarly, it should not be a public function (not capitalized).

Gluster-block returns Human redable formats, counting from Bytes, MiB's, GiB's ...EiB's. The block volume size in heketi start with unit GiB, i.e. heketi cannot create Block Volume with Size less than 1GiB. Were as Gluster-block allows creating a blockVolume starting from Size 512Bytes :-)

This is why we cannot make gluster-block return size in GiB's always.

Sure, I will cover the test as part of the next PR.

phlogistonjohn · 2020-04-01T19:00:59Z

executors/cmdexec/block_volume.go

+	}
+}
+
+func (c *CmdExecutor) InfoBlockVolume(host string, blockhostingvolume string,


BlockVolumeInfo?

inspired by ListBlockVolume, will take care

phlogistonjohn · 2020-04-01T19:04:20Z

executors/cmdexec/block_volume.go

+
+	// if any of the cases above set err, log it and return
+	if err != nil {
+		logger.LogError("%v", err)


At least give some context to the person who might grep the code for an error line seen in the logs.

I just copied it from the other places, np will do.

phlogistonjohn · 2020-04-01T19:04:35Z

executors/executor.go

@@ -41,6 +41,8 @@ type Executor interface {
 	SetLogLevel(level string)
 	BlockVolumeCreate(host string, blockVolume *BlockVolumeRequest) (*BlockVolumeInfo, error)
 	BlockVolumeDestroy(host string, blockHostingVolumeName string, blockVolumeName string) error
+	BlockVolumeExpand(host string, blockHostingVolumeName string, blockVolumeName string, newSize int) error
+	InfoBlockVolume(host string, blockhostingvolume string, blockVolumeName string) (*BlockVolumeInfo, error)


BlockVolumeInfo

phlogistonjohn · 2020-04-01T19:07:53Z

I just noticed that it is not in the TODO list you have, but we will need a error handling functional test at some point. Let me know if that something you need assistance with and/or want to do in a follow-up pr.

pkalever

Thanks for the detailed Comments @phlogistonjohn , Will update the patches soon.

pkalever · 2020-04-07T10:34:47Z

apps/glusterfs/block_volume_entry.go

+	godbc.Require(hvname != "")
+	godbc.Require(h != "")
+
+	// Request the executor to get the block volume info


pkalever · 2020-04-07T10:35:01Z

apps/glusterfs/errors.go

@@ -25,6 +25,7 @@ var (
 	ErrKeyExists        = errors.New("Key already exists in the database")
 	ErrNoReplacement    = errors.New("No Replacement was found for resource requested to be removed")
 	ErrCloneBlockVol    = errors.New("Cloning of block hosting volumes is not supported")
+	ErrInvalidRequest   = errors.New("Invalid request")


pkalever · 2020-04-07T10:35:17Z

apps/glusterfs/operations_block_volume.go

+type ExpandCleanOpType int
+
+const (
+	OperationChangeInSize ExpandCleanOpType = iota + 1


I'm ok with removing

pkalever · 2020-04-07T10:35:23Z

apps/glusterfs/operations_block_volume.go

+)
+
+type BlockVolExpandReclaim struct {
+	Op   ExpandCleanOpType


I'm ok with removing

pkalever · 2020-04-07T10:37:27Z

executors/executor.go

@@ -41,6 +41,8 @@ type Executor interface {
 	SetLogLevel(level string)
 	BlockVolumeCreate(host string, blockVolume *BlockVolumeRequest) (*BlockVolumeInfo, error)
 	BlockVolumeDestroy(host string, blockHostingVolumeName string, blockVolumeName string) error
+	BlockVolumeExpand(host string, blockHostingVolumeName string, blockVolumeName string, newSize int) error
+	InfoBlockVolume(host string, blockhostingvolume string, blockVolumeName string) (*BlockVolumeInfo, error)


pkalever · 2020-04-07T11:08:12Z

apps/glusterfs/operations_block_volume.go

+	}
+
+	if bv.Info.Size != gotSize {
+		bve.Reclaim.Op = OperationChangeInSize


pkalever · 2020-04-07T11:08:46Z

apps/glusterfs/operations_block_volume.go

+
+func (bve *BlockVolumeExpandOperation) CleanDone() error {
+	logger.Info("Clean is done for %v op:%v", bve.Label(), bve.op.Id)
+	if bve.Reclaim.Op == OperationChangeInSize {


pkalever · 2020-04-07T13:29:13Z

apps/glusterfs/operations_block_volume.go

+// NewBlockVolumeExpandOperation creates a new BlockVolumeExpandOperation populated
+// with the given blockvolume id, db connection and newsize (in GB) that the
+// blockvolume is to be expanded by.
+func NewBlockVolumeExpandOperation(bvolId string, db wdb.DB, newSize int) (


pkalever · 2020-04-07T14:29:37Z

apps/glusterfs/operations_block_volume.go

+			return ErrNoSpace
+		}
+
+		err = bve.bvol.expandBlockVolumeComponents(txdb, bve.newSize)


Perfectly makesense to me. Will take care.

pkalever · 2020-04-07T14:29:50Z

apps/glusterfs/operations_block_volume.go

+		if e := bve.op.Save(tx); e != nil {
+			return e
+		}
+		if e := bve.bvol.Save(tx); e != nil {


pkalever · 2020-04-07T15:13:57Z

@phlogistonjohn

Split your latest comments into 3 different patches:

V4: split updating the size of the bhv and bv
V4: minimize the state carried across function calls
V4: addressing comments from John ----> minor code style comments

I hope I had addressed all of them, incase If I had missed anything, please let me know.

TODO for the next PR:

Write Unit tests (including one for toGigaBytes() Function)
error handling functional test

Thank you! For the best-detailed review.

phlogistonjohn

We're getting very close. Just a few smallish things + I do want to see a test for the output parsing function in this PR rather than in a followup because its proper functioning is critical to the clean path behaving correctly.

phlogistonjohn · 2020-04-08T14:41:07Z

apps/glusterfs/block_volume_entry.go

+	return blockVolumeInfo, nil
+}
+
+func (bv *BlockVolumeEntry) expandBlockVolumeComponents(db wdb.DB, newSize int) error {


The more I look at this function the more I think we should get rid of it. It is only called in one place, the name is misleading (IMO), and it mutates the db state of the parent BHV only not the block volume the method is connected to.
I suggest either moving the valuable logic into a private helper function in the operation or just remove it altogether and merge the logic where needed in the operation.

phlogistonjohn · 2020-04-08T14:42:00Z

apps/glusterfs/operations_block_volume.go

@@ -376,6 +376,295 @@ func (bvc *BlockVolumeCreateOperation) CleanDone() error {
 	})
 }

+// BlockVolumeExpandOperation implements the operation functions used to
+// expand an existing blockvolume.
+type BlockVolExpandReclaim struct {


Let's lowercase this name to ensure future readers of the code that's it is private to the app logic.

phlogistonjohn · 2020-04-08T14:42:30Z

apps/glusterfs/operations_block_volume.go

+
+	// modification values
+	newSize int
+	Reclaim BlockVolExpandReclaim // gets set by Clean() call


Lowercase "reclaim" too.

phlogistonjohn · 2020-04-08T14:44:37Z

apps/glusterfs/operations_block_volume.go

+			return err
+		}
+		if bhv.Pending.Id != "" {
+			logger.LogError("Pending volume %v can not expand block volume %v", bhv.Info.Id, bve.bvolId)


The wording here is a bit awkward. How about: "Can not expand block volume %v when hosting volume %v is pending"

phlogistonjohn · 2020-04-08T14:46:52Z

apps/glusterfs/operations_block_volume.go

+		if bve.newSize == bv.Info.Size {
+			logger.LogError("Requested new-size %v is same as current block volume size %v, nothing to be done.",
+				bve.newSize, bv.Info.Size)
+			return fmt.Errorf("Invalid Request: Requested new-size %v is same as current "+


Just return the result of logger.LogError [1] here, that text is (a) better/more clearly worded (b) the message the user can see on the CLI will be more easlily matched up with content in the logs.

1 - IIRC, LogError returns an error you can just use. If I'm misremembering create the error, log it, and then return it.

phlogistonjohn · 2020-04-08T14:47:12Z

apps/glusterfs/operations_block_volume.go

+		} else if bve.newSize < bv.Info.Size {
+			logger.LogError("Requested new-size %v is less than current block volume size %v, shrinking is not allowed.",
+				bve.newSize, bv.Info.Size)
+			return fmt.Errorf("Invalid Request: Requested new-size %v is less than current "+


Same as the above block.

phlogistonjohn · 2020-04-08T14:48:20Z

apps/glusterfs/operations_block_volume.go

+
+func (bve *BlockVolumeExpandOperation) Finalize() error {
+	return bve.db.Update(func(tx *bolt.Tx) error {
+		var bv *BlockVolumeEntry


This line is redundant.

phlogistonjohn · 2020-04-08T14:50:21Z

apps/glusterfs/operations_block_volume.go

+		if err != nil {
+			return err
+		}
+		actualSize = bvolInfo.Size


Unless you have a reason not to you could just assign to bve.Reclaim.actualSize here and skip the intermediate variable. But it's not a big deal.

how silly! thanks for catching, I think this is a by-product of removing the enum before.

phlogistonjohn · 2020-04-08T14:52:15Z

apps/glusterfs/operations_block_volume.go

+			if err != nil {
+				return err
+			}
+			ReclaimHsize := bve.newSize - bve.Reclaim.actualSize


Don't capitalize this. Also, what's the "H" mean?

ok. I mean reclaimBhvSize, fixed it this way.

executors/cmdexec/block_volume.go

phlogistonjohn · 2020-04-08T14:56:30Z

@raghavendra-talur PTAL

pkalever

@phlogistonjohn thanks!

pkalever · 2020-04-09T06:38:58Z

apps/glusterfs/block_volume_entry.go

+	return blockVolumeInfo, nil
+}
+
+func (bv *BlockVolumeEntry) expandBlockVolumeComponents(db wdb.DB, newSize int) error {


pkalever · 2020-04-09T06:40:49Z

apps/glusterfs/operations_block_volume.go

@@ -376,6 +376,295 @@ func (bvc *BlockVolumeCreateOperation) CleanDone() error {
 	})
 }

+// BlockVolumeExpandOperation implements the operation functions used to
+// expand an existing blockvolume.
+type BlockVolExpandReclaim struct {


pkalever · 2020-04-09T06:41:01Z

apps/glusterfs/operations_block_volume.go

+
+	// modification values
+	newSize int
+	Reclaim BlockVolExpandReclaim // gets set by Clean() call


pkalever · 2020-04-09T06:43:23Z

apps/glusterfs/operations_block_volume.go

+			return err
+		}
+		if bhv.Pending.Id != "" {
+			logger.LogError("Pending volume %v can not expand block volume %v", bhv.Info.Id, bve.bvolId)


pkalever · 2020-04-09T07:20:38Z

apps/glusterfs/operations_block_volume.go

+		if bve.newSize == bv.Info.Size {
+			logger.LogError("Requested new-size %v is same as current block volume size %v, nothing to be done.",
+				bve.newSize, bv.Info.Size)
+			return fmt.Errorf("Invalid Request: Requested new-size %v is same as current "+


pkalever · 2020-04-09T07:23:01Z

apps/glusterfs/operations_block_volume.go

+
+func (bve *BlockVolumeExpandOperation) Finalize() error {
+	return bve.db.Update(func(tx *bolt.Tx) error {
+		var bv *BlockVolumeEntry


pkalever · 2020-04-09T07:29:17Z

apps/glusterfs/operations_block_volume.go

+		if err != nil {
+			return err
+		}
+		actualSize = bvolInfo.Size


how silly! thanks for catching, I think this is a by-product of removing the enum before.

pkalever · 2020-04-09T07:30:37Z

apps/glusterfs/operations_block_volume.go

+			if err != nil {
+				return err
+			}
+			ReclaimHsize := bve.newSize - bve.Reclaim.actualSize


ok. I mean reclaimBhvSize, fixed it this way.

executors/cmdexec/block_volume.go

pkalever · 2020-04-09T09:25:05Z

@phlogistonjohn updated with the patch fixing the latest review comments. I'm yet to add the Unit Test for toGigaBytes(), which is pending for this PR.

I will start working on the Unit Test for toGigaBytes() soon.

Thanks!

pkalever · 2020-04-09T11:51:25Z

@phlogistonjohn @raghavendra-talur added unit test for toGigaBytes()

Thanks!

phlogistonjohn · 2020-04-09T12:58:11Z

@phlogistonjohn @raghavendra-talur added unit test for toGigaBytes()

Thanks!

Test looks good with one line uncovered, which is the default case for the switch statement. If you add another section to the test that creates a bogus suffix I think you will trigger that error case and get full coverage of the function. Thanks!

phlogistonjohn · 2020-04-13T11:47:21Z

Yes please.

pkalever · 2020-04-14T07:38:09Z

@phlogistonjohn squashed them now. Travis CI crashing everytime on s390x/Go-1.1.3 attempting to run "# go get github.com/mattn/goveralls", doesn't look related to this PR anyways.

Thanks!

phlogistonjohn · 2020-04-14T11:31:49Z

Yeah. I'm thinking about changing the travis config and just disabling s390.

phlogistonjohn

Thank you for the contribution. I look forward to seeing the follow-up PRs with the functional tests, etc!

pkalever · 2020-04-14T13:06:25Z

Thank you for the contribution. I look forward to seeing the follow-up PRs with the functional tests, etc!

Well, I should actually thank you for all the quick and detailed reviews.

Yes @phlogistonjohn, as agreed before, I will spend some time this week to work on the pending list and submit them in a fresh PR.

BTW the items in the pending TODO list are:

Writing Unit tests
Writing error handling functional tests

raghavendra-talur · 2020-04-14T13:57:02Z

apps/glusterfs/operations_block_volume.go

+			return err
+		}
+
+		requiredFeeSize := bve.newSize - bv.Info.Size


Small typo but it also gives a new meaning to the statement.

raghavendra-talur · 2020-04-14T14:02:45Z

apps/glusterfs/operations_block_volume.go

+				return err
+			}
+		}
+		if err := bve.op.Save(tx); err != nil {


Should this be a delete of the operation instead of a save?

I agree. This does look wrong to me now that you point it out. Let's fix this now.
This is another reason why I don't love doing the tests in a seperate PR because we should have a check in the tests that ensures no pending ops after the clean + clean done functions are run. Still not going to insist on that but this is why we do tests and not just reviews. :-)

Good catch.

phlogistonjohn · 2020-04-14T14:09:58Z

apps/glusterfs/operations_block_volume.go

+				return err
+			}
+		}
+		if err := bve.op.Save(tx); err != nil {


I agree. This does look wrong to me now that you point it out. Let's fix this now.
This is another reason why I don't love doing the tests in a seperate PR because we should have a check in the tests that ensures no pending ops after the clean + clean done functions are run. Still not going to insist on that but this is why we do tests and not just reviews. :-)

phlogistonjohn · 2020-04-14T14:10:22Z

apps/glusterfs/operations_block_volume.go

+				return err
+			}
+		} else {
+			logger.Info("Actual Size match newly requested Size %v", bve.reclaim.actualSize)


Just noticed a minor grammar error here. Should be " Actual Size matches newly requested Size "

pkalever

I will post a patch addressing the review comments soon.

Thanks!

pkalever · 2020-04-15T10:22:24Z

apps/glusterfs/operations_block_volume.go

+			return err
+		}
+
+		requiredFeeSize := bve.newSize - bv.Info.Size


pkalever · 2020-04-15T10:22:35Z

apps/glusterfs/operations_block_volume.go

+				return err
+			}
+		} else {
+			logger.Info("Actual Size match newly requested Size %v", bve.reclaim.actualSize)


pkalever · 2020-04-15T10:23:16Z

apps/glusterfs/operations_block_volume.go

+				return err
+			}
+		}
+		if err := bve.op.Save(tx); err != nil {


Good catch.

pkalever · 2020-04-15T10:40:55Z

@phlogistonjohn @raghavendra-talur updated the patch addressing the review comments. Once the reviewing is done, will squash it and resend.

Thanks!

phlogistonjohn · 2020-04-15T14:18:29Z

Latest changes look OK to me. Please squash at your earliest convenience.

Before Expand: ------------- [root@server1 ~]# heketi-cli blockvolume create --size 1 Name: blockvol_365f428f2bf4b5a37f644c239f148058 Size: 1 Volume Id: 365f428f2bf4b5a37f644c239f148058 Cluster Id: 0b5740b0e3c34e935c1cd73e8bbf3c71 Hosts: [192.168.121.42 192.168.121.68 192.168.121.155] IQN: iqn.2016-12.org.gluster-block:afcc41a3-1c46-4102-812b-5a0767cfb16a LUN: 0 Hacount: 3 Username: Password: Block Hosting Volume: 7b3e7fceae17a57e0fc9ddedcce75912 [root@server1 ~]# heketi-cli volume info 7b3e7fceae17a57e0fc9ddedcce75912 | grep "Free Size" Free Size: 8 [root@server1 ~]# targetcli ls | grep vol_7b3e7fceae17a57e0fc9ddedcce75912 o- blockvol_365f428f2bf4b5a37f644c239f148058 [vol_7b3e7fceae17a57e0fc9ddedcce75912@... (1.0GiB) activated] After Expand: ------------ [root@server1 ~]# heketi-cli blockvolume expand --block-volume 365f428f2bf4b5a37f644c239f148058 --new-size 2 Name: blockvol_365f428f2bf4b5a37f644c239f148058 Size: 2 Volume Id: 365f428f2bf4b5a37f644c239f148058 Cluster Id: 0b5740b0e3c34e935c1cd73e8bbf3c71 Hosts: [192.168.121.42 192.168.121.68 192.168.121.155] IQN: iqn.2016-12.org.gluster-block:afcc41a3-1c46-4102-812b-5a0767cfb16a LUN: 0 Hacount: 3 Username: Password: Block Hosting Volume: 7b3e7fceae17a57e0fc9ddedcce75912 [root@server1 ~]# heketi-cli volume info 7b3e7fceae17a57e0fc9ddedcce75912 | grep "Free Size" Free Size: 7 [root@server1 ~]# targetcli ls | grep vol_7b3e7fceae17a57e0fc9ddedcce75912 o- blockvol_365f428f2bf4b5a37f644c239f148058 [vol_7b3e7fceae17a57e0fc9ddedcce75912@... (2.0GiB) activated] Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever · 2020-04-15T14:28:25Z

Latest changes look OK to me. Please squash at your earliest convenience.

@phlogistonjohn done now. Thanks!

phlogistonjohn · 2020-04-15T18:10:34Z

retest this please

phlogistonjohn · 2020-04-15T20:22:43Z

retest this please

In PR heketi#1702 we added a new OperationType and ChangeType. However, it renumbered the consts. In cases where heketi is upgraded when the db has pending ops, this can lead to undefined behavior. Signed-off-by: Raghavendra Talur <rtalur@redhat.com>

In PR heketi#1702 we added a new OperationType and ChangeType. However, it renumbered the enums. In cases where heketi is upgraded when the db has pending ops, this can lead to undefined behavior. Signed-off-by: Raghavendra Talur <rtalur@redhat.com>

In PR #1702 we added a new OperationType and ChangeType. However, it renumbered the enums. In cases where heketi is upgraded when the db has pending ops, this can lead to undefined behavior. Signed-off-by: Raghavendra Talur <rtalur@redhat.com>

In PR #1702 we added a new OperationType and ChangeType. However, it renumbered the enums. In cases where heketi is upgraded when the db has pending ops, this can lead to undefined behavior. Signed-off-by: Raghavendra Talur <rtalur@redhat.com> (cherry picked from commit 9c9c572)

phlogistonjohn added the do not merge label Mar 24, 2020

pkalever force-pushed the expand-bv-rfe branch from f66c44e to d80cce2 Compare March 30, 2020 11:34

pkalever changed the title ~~[RFC] gluster-block: implement expand feature interface~~ gluster-block: implement expand feature interface Mar 30, 2020

pkalever mentioned this pull request Mar 30, 2020

resize: update the size in metafile soon after ResizeEntry gluster/gluster-block#267

Merged

phlogistonjohn suggested changes Mar 30, 2020

View reviewed changes

pkalever force-pushed the expand-bv-rfe branch 3 times, most recently from 466741e to 1814c15 Compare March 31, 2020 13:38

pkalever requested a review from phlogistonjohn March 31, 2020 13:48

pkalever force-pushed the expand-bv-rfe branch from 1814c15 to f190516 Compare April 1, 2020 05:14

phlogistonjohn suggested changes Apr 1, 2020

View reviewed changes

pkalever commented Apr 7, 2020

View reviewed changes

pkalever force-pushed the expand-bv-rfe branch from 8a744e0 to 3cf945e Compare April 7, 2020 15:05

phlogistonjohn suggested changes Apr 8, 2020

View reviewed changes

phlogistonjohn requested a review from raghavendra-talur April 8, 2020 14:56

pkalever commented Apr 9, 2020

View reviewed changes

pkalever force-pushed the expand-bv-rfe branch from 4912576 to 3ae7a14 Compare April 14, 2020 07:27

phlogistonjohn approved these changes Apr 14, 2020

View reviewed changes

raghavendra-talur reviewed Apr 14, 2020

View reviewed changes

raghavendra-talur approved these changes Apr 14, 2020

View reviewed changes

raghavendra-talur reviewed Apr 14, 2020

View reviewed changes

phlogistonjohn suggested changes Apr 14, 2020

View reviewed changes

pkalever commented Apr 15, 2020

View reviewed changes

Prasanna Kumar Kalever added 3 commits April 15, 2020 19:55

unit-test: Add unit test for toGigaBytes function

4c6e1a7

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

functional tests: add tests for block volume expand

3518337

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever force-pushed the expand-bv-rfe branch from 817a576 to 3518337 Compare April 15, 2020 14:27

pkalever requested a review from phlogistonjohn April 15, 2020 14:28

phlogistonjohn approved these changes Apr 15, 2020

View reviewed changes

phlogistonjohn merged commit 65462a5 into heketi:master Apr 16, 2020

phlogistonjohn mentioned this pull request Jul 9, 2020

Implement expansion for block-volumes #1319

Closed

raghavendra-talur mentioned this pull request Dec 4, 2020

apps: revert the renumbering of enum in pendingop #1815

Merged

gluster-block: implement expand feature interface #1702

gluster-block: implement expand feature interface #1702

Conversation

pkalever commented Mar 24, 2020 • edited

What does this PR achieve? Why do we need it?

Notes for the reviewer

centos-ci commented Mar 24, 2020

pkalever commented Mar 24, 2020

phlogistonjohn commented Mar 24, 2020

phlogistonjohn commented Mar 24, 2020

pkalever commented Mar 30, 2020

phlogistonjohn commented Mar 30, 2020

phlogistonjohn 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

pkalever commented Mar 31, 2020

pkalever commented Apr 1, 2020

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

phlogistonjohn commented Apr 1, 2020

pkalever 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

pkalever commented Apr 7, 2020

phlogistonjohn 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

pkalever commented Mar 24, 2020 •

edited