Change in vdsm[master]: volume: support multi line metadata values

Liron Ar has uploaded a new change for review.

Change subject: volume: support multi line metadata values
......................................................................

volume: support multi line metadata values

Currently when creating the metadata dict it's assumed that each value
can be in a single line only while it's not necessarily true.

Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Signed-off-by: Liron Aravot 
---
M vdsm/storage/blockVolume.py
M vdsm/storage/volume.py
2 files changed, 12 insertions(+), 5 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/93/28493/1

diff --git a/vdsm/storage/blockVolume.py b/vdsm/storage/blockVolume.py
index d448e00..d7cf020 100644
--- a/vdsm/storage/blockVolume.py
+++ b/vdsm/storage/blockVolume.py
@@ -580,7 +580,6 @@
             self.log.error(e, exc_info=True)
             raise se.VolumeMetadataReadError("%s: %s" % (metaId, e))

-
     def setMetadata(self, meta, metaId=None):
         """
         Set the meta data hash as the new meta data of the Volume
diff --git a/vdsm/storage/volume.py b/vdsm/storage/volume.py
index aed932e..bfc1ff1 100644
--- a/vdsm/storage/volume.py
+++ b/vdsm/storage/volume.py
@@ -814,19 +814,27 @@
         """
         pass

-
     def metadata2dict(self, meta):
         out = {}
+        key = None
+        value = None
+
+        def put(key, value):
+            out[key.strip()] = "\n".join(value).strip()
+
         for l in meta:
             if l.startswith("EOF"):
-                return out
+                break
             if l.find("=") < 0:
+                value.append(l)
                 continue
+            if key:
+                put(key, value)
             key, value = l.split("=")
-            out[key.strip()] = value.strip()
+            value = [value]

+        put(key, value)
         return out
-

def metadata2info(self, meta):
         return {

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: volume: support multi line metadata values
......................................................................

Patch Set 1:

Build Successful

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

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

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

http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_ge... : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: volume: support multi line metadata values
......................................................................

Patch Set 2:

Build Failed

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

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

http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_ge... : SUCCESS

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: volume: support multi line metadata values
......................................................................

Patch Set 3:

Build Successful

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

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

http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_ge... : SUCCESS

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: volume: support multi line metadata values
......................................................................

Patch Set 3: Code-Review-1

(2 comments)

Too complex, no tests for very easy to test code.

http://gerrit.ovirt.org/#/c/28493/3/vdsm/storage/volume.py
File vdsm/storage/volume.py:

Line 823:             out[key.strip()] = "\n".join(value).strip()
Line 824: 
Line 825:         for l in meta:
Line 826:             if l.startswith("EOF"):
Line 827:                 break
This change is good but not required for this patch.
Line 828:             if l.find("=") < 0:
Line 829:                 value.append(l)
Line 830:                 continue
Line 831:             if key:

Line 833:             key, value = l.split("=")
Line 834:             value = [value]
Line 835: 
Line 836:         put(key, value)
Line 837:         return out
This is too complex.

You only need to change lines 823-825:

if l.find("=") < 0:
        if key is not None:
            out[key] += l # Continuation
    key, value = [s.strip() for s in l.split("=", 1)]
    out[key] = value

In any case - this code need tests, to make sure we did not break the old code, and that your new code actually works. While this is quite simple parser, it is too easy to make stupid errors in parsing. And it is trivial to write some tests for this. For example:

def test_empty(self):
        out = metadata2dict([])
        self.assertEquals({}, out}

def test_single_line(self):
        out = metadata2dict([
            'key 1 = value 1\n'
            'key 2 = value 2\n'
        ])
        self.assertEquals({'key 2': 'value 2', 'key 1': 'value 1'}, out}

With such easy to test code, there should be no excuse not to add tests.

Please add a patch with tests covering the old code behavior before this patch. Then it is trivial to see that your new code did not break the existing tests.
Line 838: 
Line 839:     def metadata2info(self, meta):
Line 840:         return {
Line 841:             "uuid": self.volUUID,

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: volume: support multi line metadata values
......................................................................

Patch Set 3:

Also, the need for storing multilines in the metadata file is not clear - the description field is meant to hold one line description, not short stories or application meta data.

Storing multiline json data in the metadata file does not sound like a good idea.

If we like to store additional metadata, we probbaly should use custom keys:

For example:

SIZE = 1                              
    FORMAT = COW
    OVF_DATE = ...
    OVF_SIZE = ...

And not:

SIZE = 1                              
    FORMAT = COW
    DESCRIPTION = {
        "ovf-date": "...",
        "ovf-size": "..."
    }

Yoav Kleinberger has posted comments on this change.

Change subject: volume: support multi line metadata values
......................................................................

Patch Set 3: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/28493/3/vdsm/storage/volume.py
File vdsm/storage/volume.py:

Line 833:             key, value = l.split("=")
Line 834:             value = [value]
Line 835: 
Line 836:         put(key, value)
Line 837:         return out
> This is too complex.

agree with Nir on this.
Line 838: 
Line 839:     def metadata2info(self, meta):
Line 840:         return {
Line 841:             "uuid": self.volUUID,

[Jenkins CI RO]

Jenkins CI RO has abandoned this change.

Change subject: volume: support multi line metadata values
......................................................................

Abandoned

Abandoned due to no activity - please restore if still relevant

[Jenkins CI RO]

Jenkins CI RO has posted comments on this change.

Change subject: volume: support multi line metadata values
......................................................................

Patch Set 3:

Abandoned due to no activity - please restore if still relevant

[e-mail] has posted comments on this change.

Change subject: volume: support multi line metadata values
......................................................................

Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found