Change in vdsm[master]: qemuimg: Handle new output format

Adam Litke has uploaded a new change for review.

Change subject: qemuimg: Handle new output format
......................................................................

qemuimg: Handle new output format

The current qemu-img info parsing logic assumes that if the output is
long enough, the backing image will appear on line 6.  In newer qemu
versions, some additional information has been added to the output.  In
the case of no backing file, the additional lines will fail to parse.
Handle this by trying to parse line 6 as a backing file and ignoring
regex parse errors for this line only.

Change-Id: Ic229198ab7c2bb9743bdf8629416131186115431
Signed-off-by: Adam Litke 
---
M lib/vdsm/qemuimg.py
1 file changed, 13 insertions(+), 3 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/52/27552/1

diff --git a/lib/vdsm/qemuimg.py b/lib/vdsm/qemuimg.py
index 69912d9..2dfbc4d 100644
--- a/lib/vdsm/qemuimg.py
+++ b/lib/vdsm/qemuimg.py
@@ -43,8 +43,15 @@
 }

+class _RegexSearchError(Exception):
+    pass
+
+
 def __iregexSearch(pattern, text):
-    return __iregex[pattern].search(text).group("value")
+    m = __iregex[pattern].search(text)
+    if m is None:
+        raise _RegexSearchError()
+    return m.group("value")

class QImgError(Exception):
@@ -81,8 +88,11 @@
             info['clustersize'] = int(__iregexSearch("clustersize", out[4]))

if len(out) > 5:
-            info['backingfile'] = __iregexSearch("backingfile", out[5])
-    except:
+            try:
+                info['backingfile'] = __iregexSearch("backingfile", out[5])
+            except _RegexSearchError:
+                pass
+    except _RegexSearchError:
         raise QImgError(rc, out, err, "unable to parse qemu-img info output")

return info

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 1:

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/8848/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/7926/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 2:

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9244/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8313/ : SUCCESS

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 2: Code-Review-1

Do we know what are the new info? Can you paste them here? Thanks.

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 3:

Federico: Updated commit message with sample output that demonstrates the problem.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8619/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9553/ : SUCCESS

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

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 3: Verified+1

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/27552/3/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 86:             'virtualsize': int(__iregexSearch("virtualsize", out[2])),
Line 87:         }
Line 88: 
Line 89:         if len(out) > 4:
Line 90:             info['clustersize'] = int(__iregexSearch("clustersize", out[4]))
Looking at the code it seems that also clustersize was optional. You may want to protect this too.
Line 91: 
Line 92:         if len(out) > 5:
Line 93:             try:
Line 94:                 info['backingfile'] = __iregexSearch("backingfile", out[5])

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/27552/3/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 86:             'virtualsize': int(__iregexSearch("virtualsize", out[2])),
Line 87:         }
Line 88: 
Line 89:         if len(out) > 4:
Line 90:             info['clustersize'] = int(__iregexSearch("clustersize", out[4]))
> Looking at the code it seems that also clustersize was optional. You may wa

Done
Line 91: 
Line 92:         if len(out) > 5:
Line 93:             try:
Line 94:                 info['backingfile'] = __iregexSearch("backingfile", out[5])

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 4: Verified+1

Added test cases

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 4:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8938/ : SUCCESS

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

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9877/ : SUCCESS

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/27552/4/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 89:         # Scan for optional fields in the output
Line 90:         row = 4
Line 91:         for field, filterFn in (('clustersize', int), ('backingfile', str)):
Line 92:             try:
Line 93:                 info[field] = filterFn(__iregexSearch(field, out[row]))
My problem with this approach are:

* I don't like having a counter (if we really need one we should try to use enumerate)
* I don't like catching such a common exception as IndexError with so many operations involved
Line 94:                 row = row + 1
Line 95:             except (_RegexSearchError, IndexError):
Line 96:                 pass
Line 97:     except _RegexSearchError:

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/27552/4/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 89:         # Scan for optional fields in the output
Line 90:         row = 4
Line 91:         for field, filterFn in (('clustersize', int), ('backingfile', str)):
Line 92:             try:
Line 93:                 info[field] = filterFn(__iregexSearch(field, out[row]))
> My problem with this approach are:

