Change in vdsm[master]: VolumeMetadata: Add from_lines factory method

alitke at redhat.com alitke at redhat.com
Thu Apr 21 14:42:03 UTC 2016


Adam Litke has posted comments on this change.

Change subject: VolumeMetadata: Add from_lines factory method
......................................................................


Patch Set 4:

(12 comments)

https://gerrit.ovirt.org/#/c/52672/3//COMMIT_MSG
Commit Message:

Line 8: 
Line 9: Add support for creating a VolumeMetadata instance from a list of lines.
Line 10: This is the format that can be read from either block or file based
Line 11: volume metadata and enables us to consolidate the logic into one place.
Line 12: 
> Add comment that we now raise when integer values were expected in from_lin
Done
Line 13: Note the following semantics changes:
Line 14: 
Line 15:  - from_lines can raise ValueError if fields that represent int values
Line 16:    are not valid integers


https://gerrit.ovirt.org/#/c/52672/4/tests/storage_volume_metadata_test.py
File tests/storage_volume_metadata_test.py:

Line 107:         params = get_params()
Line 108:         params[param] = 'not_an_int'
Line 109:         self.assertRaises(AssertionError, volume.VolumeMetadata, **params)
Line 110: 
Line 111:     def test_from_lines_missing_param(self):
> Missing POOL will not raise, but this test does not check this. I think tha
Agreed.  Split the pool test case out and am now testing the omission of each required key as a separate permutation.
Line 112:         self.assertRaises(se.MetaDataKeyNotFoundError,
Line 113:                           volume.VolumeMetadata.from_lines, [])
Line 114: 
Line 115:     def test_from_lines_invalid_param(self):


Line 118:         lines.insert(0, "FOO=BAR")
Line 119:         self.assertNotIn("FOO",
Line 120:                          volume.VolumeMetadata.from_lines(lines).legacy_info())
Line 121: 
Line 122:     def test_from_lines_invalid_format(self):
> invalid_format is confusing, I was trying to understand whats wrong with "F
renamed and test all int params via permutations.
Line 123:         lines = ["DOMAIN=domain", "IMAGE=image", "PUUID=parent", "SIZE=FOO",
Line 124:                  "FORMAT=format", "TYPE=type", "VOLTYPE=voltype",
Line 125:                  "DISKTYPE=disktype", "EOF"]
Line 126:         self.assertRaises(ValueError,


Line 121: 
Line 122:     def test_from_lines_invalid_format(self):
Line 123:         lines = ["DOMAIN=domain", "IMAGE=image", "PUUID=parent", "SIZE=FOO",
Line 124:                  "FORMAT=format", "TYPE=type", "VOLTYPE=voltype",
Line 125:                  "DISKTYPE=disktype", "EOF"]
> This is very hard to read, please use always one item per line, sorted.
done.  Added a helper.
Line 126:         self.assertRaises(ValueError,
Line 127:                           volume.VolumeMetadata.from_lines, lines)
Line 128: 
Line 129:     def test_from_lines(self):


Line 129:     def test_from_lines(self):
Line 130:         lines = ["DOMAIN=domain", "IMAGE=image", "PUUID=parent", "SIZE=0",
Line 131:                  "FORMAT=format", "TYPE=type", "VOLTYPE=voltype",
Line 132:                  "DISKTYPE=disktype", "DESCRIPTION=description", "MTIME=0",
Line 133:                  "CTIME=0", "LEGALITY=legality", "EOF"]
> Same
Done
Line 134:         md = volume.VolumeMetadata.from_lines(lines)
Line 135:         self.assertEqual('domain', md.sd_id)
Line 136:         self.assertEqual('image', md.img_id)
Line 137:         self.assertEqual('parent', md.parent_vol_id)


https://gerrit.ovirt.org/#/c/52672/4/vdsm/storage/volume.py
File vdsm/storage/volume.py:

Line 199
Line 200
Line 201
Line 202
Line 203
> Lets use validate_description - this class is does not have any other legac
Done


Line 169:     TYPE_NETWORK = "network"
Line 170: 
Line 171: 
Line 172: class VolumeMetadata(object):
Line 173:     # metadata_key: (attribute, coercion_fn)
> This is not coercion but validation - it convert the string to the correct 
Done
Line 174:     MD_FIELDS = {
Line 175:         DOMAIN: ('sd_id', None),
Line 176:         IMAGE:  ('img_id', None),
Line 177:         PUUID:  ('parent_vol_id', None),


Line 209: 
Line 210:     @classmethod
Line 211:     def from_lines(cls, lines):
Line 212:         kwargs = {}
Line 213:         found_md = []
> We can use set() and name it found_keys
Done
Line 214:         for l in lines:
Line 215:             if l.startswith("EOF"):
Line 216:                 break
Line 217:             if l.find("=") < 0:


Line 210:     @classmethod
Line 211:     def from_lines(cls, lines):
Line 212:         kwargs = {}
Line 213:         found_md = []
Line 214:         for l in lines:
> Use "line" instead of "l".
Done
Line 215:             if l.startswith("EOF"):
Line 216:                 break
Line 217:             if l.find("=") < 0:
Line 218:                 continue


Line 213:         found_md = []
Line 214:         for l in lines:
Line 215:             if l.startswith("EOF"):
Line 216:                 break
Line 217:             if l.find("=") < 0:
> Better use:
Done
Line 218:                 continue
Line 219:             key, value = [item.strip() for item in l.split("=", 1)]
Line 220: 
Line 221:             if key == POOL:  # POOL is deprecated


Line 230:             found_md.append(key)
Line 231: 
Line 232:         missing = set(cls.MD_FIELDS.keys()) - set(found_md)
Line 233:         if missing:
Line 234:             raise se.MetaDataKeyNotFoundError("Missing: %s" % list(missing))
> Nice!
thanks :)
Line 235:         return cls(**kwargs)
Line 236: 
Line 237:     @property
Line 238:     def description(self):


Line 231: 
Line 232:         missing = set(cls.MD_FIELDS.keys()) - set(found_md)
Line 233:         if missing:
Line 234:             raise se.MetaDataKeyNotFoundError("Missing: %s" % list(missing))
Line 235:         return cls(**kwargs)
> I think we can simplify this a lot like this:
Nice!
Line 236: 
Line 237:     @property
Line 238:     def description(self):
Line 239:         return self._description


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7f63dce8e42e99d3240f4e916a9bd6ee5ee4b61
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list