Change in vdsm[master]: vm: Update LUN size when starting a vm

Daniel Erez has uploaded a new change for review.

Change subject: vm: Update LUN size when starting a vm
......................................................................

vm: Update LUN size when starting a vm

In order to handle resize of DirectLUN disks
(i.e. when a LUN volume is extended by user),
relevant data should be retrieved when invoking getVmStats.

Hence, added the following information to disks stats for
DirectLUN disks: truesize, apparentsize and lunGUID
(the sizes values already exist but are zeroed).
Using these values, the engine could keep the DB updated.

The following changes are introduced:
* hsm:
  appropriateDevice method now fetches LUN's device size
  using 'multipath -> getDeviceSize'.
* clientIF:
  For LUN devices, updated drive's truesize/apparentsize
  using hsm's getVolumeSize method.
* vm:
  _getDiskStats -> in order to identify each LUN
  (e.g. on engine side), added 'lunGUID' to dStats
  when vmDrive.GUID exists (applicable for DirectLUN disks).

Change-Id: Ia4988212f7f96078e774d2a4e7b5cd1681383cb0
Bug-Url: https://bugzilla.redhat.com/1026868
Signed-off-by: Daniel Erez 
---
M vdsm/clientIF.py
M vdsm/storage/hsm.py
M vdsm/vm.py
3 files changed, 11 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/35/24235/1

diff --git a/vdsm/clientIF.py b/vdsm/clientIF.py
index 894a5d7..d0095e4 100644
--- a/vdsm/clientIF.py
+++ b/vdsm/clientIF.py
@@ -263,6 +263,10 @@
                 if res['status']['code']:
                     raise vm.VolumeError(drive)

+                # Update size for LUN volume
+                drive["truesize"] = res['truesize']
+                drive["apparentsize"] = res['apparentsize']
+
                 volPath = os.path.join("/dev/mapper", drive["GUID"])

# UUID drive format
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 53c9dd0..15485e2 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -3107,6 +3107,11 @@
                     expectedException=OSError,
                     timeout=QEMU_READABLE_TIMEOUT)

+        # Get the size of the logical unit volume.
+        size = str(multipath.getDeviceSize(devicemapper.getDmId(guid)))
+
+        return dict(truesize=size, apparentsize=size)
+
     @public
     def inappropriateDevices(self, thiefId):
         """
diff --git a/vdsm/vm.py b/vdsm/vm.py
index aae8bd6..295814e 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -631,6 +631,8 @@
                           'apparentsize': str(vmDrive.apparentsize)}
                 if isVdsmImage(vmDrive):
                     dStats['imageID'] = vmDrive.imageID
+                else:
+                    dStats['lunGUID'] = vmDrive.GUID
                 dStats['readRate'] = ((eInfo[dName][1] - sInfo[dName][1]) /
                                       sampleInterval)
                 dStats['writeRate'] = ((eInfo[dName][3] - sInfo[dName][3]) /

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Update LUN size when starting a vm
......................................................................

Patch Set 1: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_localfs/129/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6258/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7037/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7148/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_nfs/68/ : FAILURE

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Update LUN size when starting a vm
......................................................................

Patch Set 1: Code-Review-1

(4 comments)

Some issue to fix in the implementation.

http://gerrit.ovirt.org/#/c/24235/1/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 259:                 if not res["visible"][drive["GUID"]]:
Line 260:                     raise vm.VolumeError(drive)
Line 261: 
Line 262:                 res = self.irs.appropriateDevice(drive["GUID"], vmId)
Line 263:                 if res['status']['code']:
This will raise KeyError now, since you are not returning standard status reply with zero (falsy) status code.
Line 264:                     raise vm.VolumeError(drive)
Line 265: 
Line 266:                 # Update size for LUN volume
Line 267:                 drive["truesize"] = res['truesize']

http://gerrit.ovirt.org/#/c/24235/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3106:         utils.retry(partial(fileUtils.validateQemuReadable, devPath),
Line 3107:                     expectedException=OSError,
Line 3108:                     timeout=QEMU_READABLE_TIMEOUT)
Line 3109: 
Line 3110:         # Get the size of the logical unit volume.
This comment does not add any information or value. The code is explaining itself.

The only comment needed is why we convert the size to a string. The reason is that xmlrpc cannot handle big integers, and our xmlrpc code is not smart enough to do the conversion when needed.

Since this is an internal API, and this code is never called by the xmlrpc server, there is no need to convert the value to string here. But since we need to use the standard xmlrpc return value (see next comment), it may be better to keep it consistent with other methods and keep the str conversion.
Line 3111:         size = str(multipath.getDeviceSize(devicemapper.getDmId(guid)))
Line 3112: 
Line 3113:         return dict(truesize=size, apparentsize=size)
Line 3114:

Line 3109: 
Line 3110:         # Get the size of the logical unit volume.
Line 3111:         size = str(multipath.getDeviceSize(devicemapper.getDmId(guid)))
Line 3112: 
Line 3113:         return dict(truesize=size, apparentsize=size)
The code calling this expect a standard xmlrpc reply:

{"status": {"code": some code, "message": error message...}}

If this fails in line 3106 (utils.retry), the @public wrapper will generate an error response for you, so it seems that the best approach is to use the same format to return positive result.

Check how other methods return data *and* status.
Line 3114: 
Line 3115:     @public
Line 3116:     def inappropriateDevices(self, thiefId):
Line 3117:         """

