Change in vdsm[master]: vm: increase the volume extension on storage migration

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: vm: increase the volume extension on storage migration
......................................................................

vm: increase the volume extension on storage migration

During live migration VDSM needs to subsequently extend two volumes
instead of one; doubling the size of the chunk to extend the watermark
limit is doubled and VDSM has more time to accomplish the operations.

Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Signed-off-by: Federico Simoncelli 
---
M vdsm/libvirtvm.py
M vdsm/vm.py
2 files changed, 15 insertions(+), 8 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/00/9200/1

diff --git a/vdsm/libvirtvm.py b/vdsm/libvirtvm.py
index ca31a43..5f525bf 100644
--- a/vdsm/libvirtvm.py
+++ b/vdsm/libvirtvm.py
@@ -108,7 +108,7 @@
             capacity, alloc, physical = \
                 self._vm._dom.blockInfo(vmDrive.path, 0)

-            if physical - alloc >= self._vm._MIN_DISK_REMAIN:
+            if physical - alloc >= vmDrive.watermarkLimit:
                 continue

self._log.info('%s/%s apparent: %s capacity: %s, alloc: %s, '
@@ -1024,6 +1024,9 @@

class Drive(LibvirtVmDevice):
+    VOLWM_CHUNK_MB = config.getint('irs', 'volume_utilization_chunk_mb')
+    VOLWM_FREE_PCT = 100 - config.getint('irs', 'volume_utilization_percent')
+    VOLWM_CHUNK_LSM_MULT = 2  # Chunk multiplier during live storage migration

def __init__(self, conf, log, **kwargs):
         if not kwargs.get('serial'):
@@ -1041,6 +1044,16 @@
             self._blockDev = None

self._customize()
+
+    @property
+    def volExtensionChunk(self):
+        if hasattr(self, "diskReplicate"):
+            return self.VOLWM_CHUNK_MB * self.VOLWM_CHUNK_LSM_MULT
+        return self.VOLWM_CHUNK_MB
+
+    @property
+    def watermarkLimit(self):
+        return self.VOLWM_FREE_PCT * self.volExtensionChunk * (2 ** 20 / 100)

@property
     def blockDev(self):
@@ -1195,12 +1208,6 @@
         self._qemuguestSocketFile = (constants.P_LIBVIRT_VMCHANNELS +
                                      self.conf['vmName'].encode('utf-8') +
                                      '.' + _QEMU_GA_DEVICE_NAME)
-        # TODO find a better idea how to calculate this constant only after
-        # config is initialized
-        self._MIN_DISK_REMAIN = \
-            ((100 - config.getint('irs', 'volume_utilization_percent')) *
-             config.getint('irs', 'volume_utilization_chunk_mb') *
-             2 ** 20 / 100)
         self._lastXMLDesc = '<domain><uuid>%s</uuid></domain>' % self.id
         self._devXmlHash = '0'
         self._released = False
diff --git a/vdsm/vm.py b/vdsm/vm.py
index 7a421bf..8661911 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -800,7 +800,7 @@

if newSize is None:
             # newSize is always in megabytes
-            newSize = (config.getint('irs', 'volume_utilization_chunk_mb') +
+            newSize = (vmDrive.volExtensionChunk +
                        ((vmDrive.apparentsize + constants.MEGAB - 1) /
                         constants.MEGAB))

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/libvirtvm.py
Line 1025: 
Line 1026: class Drive(LibvirtVmDevice):
Line 1027:     VOLWM_CHUNK_MB = config.getint('irs', 'volume_utilization_chunk_mb')
Line 1028:     VOLWM_FREE_PCT = 100 - config.getint('irs', 'volume_utilization_percent')
Line 1029:     VOLWM_CHUNK_LSM_MULT = 2  # Chunk multiplier during live storage migration
We could expose this as configuration even though I think it's our responsibility to make this work without relying on custom configurations.
Line 1030: 
Line 1031:     def __init__(self, conf, log, **kwargs):
Line 1032:         if not kwargs.get('serial'):
Line 1033:             self.serial = kwargs.get('imageID'[-20:]) or ''

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli

Ayal Baron has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 1: (2 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 1025: 
Line 1026: class Drive(LibvirtVmDevice):
Line 1027:     VOLWM_CHUNK_MB = config.getint('irs', 'volume_utilization_chunk_mb')
Line 1028:     VOLWM_FREE_PCT = 100 - config.getint('irs', 'volume_utilization_percent')
Line 1029:     VOLWM_CHUNK_LSM_MULT = 2  # Chunk multiplier during live storage migration
We need to double the time as we're performing 2 lvextend operations.  User controls this by setting the first 2.
Line 1030: 
Line 1031:     def __init__(self, conf, log, **kwargs):
Line 1032:         if not kwargs.get('serial'):
Line 1033:             self.serial = kwargs.get('imageID'[-20:]) or ''

....................................................
File vdsm/vm.py
Line 799:             return
Line 800: 
Line 801:         if newSize is None:
Line 802:             # newSize is always in megabytes
Line 803:             newSize = (vmDrive.volExtensionChunk +
took me a while to understand what's bugging me here.
extendDriveVolume is called from 3 places:
diskReplicateStart
abnormalVmStop
highWrite

in diskReplicateStart we know it should be double
abnormalVmStop probably should be double too by the way as the regular flow didn't catch it in time
And in any event we perform all the checks in the calling methods anyway so my point is that the size should be always passed and not calculated here.
Line 804:                        ((vmDrive.apparentsize + constants.MEGAB - 1) /
Line 805:                         constants.MEGAB))
Line 806: 
Line 807:         if getattr(vmDrive, 'diskReplicate', None):

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/vm.py
Line 799:             return
Line 800: 
Line 801:         if newSize is None:
Line 802:             # newSize is always in megabytes
Line 803:             newSize = (vmDrive.volExtensionChunk +
Wouldn't this duplicate this code to at least three different places (diskReplicateStart, abnormalVmStop, highWrite)?

With the current code we have a doubled extension if diskReplicate is present (regardless where the call comes from). Why wouldn't you want a double extension in highWrite? (BTW I think that the call in abnormalVmStop is useless, it has also been proven by the fact that now sometimes we receive EOTHER and the extension works fine since it's handled by highWrite).

Actually my initial idea was to move this computation in the vmDrive object (eg: vmDrive.nextVolumeSize) to keep all the logic there (skipped because I didn't want to change too many things).
Line 804:                        ((vmDrive.apparentsize + constants.MEGAB - 1) /
Line 805:                         constants.MEGAB))
Line 806: 
Line 807:         if getattr(vmDrive, 'diskReplicate', None):

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli

Ayal Baron has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/vm.py
Line 799:             return
Line 800: 
Line 801:         if newSize is None:
Line 802:             # newSize is always in megabytes
Line 803:             newSize = (vmDrive.volExtensionChunk +
moving it to nextVolumeSize could be the way to go, but currently it seems wrong to compute next size in extend.
Line 804:                        ((vmDrive.apparentsize + constants.MEGAB - 1) /
Line 805:                         constants.MEGAB))
Line 806: 
Line 807:         if getattr(vmDrive, 'diskReplicate', None):

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/188/ (2/2)

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/154/ (1/2)

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 2:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/188/ : SUCCESS

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 2: (2 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 1064:         extension is in order (thin provisioning on block devices).
Line 1065:         """
Line 1066:         return self.VOLWM_FREE_PCT * self.volExtensionChunk * (2 ** 20 / 100)
Line 1067: 
Line 1068:     def getNextVolumeExtSize(self):
This could be a property too, even though since it is explicitly a calculation whose result is intended to change I picked a method.
Line 1069:         """
Line 1070:         Returns the next volume size in megabytes. This value is based on the
Line 1071:         volExtensionChunk property and it's the size that should be requested
Line 1072:         for the next LV extension.

....................................................
File vdsm/vm.py
Line 809:     def _rtcUpdate(self, timeOffset):
Line 810:         self.log.debug('new rtc offset %s', timeOffset)
Line 811:         self.conf['timeOffset'] = timeOffset
Line 812: 
Line 813:     def extendDriveVolume(self, vmDrive):
You probably (initially) asked the opposite but I figured out that if we make the newSize mandatory then we have 2 similar calls:

vmDrive.extendDriveVolume(vmDrive.getNextVolumeExtSize())

And we also might need to move the vmDrive.blockDev check into the getNextVolumeExtSize method (I didn't verify it but it might be problematic and we might need it there too). That said, if you still prefer to have the above format I'll go ahead and make the change.
Line 814:         if not vmDrive.blockDev:
Line 815:             return
Line 816: 
Line 817:         newSize = vmDrive.getNextVolumeExtSize()  # newSize is in megabytes

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 3: Verified

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 3:

Verified in 2 scenarios: 1) one VM running on 1 host (regular extension), 2) one VM running on 1 host live migrating the disk (double extension)

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(3 inline comments)

minor comments.

....................................................
File vdsm/libvirtvm.py
Line 1038:         return iface
Line 1039: 
Line 1040: 
Line 1041: class Drive(LibvirtVmDevice):
Line 1042:     VOLWM_CHUNK_MB = config.getint('irs', 'volume_utilization_chunk_mb')
what does VOLWM stand for?
Line 1043:     VOLWM_FREE_PCT = 100 - config.getint('irs', 'volume_utilization_percent')
Line 1044:     VOLWM_CHUNK_REPLICATE_MULT = 2  # Chunk multiplier during replication
Line 1045: 
Line 1046:     def __init__(self, conf, log, **kwargs):

Line 1078:         Returns the watermark limit, when the LV usage reaches this limit an
Line 1079:         extension is in order (thin provisioning on block devices).
Line 1080:         """
Line 1081:         return (self.VOLWM_FREE_PCT * self.volExtensionChunk *
Line 1082:                 (constants.MEGAB / 100))
The original calculation, avoiding the parenthesis, is slightly more exact. "100" divides the bigger number, not MEGAB. I see no reason to change that in this patch.
Line 1083: 
Line 1084:     def getNextVolumeExtSize(self):
Line 1085:         """
Line 1086:         Returns the next volume size in megabytes. This value is based on the

Line 1080:         """
Line 1081:         return (self.VOLWM_FREE_PCT * self.volExtensionChunk *
Line 1082:                 (constants.MEGAB / 100))
Line 1083: 
Line 1084:     def getNextVolumeExtSize(self):
the name confused me to think you are calculating the size of the next extension. actually it is is the next target size for the volume. I'd drop the Ext.
Line 1085:         """
Line 1086:         Returns the next volume size in megabytes. This value is based on the
Line 1087:         volExtensionChunk property and it's the size that should be requested
Line 1088:         for the next LV extension.

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 4: Looks good to me, approved

thanks for the quick fix.

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................

Patch Set 4: Verified

Verified in 3 scenarios: 1) one VM running on 1 host (regular extension), 2) one VM running on 1 host live migrating the disk (double extension) 3) one VM running on 1 host, after live migrating the disk (the extension is back to regular size)

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm: increase the volume extension on storage migration
......................................................................

vm: increase the volume extension on storage migration

During live migration VDSM needs to subsequently extend two volumes
instead of one; doubling the size of the chunk to extend the watermark
limit is doubled and VDSM has more time to accomplish the operations.

Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Signed-off-by: Federico Simoncelli 
---
M vdsm/libvirtvm.py
M vdsm/vm.py
2 files changed, 37 insertions(+), 13 deletions(-)

Approvals:
  Federico Simoncelli: Verified
  Dan Kenigsberg: Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/9200
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server