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