http://gerrit.ovirt.org/#/c/24235/1/vdsm/vm.py
File vdsm/vm.py:

Line 630:                 dStats = {'truesize': str(vmDrive.truesize),
Line 631:                           'apparentsize': str(vmDrive.apparentsize)}
Line 632:                 if isVdsmImage(vmDrive):
Line 633:                     dStats['imageID'] = vmDrive.imageID
Line 634:                 else:
Is this always correct? all devices are either vdsmimage or have a GUID?

Looking at clientIF.prepareVolumePath, this seems to be false.
Line 635:                     dStats['lunGUID'] = vmDrive.GUID
Line 636:                 dStats['readRate'] = ((eInfo[dName][1] - sInfo[dName][1]) /
Line 637:                                       sampleInterval)
Line 638:                 dStats['writeRate'] = ((eInfo[dName][3] - sInfo[dName][3]) /

Daniel Erez has posted comments on this change.

Change subject: vm: Update LUN size when starting a vm
......................................................................

Patch Set 1:

(4 comments)

http://gerrit.ovirt.org/#/c/24235/1/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 259:                 if not res["visible"][drive["GUID"]]:
Line 260:                     raise vm.VolumeError(drive)
Line 261: 
Line 262:                 res = self.irs.appropriateDevice(drive["GUID"], vmId)
Line 263:                 if res['status']['code']:
> This will raise KeyError now, since you are not returning standard status r

Seems fine, the status is probably generated by public api wrapper, after the change 'res' contains:
{'status': {'message': 'OK', 'code': 0}, 'truesize': '21474836480', 'apparentsize': '21474836480'}
Line 264:                     raise vm.VolumeError(drive)
Line 265: 
Line 266:                 # Update size for LUN volume
Line 267:                 drive["truesize"] = res['truesize']

http://gerrit.ovirt.org/#/c/24235/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3106:         utils.retry(partial(fileUtils.validateQemuReadable, devPath),
Line 3107:                     expectedException=OSError,
Line 3108:                     timeout=QEMU_READABLE_TIMEOUT)
Line 3109: 
Line 3110:         # Get the size of the logical unit volume.
> This comment does not add any information or value. The code is explaining

Done
Line 3111:         size = str(multipath.getDeviceSize(devicemapper.getDmId(guid)))
Line 3112: 
Line 3113:         return dict(truesize=size, apparentsize=size)
Line 3114:

