Change in vdsm[master]: vm: new parameter 'protected' for vm creation

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

vm: new parameter 'protected' for vm creation

Setting the new parameter 'protected' to true when creating a VM
enables sanlock protection for the VM resources (disks).
As a result few operations on the VM are blocked as they're not
supported yet.

Change-Id: I9429ead45caac1178957a33393642817db59508f
Signed-off-by: Federico Simoncelli 
---
M client/vdsClient.py
M vdsm/storage/hsm.py
M vdsm/vm.py
M vdsm_api/vdsmapi-schema.json
4 files changed, 45 insertions(+), 22 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/14/17714/1

diff --git a/client/vdsClient.py b/client/vdsClient.py
index c52256a..c65531c 100644
--- a/client/vdsClient.py
+++ b/client/vdsClient.py
@@ -1822,7 +1822,9 @@
                     'name:val}]} : add a fully specified device',
                     'o   cpuPinning={vcpuid:pinning} cpu pinning in '
                     'libvirt-like format. see '
-                    'http://libvirt.org/formatdomain.html#elementsCPUTuning'
+                    'http://libvirt.org/formatdomain.html#elementsCPUTuning',
+                    'o   protected=<true/false> whether the vm resources '
+                    '(disks) should be protected using sanlock',
                     )),
         'vmUpdateDevice': (serv.vmUpdateDevice,
                            ('<vmId> <devicespec>',
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index c754ee8..2112693 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -3235,17 +3235,16 @@
                        'volumeID': vol.volUUID, 'path': vol.getVolumePath(),
                        'vmVolInfo': vol.getVmVolumeInfo()}

-            if config.getboolean('irs', 'use_volume_leases'):
-                leasePath, leaseOffset = dom.getVolumeLease(vol.imgUUID,
-                                                            vol.volUUID)
+            leasePath, leaseOffset = \
+                dom.getVolumeLease(vol.imgUUID, vol.volUUID)

-                if leasePath and leaseOffset is not None:
-                    volInfo.update({
-                        'leasePath': leasePath,
-                        'leaseOffset': leaseOffset,
-                        'shared': (vol.getVolType() ==
-                                   volume.type2name(volume.SHARED_VOL)),
-                    })
+            if leasePath and leaseOffset is not None:
+                volInfo.update({
+                    'leasePath': leasePath,
+                    'leaseOffset': leaseOffset,
+                    'shared': (vol.getVolType() ==
+                               volume.type2name(volume.SHARED_VOL)),
+                })

chain.append(volInfo)

diff --git a/vdsm/vm.py b/vdsm/vm.py
index dc52909..1b86eb7 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -2073,6 +2073,9 @@
     def isDisksStatsCollectionEnabled(self):
         return self._volumesPrepared

+    def isProtected(self):
+        return utils.tobool(self.conf.get('protected', 'false'))
+
     def preparePaths(self, drives):
         domains = set()
         for drive in drives:
@@ -2685,21 +2688,19 @@

self._appendDevices(domxml)

-        for drive in self._devices[DISK_DEVICES][:]:
-            if not hasattr(drive, 'volumeChain'):
-                continue
+        if (config.getboolean('irs', 'use_volume_leases')
+                or self.isProtected()):

-            for volInfo in drive.volumeChain:
-                if ('leasePath' not in volInfo or
-                        'leaseOffset' not in volInfo or
-                        volInfo['shared']):
+            for drive in self._devices[DISK_DEVICES][:]:
+                if not hasattr(drive, 'volumeChain'):
                     continue

-                leaseElem = self._buildLease(
-                    drive.domainID, volInfo['volumeID'], volInfo['leasePath'],
-                    volInfo['leaseOffset'])
+                for volInfo in drive.volumeChain:
+                    leaseElem = self._buildLease(
+                        drive.domainID, volInfo['volumeID'],
+                        volInfo['leasePath'], volInfo['leaseOffset'])

-                domxml._devices.appendChild(leaseElem)
+                    domxml._devices.appendChild(leaseElem)

return domxml.toxml()

@@ -3224,6 +3225,9 @@
         return {'status': doneCode, 'vmList': self.status()}

def hotplugDisk(self, params):
+        if self.isProtected():
+            return errCode['noimpl']
+
         if self.isMigrating():
             return errCode['migInProgress']

@@ -3271,6 +3275,9 @@
         return {'status': doneCode, 'vmList': self.status()}

def hotunplugDisk(self, params):
+        if self.isProtected():
+            return errCode['noimpl']
+
         if self.isMigrating():
             return errCode['migInProgress']

@@ -3551,6 +3558,9 @@
         disks = xml.dom.minidom.Element('disks')
         newDrives = {}

+        if self.isProtected():
+            return errCode['noimpl']
+
         if self.isMigrating():
             return errCode['migInProgress']

@@ -3726,6 +3736,9 @@
     def merge(self, mergeDrives):
         """Live merge command"""

+        if self.isProtected():
+            return errCode['noimpl']
+
         # Check if there is a merge still in progress
         for mergeStatus in self.conf.get('liveMerge', []):
             if mergeStatus['status'] == MERGESTATUS.IN_PROGRESS:
@@ -3834,6 +3847,9 @@
         del srcDrive.diskReplicate

def diskReplicateStart(self, srcDisk, dstDisk):
+        if self.isProtected():
+            return errCode['noimpl']
+
         try:
             srcDrive = self._findDriveByUUIDs(srcDisk)
         except LookupError:
@@ -3882,6 +3898,9 @@
         return {'status': doneCode}

def diskReplicateFinish(self, srcDisk, dstDisk):
+        if self.isProtected():
+            return errCode['noimpl']
+
         try:
             srcDrive = self._findDriveByUUIDs(srcDisk)
         except LookupError:
diff --git a/vdsm_api/vdsmapi-schema.json b/vdsm_api/vdsmapi-schema.json
index 4495dc5..8a9d325 100644
--- a/vdsm_api/vdsmapi-schema.json
+++ b/vdsm_api/vdsmapi-schema.json
@@ -2717,6 +2717,9 @@
 #
 # @nice:                  The host scheduling priority
 #
+# @protected:             Whether the VM resources must be protected using
+#                         sanlock or not
+#
 # @smp:                   The number of CPUs presented to the VM
 #
 # @smpCoresPerSocket:     #optional Indicates the number of CPU cores per socket

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm/vm.py
Line 2687:         domxml.appendGraphics()
Line 2688: 
Line 2689:         self._appendDevices(domxml)
Line 2690: 
Line 2691:         if (config.getboolean('irs', 'use_volume_leases')
Check here is ~wrong. Refactoring.
Line 2692:                 or self.isProtected()):
Line 2693: 
Line 2694:             for drive in self._devices[DISK_DEVICES][:]:
Line 2695:                 if not hasattr(drive, 'volumeChain'):

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm/vm.py
Line 2687:         domxml.appendGraphics()
Line 2688: 
Line 2689:         self._appendDevices(domxml)
Line 2690: 
Line 2691:         if (config.getboolean('irs', 'use_volume_leases')
Actually I wonder if we could remove use_volume_leases
Line 2692:                 or self.isProtected()):
Line 2693: 
Line 2694:             for drive in self._devices[DISK_DEVICES][:]:
Line 2695:                 if not hasattr(drive, 'volumeChain'):

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 1: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3738/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3654/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2847/ : FAILURE

[Sandro Bonazzola]

Sandro Bonazzola has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 2:

code looks good to me, but I don't know how storage works enough for giving a +1 here.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 2:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3657/ : SUCCESS

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

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 2:

(1 comment)

....................................................
File vdsm/vm.py
Line 2073:     def isDisksStatsCollectionEnabled(self):
Line 2074:         return self._volumesPrepared
Line 2075: 
Line 2076:     def isProtected(self):
Line 2077:         return (config.getboolean('irs', 'use_volume_leases')
Maybe best thing would be to set protected = true when the vm is created if use_volume_leases is true. Even better would be removing use_volume_leases completely.
Line 2078:                 or utils.tobool(self.conf.get('protected', 'false')))
Line 2079: 
Line 2080:     def preparePaths(self, drives):
Line 2081:         domains = set()

Itamar Heim has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 2:

should this be vm level or disk level?

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 2:

It is at vm level.

[Sandro Bonazzola]

Sandro Bonazzola has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 2: Code-Review+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 3: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3853/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3770/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2964/ : FAILURE

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 4: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3854/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3771/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2965/ : FAILURE

[Sandro Bonazzola]

Sandro Bonazzola has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 4: Code-Review+1

[Sandro Bonazzola]

Sandro Bonazzola has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 4:

Any chance to have this patch queue merged soon?

Yeela Kaplan has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 4:

(1 comment)

....................................................
File vdsm/vm.py
Line 1645:         """
Line 1646:         self._dom = None
Line 1647:         self.conf = {
Line 1648:             'pid': '0',
Line 1649:             'protected': config.getboolean('irs', 'use_volume_leases'),
As I understand, this should be a configuration per vm, so how can it be we take it as a constant from config.py?
Line 1650:         }
Line 1651:         self.conf.update(params)
Line 1652:         self.cif = cif
Line 1653:         self.log = SimpleLogAdapter(self.log, {"vmId": self.conf['vmId']})

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 4:

(1 comment)

....................................................
File vdsm/vm.py
Line 1645:         """
Line 1646:         self._dom = None
Line 1647:         self.conf = {
Line 1648:             'pid': '0',
Line 1649:             'protected': config.getboolean('irs', 'use_volume_leases'),
Yes it's per VM, but the default value is taken from the configuration (default: false), which means that you can override the default and set it to true for all the VMs.

If you look at the next line (1650) we update this value with the parameters coming from the engine.

Bottom line: if engine provides a value for "protected" we use such value, if it's missing then we use the default provided by 'use_volume_leases'.
Line 1650:         }
Line 1651:         self.conf.update(params)
Line 1652:         self.cif = cif
Line 1653:         self.log = SimpleLogAdapter(self.log, {"vmId": self.conf['vmId']})

Ayal Baron has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 4:

(2 comments)

....................................................
File vdsm/storage/hsm.py
Line 3255:                        'volType': "path"}
Line 3256: 
Line 3257:             leasePath, leaseOffset = dom.getVolumeLease(imgUUID, volUUID)
Line 3258: 
Line 3259:             if leasePath and leaseOffset is not None:
nit - this is checking if leasePath evaluates to True and leaseOffset is not None.  Although the result is what you expect, it is not exactly what you meant I believe.
e.g.
leasePath = True
leaseOffset = False
above would evaluate to True
Line 3260:                 volInfo.update({
Line 3261:                     'leasePath': leasePath,
Line 3262:                     'leaseOffset': leaseOffset,
Line 3263:                 })

