Nir Soffer has posted comments on this change. Change subject: sdm: Move create_volume parent uuids into separate type ......................................................................
Patch Set 1: Code-Review-1 (9 comments) https://gerrit.ovirt.org/#/c/55830/1//COMMIT_MSG Commit Message: Line 8: Line 9: The schema defines the parent volume reference used in SDM.create_volume Line 10: as a distinct type (ParentVolumeInfo). Implement this in the code. Line 11: Parent volume parameters are not required but if provided, both must be Line 12: defined. Parent parameters should be required, parent is optional. Line 13: Line 14: Change-Id: I81aa1be73cb66ef6ca00d5b6a1997defd9aa8a48 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. 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. 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? Since both argument are required, empty dict should raise. 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? 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: parent_vol_id = BLANK_UUID if self.vol_info.parent is None else self.vol_info.parent.vol_id This is ugly but new code has no reason to verify the volume id, it can check if parent is None. 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 anyway, the fact that it is missing is a bug. 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. We can do: parent = params.get('parent') self.parent = ParentVolumeInfo(parent) if parent else None 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. We can replace this with: class ParentVolumeInfo(object): def __init__(self, params): self.img_id = _required(params, 'img_id') self.vol_id = _required(params, 'vol_id') 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 <ali...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
