Change in vdsm[master]: vm: improve extension check in extendDrivesIfNeeded

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: vm: improve extension check in extendDrivesIfNeeded
......................................................................

vm: improve extension check in extendDrivesIfNeeded

Change-Id: Ide5034cd5d87451c06a2ba034011fdf5696440e6
Signed-off-by: Federico Simoncelli 
---
M vdsm/vm.py
1 file changed, 23 insertions(+), 25 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/23/21423/1

diff --git a/vdsm/vm.py b/vdsm/vm.py
index e82867c..192f632 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -78,6 +78,9 @@
 CONSOLE_DEVICES = 'console'
 SMARTCARD_DEVICES = 'smartcard'

+# qcow2 format overhead is estimated to be 10% of capacity
+QCOW2_OVERHEAD = 0.1
+

def isVdsmImage(drive):
     return all(k in drive.keys() for k in ('volumeID', 'domainID', 'imageID',
@@ -2288,46 +2291,41 @@
             if not drive.blockDev or drive.format != 'cow':
                 continue

-            capacity, alloc, physical = self._dom.blockInfo(drive.path, 0)
+            capacity, curAlloc, physical = self._dom.blockInfo(drive.path, 0)

-            # Since the check based on nextPhysSize is extremly risky (it
-            # may result in the VM being paused) we can't use the regular
-            # getNextVolumeSize call as it relies on a cached value of the
-            # drive apparentsize.
-            nextPhysSize = physical + drive.VOLWM_CHUNK_MB * constants.MEGAB
+            if physical - curAlloc > drive.watermarkLimit:
+                continue  # Extension is not needed
+
+            # When an extension is needed we compute the maxium allowed size
+            # taking in account the qcow2 format overhead and rounding it up
+            # to the next volume chunk.
+            extChunkBytes = drive.VOLWM_CHUNK_MB * constants.MEGAB
+            totAlloc = capacity * (1.0 + QCOW2_OVERHEAD)
+            maxAlloc = (
+                int((totAlloc + extChunkBytes - 1) / extChunkBytes) *
+                extChunkBytes
+            )

# NOTE: the intent of this check is to prevent faulty images to
             # trick qemu in requesting extremely large extensions (BZ#998443).
-            # Probably the definitive check would be comparing the allocated
-            # space with capacity + format_overhead. Anyway given that:
-            #
-            # - format_overhead is tricky to be computed (it depends on few
-            #   assumptions that may change in the future e.g. cluster size)
-            # - currently we allow only to extend by one chunk at time
-            #
-            # the current check compares alloc with the next volume size.
-            # It should be noted that alloc cannot be directly compared with
-            # the volume physical size as it includes also the clusters not
-            # written yet (pending).
-            if alloc > nextPhysSize:
+            if curAlloc > maxAlloc:
                 self.log.error(
                     "Improbable extension request for volume %s on domain "
                     "%s, pausing the VM to avoid corruptions (capacity: %s, "
-                    "allocated: %s, physical: %s, next physical size: %s)",
-                    drive.volumeID, drive.domainID, capacity, alloc,
-                    physical, nextPhysSize)
+                    "allocated: %s, physical: %s, max allocation: %s)",
+                    drive.volumeID, drive.domainID, capacity, curAlloc,
+                    physical, maxAlloc)
                 self.pause(pauseCode='EOTHER')
                 return False

-            if physical - alloc < drive.watermarkLimit:
-                extend.append((drive, capacity, alloc, physical))
+            extend.append((drive, capacity, curAlloc, physical))

-        for drive, capacity, alloc, physical in extend:
+        for drive, capacity, curAlloc, physical in extend:
             self.log.info(
                 "Requesting extension for volume %s on domain %s (apparent: "
                 "%s, capacity: %s, allocated: %s, physical: %s)",
                 drive.volumeID, drive.domainID, drive.apparentsize, capacity,
-                alloc, physical)
+                curAlloc, physical)
             self.extendDriveVolume(drive)

return len(extend) > 0

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: improve extension check in extendDrivesIfNeeded
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm/vm.py
Line 2307:             )
Line 2308: 
Line 2309:             # NOTE: the intent of this check is to prevent faulty images to
Line 2310:             # trick qemu in requesting extremely large extensions (BZ#998443).
Line 2311:             if curAlloc > maxAlloc:
We're blocked on the fact that maxAlloc might be much higher if we have leaked clusters.
Line 2312:                 self.log.error(
Line 2313:                     "Improbable extension request for volume %s on domain "
Line 2314:                     "%s, pausing the VM to avoid corruptions (capacity: %s, "
Line 2315:                     "allocated: %s, physical: %s, max allocation: %s)",

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: improve extension check in extendDrivesIfNeeded
......................................................................

Patch Set 1:

Build Successful

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

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

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

Ayal Baron has posted comments on this change.

Change subject: vm: improve extension check in extendDrivesIfNeeded
......................................................................

Patch Set 1:

(3 comments)

....................................................
Commit Message
Line 3: AuthorDate: 2013-11-14 09:08:08 -0500
Line 4: Commit:     Federico Simoncelli 
Line 5: CommitDate: 2013-11-19 10:38:09 -0500
Line 6: 
Line 7: vm: improve extension check in extendDrivesIfNeeded
improve how? why?
Line 8: 
Line 9: Change-Id: Ide5034cd5d87451c06a2ba034011fdf5696440e6

....................................................
File vdsm/vm.py
Line 2299:             # When an extension is needed we compute the maxium allowed size
Line 2300:             # taking in account the qcow2 format overhead and rounding it up
Line 2301:             # to the next volume chunk.
Line 2302:             extChunkBytes = drive.VOLWM_CHUNK_MB * constants.MEGAB
Line 2303:             totAlloc = capacity * (1.0 + QCOW2_OVERHEAD)
Same result but possibly more legible?
overhead = capacity * QCOW2_OVERHEAD
totAlloc = capacity + overhead
(I think this makes the part in the comment about the overhead redundant)
Line 2304:             maxAlloc = (
Line 2305:                 int((totAlloc + extChunkBytes - 1) / extChunkBytes) *
Line 2306:                 extChunkBytes
Line 2307:             )

Line 2307:             )
Line 2308: 
Line 2309:             # NOTE: the intent of this check is to prevent faulty images to
Line 2310:             # trick qemu in requesting extremely large extensions (BZ#998443).
Line 2311:             if curAlloc > maxAlloc:
1. Meaning this patch cannot go in? so -1 its verification or something?
2. Assuming (1) is correct then we will only be able to accept this patch if we have a way to fix existing volumes (reclaim leaked clusters)
Line 2312:                 self.log.error(
Line 2313:                     "Improbable extension request for volume %s on domain "
Line 2314:                     "%s, pausing the VM to avoid corruptions (capacity: %s, "
Line 2315:                     "allocated: %s, physical: %s, max allocation: %s)",

Itamar Heim has posted comments on this change.

Change subject: vm: improve extension check in extendDrivesIfNeeded
......................................................................

Patch Set 1:

ping?

[Federico Simoncelli]

Federico Simoncelli has abandoned this change.

Change subject: vm: improve extension check in extendDrivesIfNeeded
......................................................................

Abandoned

Almost entirely replaced by http://gerrit.ovirt.org/21150