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 <laravot(a)redhat.com&gt;
---
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 {


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot(a)redhat.com&gt;

[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... :
SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[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... :
SUCCESS

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[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... :
SUCCESS

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[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,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[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": "..."
    }

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[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

-- 
To view, visit https://gerrit.ovirt.org/28493
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org

[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

-- 
To view, visit https://gerrit.ovirt.org/28493
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

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


Patch Set 3:

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

-- 
To view, visit https://gerrit.ovirt.org/28493
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No