How about we just use multi-line regular expressions and search for 'backing file' where ever it might appear?  It seems like a generally fragile approach to require things to be on specific lines of the output anyway.
Line 94:                 row = row + 1
Line 95:             except (_RegexSearchError, IndexError):
Line 96:                 pass
Line 97:     except _RegexSearchError:

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/27552/4/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 89:         # Scan for optional fields in the output
Line 90:         row = 4
Line 91:         for field, filterFn in (('clustersize', int), ('backingfile', str)):
Line 92:             try:
Line 93:                 info[field] = filterFn(__iregexSearch(field, out[row]))
> How about we just use multi-line regular expressions and search for 'backin

Pardon a possibly dumb question: why don't we iterate output lines and catch the one we need with starts with()? I don't recall if we hand a reason.
Not that I mind long re, but the output seems to have natural line delimitation.
Line 94:                 row = row + 1
Line 95:             except (_RegexSearchError, IndexError):
Line 96:                 pass
Line 97:     except _RegexSearchError:

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 4: Code-Review-1

After speaking with Adam it seems that this is unrelated to live merge. So for now I am marking this to -1. Let's re-evaluate this after feature freeze.

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 4: -Verified

Removing verified and will revisit patch later

Itamar Heim has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 4:

ping

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 5:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11064/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12006/ : SUCCESS

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

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 6: Verified+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 6:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11084/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12026/ : SUCCESS

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

Francesco Romani has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 6: Code-Review+1

(5 comments)

seems OK and nicely tested.

http://gerrit.ovirt.org/#/c/27552/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 86:             'virtualsize': int(__iregexSearch("virtualsize", out[2])),
Line 87:         }
Line 88: 
Line 89:         # Scan for optional fields in the output
Line 90:         row = 4
Not a fan of magic numbers, but that was also in the old code - you actually removed one of them - so good enough for now. Can be cleaned further on a later patch.
Line 91:         for field, filterFn in (('clustersize', int), ('backingfile', str)):
Line 92:             try:
Line 93:                 info[field] = filterFn(__iregexSearch(field, out[row]))
Line 94:                 row = row + 1

Line 94:                 row = row + 1
Line 95:             except (_RegexSearchError, IndexError):
Line 96:                 pass
Line 97:     except _RegexSearchError:
Line 98:         raise QImgError(rc, out, err, "unable to parse qemu-img info output")
what about narrowing down this try/except clause by moving the except to line 88? In a later patch of course
Line 99: 
Line 100:     return info
Line 101: 
Line 102:

http://gerrit.ovirt.org/#/c/27552/6/tests/qemuimgTests.py
File tests/qemuimgTests.py:

Line 90: 
Line 91:     @monkeypatch.MonkeyPatch(utils, 'execCmd',
Line 92:                              partial(fakeCmd, outputQemu1NoBackingFile))
Line 93:     def testQemu1NoBackingFile(self):
Line 94:         info = qemuimg.info('foo')
I'd rather use 'unused' or something like it, but this is very minor, don't bother to change unless you need to resubmit.
Line 95:         self.assertFalse('backingfile' in info)
Line 96: 
Line 97:     @monkeypatch.MonkeyPatch(utils, 'execCmd',
Line 98:                              partial(fakeCmd, outputQemu1Backing))

Line 91:     @monkeypatch.MonkeyPatch(utils, 'execCmd',
Line 92:                              partial(fakeCmd, outputQemu1NoBackingFile))
Line 93:     def testQemu1NoBackingFile(self):
Line 94:         info = qemuimg.info('foo')
Line 95:         self.assertFalse('backingfile' in info)
assertNotIn was backported, we can make use of it.
Again, don't bother to change unless you need to resubmit.
Line 96: 
Line 97:     @monkeypatch.MonkeyPatch(utils, 'execCmd',
Line 98:                              partial(fakeCmd, outputQemu1Backing))
Line 99:     def testQemu1Backing(self):