....................................................
File vdsm/vm.py
Line 3213:                                   params=customProps)
Line 3214:         return {'status': doneCode, 'vmList': self.status()}
Line 3215: 
Line 3216:     def hotplugDisk(self, params):
Line 3217:         if self.isProtected():
you have a set of criteria which define disks which are not protected, why block these operations for such disks?
e.g. volInfo.get('shared'....
Line 3218:             return errCode['noimpl']
Line 3219: 
Line 3220:         if self.isMigrating():
Line 3221:             return errCode['migInProgress']

Eduardo has posted comments on this change.

Change subject: vm: new parameter 'protected' for vm creation
......................................................................

Patch Set 4: Code-Review-1

Please consider:

The shared element is optional and actually sent by the engine by default 'false'.

IMHO it should be used for shared = ('Exclusive', 'Shared', 'None') and we do not need extra parameters, since is sent today by default 'false'.
For BC (using use_volume_leases) set:
None if shared == 'false'.
'Shared' if shared == 'True'

Instead 'protected', the VM protected, will sent 'Exclusive'.

The required changes are minimal, in getConfDrives() set the default to None and in getXML() acting properly.
And changing the engine default when you want VMs being protected by default.

Change in vdsm[master]: vm: extend shared property to support locking

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: extend shared property to support locking
......................................................................

Patch Set 5:

(1 comment)

....................................................
File vdsm/vm.py
Line 1486:         if not self.hasVolumeLeases:
Line 1487:             return  # empty items generator
Line 1488: 
Line 1489:         # NOTE: at the moment we are generating the lease only for the leaf
Line 1490:         # when libvirt will support shared leases this will loop over all
Finish comment.
Line 1491:         for volInfo in self.volumeChain[-1:]:
Line 1492:             lease = XMLElement('lease')
Line 1493:             lease.appendChildWithArgs('key', text=volInfo['volumeID'])
Line 1494:             lease.appendChildWithArgs('lockspace',

Change in vdsm[master]: vm: extend shared property to support locking

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: extend shared property to support locking
......................................................................

Patch Set 5: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4213/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3318/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4134/ : SUCCESS

Change in vdsm[master]: vm: extend shared property to support locking

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: extend shared property to support locking
......................................................................

Patch Set 6:

Build Successful

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

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

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4141/ : SUCCESS

Change in vdsm[master]: vm: extend shared property to support locking

Ayal Baron has posted comments on this change.

Change subject: vm: extend shared property to support locking
......................................................................

Patch Set 6: Code-Review+2

Change in vdsm[master]: vm: extend shared property to support locking

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: extend shared property to support locking
......................................................................

Patch Set 7: Verified+1

Change in vdsm[master]: vm: extend shared property to support locking

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: extend shared property to support locking
......................................................................

Patch Set 7:

Build Successful

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

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

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4172/ : SUCCESS

Change in vdsm[master]: vm: extend shared property to support locking

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: extend shared property to support locking
......................................................................

Patch Set 8: Verified+1

No relevant changes.

Change in vdsm[master]: vm: extend shared property to support locking

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: extend shared property to support locking
......................................................................

Patch Set 8:

Build Successful

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

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

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4173/ : SUCCESS

Change in vdsm[master]: vm: extend shared property to support locking

[Sandro Bonazzola]

Sandro Bonazzola has posted comments on this change.

Change subject: vm: extend shared property to support locking
......................................................................

Patch Set 8:

(1 comment)

....................................................
File client/vdsClient.py
Line 1774:                     'machine',
Line 1775:                     'r   display=<vnc|qxl|qxlnc> : send the machine '
Line 1776:                     'display to vnc, spice, or spice with no '
Line 1777:                     'image compression',
Line 1778:                     'o   drive=pool:poolID,domain:domainID,image:imageID,'
I suggest to add a note also here about the shared parameter
Line 1779:                     'volume:volumeID[,boot:true,format:cow] : disk image '
Line 1780:                     'by UUIDs',
Line 1781:                     'o   (deprecated) hda/b/c/d=<path> : Disk drive '
Line 1782:                     'images',

Change in vdsm[master]: vm: extend shared property to support locking

Ayal Baron has posted comments on this change.

Change subject: vm: extend shared property to support locking
......................................................................

Patch Set 8: Code-Review+2

Change in vdsm[master]: vm: extend shared property to support locking

Ayal Baron has submitted this change and it was merged.

Change subject: vm: extend shared property to support locking
......................................................................

vm: extend shared property to support locking

The drives "shared" property has been extended (from True/False) to
support sanlock locking.

The new values are:

- "none":      no infomation (locking disabled)
 - "exclusive": the disk is used by the VM in exclusive mode
 - "shared":    the disk might be used by multiple VMs

For backward compatibility the old values are mapped as follows:

- True: "shared"
 - False: "none" (or "exclusive" if use_volume_leases is True)

Change-Id: I9429ead45caac1178957a33393642817db59508f
Signed-off-by: Federico Simoncelli 
Reviewed-on: http://gerrit.ovirt.org/17714
Reviewed-by: Ayal Baron 
---
M client/vdsClient.py
M tests/vmTests.py
M vdsm/storage/hsm.py
M vdsm/vm.py
M vdsm_api/vdsmapi-schema.json
5 files changed, 146 insertions(+), 53 deletions(-)

Approvals:
  Ayal Baron: Looks good to me, approved
  Federico Simoncelli: Verified