Line 3109: 
Line 3110:         # Get the size of the logical unit volume.
Line 3111:         size = str(multipath.getDeviceSize(devicemapper.getDmId(guid)))
Line 3112: 
Line 3113:         return dict(truesize=size, apparentsize=size)
> The code calling this expect a standard xmlrpc reply:

It seems that the other methods return only data. iinm, the wrapper generates also for success. The return result after the change is:
{'status': {'message': 'OK', 'code': 0}, 'truesize': '21474836480', 'apparentsize': '21474836480'}
Line 3114: 
Line 3115:     @public
Line 3116:     def inappropriateDevices(self, thiefId):
Line 3117:         """

http://gerrit.ovirt.org/#/c/24235/1/vdsm/vm.py
File vdsm/vm.py:

Line 630:                 dStats = {'truesize': str(vmDrive.truesize),
Line 631:                           'apparentsize': str(vmDrive.apparentsize)}
Line 632:                 if isVdsmImage(vmDrive):
Line 633:                     dStats['imageID'] = vmDrive.imageID
Line 634:                 else:
> Is this always correct? all devices are either vdsmimage or have a GUID?

Currently, we have either of them. I.e. we have an imageID or a GUID. I can add a check for 'GUID' existence to be on the safe side but not sure it's necessary..
Line 635:                     dStats['lunGUID'] = vmDrive.GUID
Line 636:                 dStats['readRate'] = ((eInfo[dName][1] - sInfo[dName][1]) /
Line 637:                                       sampleInterval)
Line 638:                 dStats['writeRate'] = ((eInfo[dName][3] - sInfo[dName][3]) /

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Update LUN size when starting a vm
......................................................................

Patch Set 2: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6268/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_localfs/134/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7158/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_nfs/73/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7047/ : SUCCESS

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Update LUN size when starting a vm
......................................................................

Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/24235/1/vdsm/vm.py
File vdsm/vm.py:

Line 630:                 dStats = {'truesize': str(vmDrive.truesize),
Line 631:                           'apparentsize': str(vmDrive.apparentsize)}
Line 632:                 if isVdsmImage(vmDrive):
Line 633:                     dStats['imageID'] = vmDrive.imageID
Line 634:                 else:
> Currently, we have either of them. I.e. we have an imageID or a GUID. I can

What about CD ROM and floppy? - they are in the disk devices list.
Line 635:                     dStats['lunGUID'] = vmDrive.GUID
Line 636:                 dStats['readRate'] = ((eInfo[dName][1] - sInfo[dName][1]) /
Line 637:                                       sampleInterval)
Line 638:                 dStats['writeRate'] = ((eInfo[dName][3] - sInfo[dName][3]) /

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Update LUN size when starting a vm
......................................................................

Patch Set 2: Code-Review+1

(3 comments)

Looks good with few text issues.

The check for GUID looks wrong but is "protected" by other wrong code few lines before. Fixing this require fixing the entire method and is out of the scope of this patch.

http://gerrit.ovirt.org/#/c/24235/2/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3106:         utils.retry(partial(fileUtils.validateQemuReadable, devPath),
Line 3107:                     expectedException=OSError,
Line 3108:                     timeout=QEMU_READABLE_TIMEOUT)
Line 3109: 
Line 3110:         # Get the size of the logical unit volume.
This line still adds no value.
Line 3111:         # Casting to string for keeping consistency with public methods
Line 3112:         # that use it to overcome xmlrpc integer size limitation issues.
Line 3113:         size = str(multipath.getDeviceSize(devicemapper.getDmId(guid)))
Line 3114:

Line 3107:                     expectedException=OSError,
Line 3108:                     timeout=QEMU_READABLE_TIMEOUT)
Line 3109: 
Line 3110:         # Get the size of the logical unit volume.
Line 3111:         # Casting to string for keeping consistency with public methods
Converting to string
Line 3112:         # that use it to overcome xmlrpc integer size limitation issues.
Line 3113:         size = str(multipath.getDeviceSize(devicemapper.getDmId(guid)))
Line 3114: 
Line 3115:         return dict(truesize=size, apparentsize=size)

http://gerrit.ovirt.org/#/c/24235/2/vdsm/vm.py
File vdsm/vm.py:

Line 631:                           'apparentsize': str(vmDrive.apparentsize)}
Line 632:                 if isVdsmImage(vmDrive):
Line 633:                     dStats['imageID'] = vmDrive.imageID
Line 634:                 else:
Line 635:                     dStats['lunGUID'] = vmDrive.GUID
This check is wrong, but since drives without imageID or GUID also do not have truesize attribute, they will fail earlier with AttributeError on line 630.
Line 636:                 dStats['readRate'] = ((eInfo[dName][1] - sInfo[dName][1]) /
Line 637:                                       sampleInterval)
Line 638:                 dStats['writeRate'] = ((eInfo[dName][3] - sInfo[dName][3]) /
Line 639:                                        sampleInterval)

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: Update LUN size when starting a vm
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.ovirt.org/#/c/24235/2/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 266:                 # Update size for LUN volume
Line 267:                 drive["truesize"] = res['truesize']
Line 268:                 drive["apparentsize"] = res['apparentsize']
Line 269: 
Line 270:                 volPath = os.path.join("/dev/mapper", drive["GUID"])
Can you send a patch (on top of this one) to get this as well? volPath should be returned by appropriateDevice.
Line 271: 
Line 272:             # UUID drive format
Line 273:             elif "UUID" in drive:
Line 274:                 volPath = self._getUUIDSpecPath(drive["UUID"])

Daniel Erez has posted comments on this change.

Change subject: vm: Update LUN size when starting a vm
......................................................................

Patch Set 2: Verified+1

(1 comment)

http://gerrit.ovirt.org/#/c/24235/2/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 266:                 # Update size for LUN volume
Line 267:                 drive["truesize"] = res['truesize']
Line 268:                 drive["apparentsize"] = res['apparentsize']
Line 269: 
Line 270:                 volPath = os.path.join("/dev/mapper", drive["GUID"])
> Can you send a patch (on top of this one) to get this as well? volPath shou

sure, I'll send it in a following patch
Line 271: 
Line 272:             # UUID drive format
Line 273:             elif "UUID" in drive:
Line 274:                 volPath = self._getUUIDSpecPath(drive["UUID"])

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm: Update LUN size when starting a vm
......................................................................

vm: Update LUN size when starting a vm

In order to handle resize of DirectLUN disks
(i.e. when a LUN volume is extended by user),
relevant data should be retrieved when invoking getVmStats.

Hence, added the following information to disks stats for
DirectLUN disks: truesize, apparentsize and lunGUID
(the sizes values already exist but are zeroed).
Using these values, the engine could keep the DB updated.

The following changes are introduced:
* hsm:
  appropriateDevice method now fetches LUN's device size
  using 'multipath -> getDeviceSize'.
* clientIF:
  For LUN devices, updated drive's truesize/apparentsize
  using hsm's getVolumeSize method.
* vm:
  _getDiskStats -> in order to identify each LUN
  (e.g. on engine side), added 'lunGUID' to dStats
  when vmDrive.GUID exists (applicable for DirectLUN disks).

Change-Id: Ia4988212f7f96078e774d2a4e7b5cd1681383cb0
Bug-Url: https://bugzilla.redhat.com/1026868
Signed-off-by: Daniel Erez 
Reviewed-on: http://gerrit.ovirt.org/24235
Reviewed-by: Nir Soffer 
Reviewed-by: Federico Simoncelli 
---
M vdsm/clientIF.py
M vdsm/storage/hsm.py
M vdsm/vm.py
3 files changed, 13 insertions(+), 0 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Federico Simoncelli: Looks good to me, approved
  Daniel Erez: Verified

[Allon Mureinik]

Allon Mureinik has posted comments on this change.

Change subject: vm: Update LUN size when starting a vm
......................................................................

Patch Set 3:

Daniel, please backport to 3.4.