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@redhat.com --- 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 {
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
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
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 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 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 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 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
automation@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
vdsm-patches@lists.fedorahosted.org