Change in vdsm[master]: sdm: Move create_volume parent uuids into separate type
alitke at redhat.com
alitke at redhat.com
Tue Apr 12 17:00:17 UTC 2016
Adam Litke has posted comments on this change.
Change subject: sdm: Move create_volume parent uuids into separate type
......................................................................
Patch Set 1:
(8 comments)
https://gerrit.ovirt.org/#/c/55830/1/tests/storage_sdm_create_volume_test.py
File tests/storage_sdm_create_volume_test.py:
Line 155:
Line 156: def test_default_parameter(self):
Line 157: info = _get_vol_info()
Line 158: del info['description']
Line 159: del info['initial_size']
> Why delete this? you don't have a test where you don't delete them.
This isn't related to the patch other than that I am not using loops anymore. We delete them so that we can verify that the type assigns the default values we expect.
Line 160:
Line 161: info_obj = storage.sdm.api.create_volume.CreateVolumeInfo(info)
Line 162: self.assertEqual('', info_obj.description)
Line 163: self.assertEqual(0, info_obj.initial_size)
Line 161: info_obj = storage.sdm.api.create_volume.CreateVolumeInfo(info)
Line 162: self.assertEqual('', info_obj.description)
Line 163: self.assertEqual(0, info_obj.initial_size)
Line 164: self.assertEqual(volume.BLANK_UUID, info_obj.parent.vol_id)
Line 165: self.assertEqual(volume.BLANK_UUID, info_obj.parent.img_id)
> info_obj.parent should be None in this case, meaning no parent.
Done
Line 166:
Line 167: def test_bad_enum_value(self):
Line 168: info = _get_vol_info()
Line 169: info['vol_format'] = 'foo'
Line 175:
Line 176: def test_no_params(self):
Line 177: obj = storage.sdm.api.create_volume.ParentVolumeInfo({})
Line 178: self.assertEqual(volume.BLANK_UUID, obj.vol_id)
Line 179: self.assertEqual(volume.BLANK_UUID, obj.img_id)
> We raise if a parameter is missing, but we are ok without any parameter?
Done
Line 180:
Line 181: def test_incomplete_params_raises(self):
Line 182: self.assertRaises(exception.MissingParameter,
Line 183: storage.sdm.api.create_volume.ParentVolumeInfo,
Line 189: def test_complete_params(self):
Line 190: params = {'vol_id': 'foo', 'img_id': 'bar'}
Line 191: obj = storage.sdm.api.create_volume.ParentVolumeInfo(params)
Line 192: for key, val in params.items():
Line 193: self.assertEqual(val, getattr(obj, key))
> We have too parameters, why do we need a loop?
Done
https://gerrit.ovirt.org/#/c/55830/1/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:
Line 40:
Line 41: def _run(self):
Line 42: vol_format = volume.name2type(self.vol_info.vol_format)
Line 43: self.sd_manifest.validateCreateVolumeParams(
Line 44: vol_format, self.vol_info.parent.vol_id)
> parent should be None if there is no parent. Better do:
Done
Line 45:
Line 46: with self.sd_manifest.domain_lock(self.host_id):
Line 47: image_res_ns = sd.getNamespace(self.sd_manifest.sdUUID,
Line 48: IMAGE_NAMESPACE)
Line 52: self.vol_info.img_id, self.vol_info.vol_id)
Line 53: artifacts.create(
Line 54: self.vol_info.virtual_size, vol_format,
Line 55: self.vol_info.disk_type, self.vol_info.description,
Line 56: self.vol_info.parent.vol_id)
> Lets pass the parent object instead of the volume id. We need the image id
Done
Line 57: artifacts.commit()
Line 58:
Line 59:
Line 60: # TODO: Adopt the properties framework for managing complex verb parameters
Line 70: for vt in (volume.RAW_FORMAT, volume.COW_FORMAT)]
Line 71: self.vol_format = _enum(params, 'vol_format', vol_types)
Line 72: self.disk_type = _enum(params, 'disk_type', image.DISK_TYPES.values())
Line 73: self.description = params.get('description', '')
Line 74: self.parent = ParentVolumeInfo(params.get('parent', {}))
> Should create object only if params['parent'] is not None.
Done
Line 75: self.initial_size = params.get('initial_size', 0)
Line 76:
Line 77:
Line 78: class ParentVolumeInfo(object):
Line 80: # The parameters are optional, but if specified both must be given
Line 81: if ('img_id' in params) != ('vol_id' in params):
Line 82: raise exception.MissingParameter()
Line 83: self.img_id = params.get('img_id', volume.BLANK_UUID)
Line 84: self.vol_id = params.get('vol_id', volume.BLANK_UUID)
> The parameters are not optional, the object is optional.
Done
Line 85:
Line 86:
Line 87: def _required(params, name):
Line 88: if name not in params:
--
To view, visit https://gerrit.ovirt.org/55830
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I81aa1be73cb66ef6ca00d5b6a1997defd9aa8a48
Gerrit-PatchSet: 1
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