Line 106:         info = qemuimg.info('foo')
Line 107:         self.assertEquals('qcow2', info['format'])
Line 108:         self.assertEquals(1073741824, info['virtualsize'])
Line 109:         self.assertEquals(65536, info['clustersize'])
Line 110:         self.assertFalse('backingfile' in info)
I believe it would be better to do like we do in vmTestsData.
The test data (for us, domain XML) is a template, and we have pairs of template data, config data. This way we do not duplicate the actual value to be checked.

But this is very minor so it will surely wait future patches.
Line 111: 
Line 112:     @monkeypatch.MonkeyPatch(utils, 'execCmd',
Line 113:                              partial(fakeCmd, outputQemu2BackingNoCluster))
Line 114:     def testQemu2BackingNoCluster(self):

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 6:

(2 comments)

http://gerrit.ovirt.org/#/c/27552/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 86:             'virtualsize': int(__iregexSearch("virtualsize", out[2])),
Line 87:         }
Line 88: 
Line 89:         # Scan for optional fields in the output
Line 90:         row = 4
> Not a fan of magic numbers, but that was also in the old code - you actuall

We probably need a constant here. E.g.:

row = INFO_OPTFIELDS_STARTIDX
Line 91:         for field, filterFn in (('clustersize', int), ('backingfile', str)):
Line 92:             try:
Line 93:                 info[field] = filterFn(__iregexSearch(field, out[row]))
Line 94:                 row = row + 1

Line 94:                 row = row + 1
Line 95:             except (_RegexSearchError, IndexError):
Line 96:                 pass
Line 97:     except _RegexSearchError:
Line 98:         raise QImgError(rc, out, err, "unable to parse qemu-img info output")
> what about narrowing down this try/except clause by moving the except to li

this seems easy enough to be fixed now, we just need to move the except block to line 88. Right?
Line 99: 
Line 100:     return info
Line 101: 
Line 102:

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 6: -Verified

(5 comments)

http://gerrit.ovirt.org/#/c/27552/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 86:             'virtualsize': int(__iregexSearch("virtualsize", out[2])),
Line 87:         }
Line 88: 
Line 89:         # Scan for optional fields in the output
Line 90:         row = 4
> We probably need a constant here. E.g.:

Done
Line 91:         for field, filterFn in (('clustersize', int), ('backingfile', str)):
Line 92:             try:
Line 93:                 info[field] = filterFn(__iregexSearch(field, out[row]))
Line 94:                 row = row + 1

Line 94:                 row = row + 1
Line 95:             except (_RegexSearchError, IndexError):
Line 96:                 pass
Line 97:     except _RegexSearchError:
Line 98:         raise QImgError(rc, out, err, "unable to parse qemu-img info output")
> this seems easy enough to be fixed now, we just need to move the except blo

Done
Line 99: 
Line 100:     return info
Line 101: 
Line 102:

http://gerrit.ovirt.org/#/c/27552/6/tests/qemuimgTests.py
File tests/qemuimgTests.py:

Line 90: 
Line 91:     @monkeypatch.MonkeyPatch(utils, 'execCmd',
Line 92:                              partial(fakeCmd, outputQemu1NoBackingFile))
Line 93:     def testQemu1NoBackingFile(self):
Line 94:         info = qemuimg.info('foo')
> I'd rather use 'unused' or something like it, but this is very minor, don't

Done
Line 95:         self.assertFalse('backingfile' in info)
Line 96: 
Line 97:     @monkeypatch.MonkeyPatch(utils, 'execCmd',
Line 98:                              partial(fakeCmd, outputQemu1Backing))

Line 91:     @monkeypatch.MonkeyPatch(utils, 'execCmd',
Line 92:                              partial(fakeCmd, outputQemu1NoBackingFile))
Line 93:     def testQemu1NoBackingFile(self):
Line 94:         info = qemuimg.info('foo')
Line 95:         self.assertFalse('backingfile' in info)
> assertNotIn was backported, we can make use of it.

Done
Line 96: 
Line 97:     @monkeypatch.MonkeyPatch(utils, 'execCmd',
Line 98:                              partial(fakeCmd, outputQemu1Backing))
Line 99:     def testQemu1Backing(self):

Line 106:         info = qemuimg.info('foo')
Line 107:         self.assertEquals('qcow2', info['format'])
Line 108:         self.assertEquals(1073741824, info['virtualsize'])
Line 109:         self.assertEquals(65536, info['clustersize'])
Line 110:         self.assertFalse('backingfile' in info)
> I believe it would be better to do like we do in vmTestsData.

ok, let's defer this one.
Line 111: 
Line 112:     @monkeypatch.MonkeyPatch(utils, 'execCmd',
Line 113:                              partial(fakeCmd, outputQemu2BackingNoCluster))
Line 114:     def testQemu2BackingNoCluster(self):

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 7: Verified+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 7:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11170/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12112/ : FAILURE

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

Francesco Romani has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 7: Code-Review+1

nice!

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 7: Code-Review+2

(1 comment)

http://gerrit.ovirt.org/#/c/27552/7/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 94:     row = INFO_OPTFIELDS_STARTIDX
Line 95:     for field, filterFn in (('clustersize', int), ('backingfile', str)):
Line 96:         try:
Line 97:             info[field] = filterFn(__iregexSearch(field, out[row]))
Line 98:             row = row + 1
Not a big deal but should this be outside of the try/except? (maybe in else?)
Line 99:         except (_RegexSearchError, IndexError):
Line 100:             pass
Line 101: 
Line 102:     return info

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 7:

(1 comment)

http://gerrit.ovirt.org/#/c/27552/7/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 94:     row = INFO_OPTFIELDS_STARTIDX
Line 95:     for field, filterFn in (('clustersize', int), ('backingfile', str)):
Line 96:         try:
Line 97:             info[field] = filterFn(__iregexSearch(field, out[row]))
Line 98:             row = row + 1
> Not a big deal but should this be outside of the try/except? (maybe in else

Done
Line 99:         except (_RegexSearchError, IndexError):
Line 100:             pass
Line 101: 
Line 102:     return info

Adam Litke has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 8: Verified+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 8:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11195/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12137/ : SUCCESS

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 9:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11208/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12150/ : FAILURE

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 9: Code-Review+2

Copying old score.

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: qemuimg: Handle new output format
......................................................................

qemuimg: Handle new output format

The current qemu-img info parsing logic assumes that if the output is
long enough, the backing image will appear on line 6.  In newer qemu
versions, some additional information has been added to the output.  In
the case of no backing file, the additional lines will fail to parse.
Handle this by trying to parse line 6 as a backing file and ignoring
regex parse errors for this line only.

Sample output from qemu-2.0.0-rc1:

[alitke@lager qemu-2.0.0-rc1]$ ./qemu-img info base.img
image: base.img
file format: qcow2
virtual size: 1.0G (1073741824 bytes)
disk size: 196K
cluster_size: 65536
Format specific information:
    compat: 1.1
    lazy refcounts: false
[alitke@lager qemu-2.0.0-rc1]$ ./qemu-img info leaf.img
image: leaf.img
file format: qcow2
virtual size: 1.0G (1073741824 bytes)
disk size: 196K
cluster_size: 65536
backing file: base.img
Format specific information:
    compat: 1.1
    lazy refcounts: false

Change-Id: Ic229198ab7c2bb9743bdf8629416131186115431
Signed-off-by: Adam Litke 
Reviewed-on: http://gerrit.ovirt.org/27552
Reviewed-by: Dan Kenigsberg 
---
M lib/vdsm/qemuimg.py
M tests/Makefile.am
A tests/qemuimgTests.py
3 files changed, 138 insertions(+), 8 deletions(-)

Approvals:
  Adam Litke: Verified
  Dan Kenigsberg: Looks good to me, approved

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................

Patch Set 10:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5764/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged_test_debug/129/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3923/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1788/ : FAILURE