This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1485/

This is great job! You seem to understand my (undocumented) Storage provider design ;-).

mof/LMI_Storage-BlockStorage.mof (Diff revision 1)
555
             "Size of the volume group." ),
558
             "Size of the thin pool. On input, only used when creating a ThinPool." ),
This change is wrong, 'Size' is still size of the volume group, there is nothing 'thin' in CreateOrModifyVG

mof/LMI_Storage-BlockStorage.mof (Diff revision 1)
582
       [IN, Description("On input: thin pool to modify.")]
583
       LMI_VGStoragePool REF InPool,
The description should be: the volume group from which the thin pool should be allocated.

mof/LMI_Storage-BlockStorage.mof (Diff revision 1)
590
       [IN, OUT, Description(
591
           "Size of the volume group. On input, only used when creating a ThinPool." ),
592
           Units ( "Bytes" ),
593
           PUnit ( "byte" )]
594
       uint64 Size,
I would emphasize that it's *physical* size of the thin pool, the pool will can store most Size bytes of data.

The *logical* size is unlimited.

mof/LMI_Storage-BlockStorage.mof (Diff revision 1)
615
        [IN, OUT, Description("Requested thin LV size. It will be rounded to multiples of VG's ExtentSize."
616
            "\n Modification is not supported."),
617
            Units("Bytes")]
618
        uint64 Size,
I would emphasize that it's *logical* size, it may be much higher than size we have in underlying storage.

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
431
            if param_elementname is None:
432
                raise pywbem.CIMError(pywbem.CIM_ERR_NOT_SUPPORTED,
433
                                      "ElementName is required.")
This is inconsistent with MOF file:

"If this parameter is not provided, implementation will choose on
it's own when creating the device."


src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
438
            pool = self.provider_manager.get_device_for_name(param_thinpool)
This may return None if there is no pool for the device, subsequent 'if' will throw ugly exception.

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
448
            pool = self.provider_manager.get_device_for_name(param_theelement)
449
            if pool.type != "lvmthinlv":
This may return None if there is no pool for the device, subsequent 'if' will throw ugly exception.

in addition, naming thin lv as 'pool' is confusing

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
492
        thinpool = self.storage.devicetree.getDeviceByPath(pool_path)
thinpool can be None if the device was destroyed when the Job was in queue.

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
502
        newsize = thinlv.size * units.MEGABYTE
there is now storage.from_blivet_size() to recalculate thinlv.size to bytes

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
529
            newsize = thinlv.vg.align(float(size) / units.MEGABYTE, True)
there is now storage.to_blivet_size to recalc size to blivet units

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
def cim_method_createormodifyelementfromstoragepool(self, env, object_name,
640
                                  "InPool and ElementType combination is not supported.")
I understand it as '(any) InPool and (any) ElementType must not be used together'

What about '_This_ InPool and ElementType combination is not supported'

And it might be probably CIM_ERR_INVALID_PARAMETER (please check similar error handling)

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
913
            raise pywbem.CIMError(pywbem.CIM_ERR_NOT_SUPPORTED,
914
                                  "Parameter Goal is not supported.")
pywbem.CIM_ERR_INVALID_PARAMETER?

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
938
            raise pywbem.CIMError(pywbem.CIM_ERR_NOT_SUPPORTED,
pywbem.CIM_ERR_INVALID_PARAMETER?

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
943
                raise pywbem.CIMError(pywbem.CIM_ERR_NOT_SUPPORTED,
pywbem.CIM_ERR_INVALID_PARAMETER?

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
946
                raise pywbem.CIMError(pywbem.CIM_ERR_NOT_SUPPORTED,
pywbem.CIM_ERR_INVALID_PARAMETER?

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
949
            pool = self.provider_manager.get_device_for_name(param_inpool)
pool can be None if the pool is not found

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
981
        vg = self.storage.devicetree.getDeviceByPath(pool_path)
vg can be 'None' if it disappeared when the job was in queue

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
985
                'size' : size / units.MEGABYTE}
use storage.to_blivet_size

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
991
        newsize = thinpool.size * units.MEGABYTE
use storage.from_blivet_size

src/lmi/storage/LMI_StorageConfigurationService.py (Diff revision 1)
def cim_method_createormodifystoragepool(self, env, object_name,
1107
            if pool.type == 'lvmvg':
1108
                if param_inextents:
1109
                    # modify vg
1110
                    self.cim_method_createormodifyvg(env, object_name,
1111
                                                     param_elementname, param_goal,
1112
                                                     param_inextents, param_pool,
1113
                                                     input_arguments, method_name)
1114
                else:
1115
                    # create thinpool
1116
                    self.cim_method_createormodifythinpool(env, object_name,
1117
                                                           param_elementname, param_goal,
1118
                                                           param_thinpool, None,
1119
                                                           param_size)
1120
            if pool.type == 'lvmthinpool':
1121
                if param_inextents:
1122
                    raise pywbem.CIMError(pywbem.CIM_ERR_NOT_SUPPORTED,
1123
                                          "InExtents specified together with Pool .")
1124
                else:
1125
                    # modify thinpool
1126
                    self.cim_method_createormodifythinpool(env, object_name,
1127
                                                           param_elementname, param_goal,
1128
                                                           None, param_thinpool,
1129
                                                           param_size)
I think that goal.ThinProvisionedPoolType is _the_ property that decides, what caller wants to create/modify.

If it is not specified -> default is used (create plain VG).

If it is specified -> check the other options (InExtents, InPool) and throw error if there is unsupported combination (e.g. InExtents with ThinProvisionedPoolType = ThinlyProvisionedLimitlessStoragePool)

src/lmi/storage/LMI_VGStoragePool.py (Diff revision 1)
def get_instance(self, env, model, device=None):
135
        # it's important to check for LVMThinPoolDevice first, because it is a subclass of LVMVolumeGroupDevice
136
        if isinstance(device, blivet.devices.LVMThinPoolDevice):
137
            model['TotalManagedSpace'] = pywbem.Uint64(device.size * units.MEGABYTE)
138
            # workaround for #1034721
139
            if device.freeSpace < 0:
140
                model['RemainingManagedSpace'] = pywbem.Uint64(0)
141
            else:
142
                model['RemainingManagedSpace'] = pywbem.Uint64(device.freeSpace * units.MEGABYTE)
143
            model['SpaceLimit'] = pywbem.Uint64(device.size * units.MEGABYTE)
144
            model['SpaceLimitDetermination'] = self.Values.SpaceLimitDetermination.Limitless
145
            model['ThinProvisionMetaDataSpace'] = pywbem.Uint64(device.metaDataSize * units.MEGABYTE)
146
        else:
147
            model['TotalManagedSpace'] = pywbem.Uint64(device.extents * device.peSize * units.MEGABYTE)
148
            model['RemainingManagedSpace'] = pywbem.Uint64(device.peFree * device.peSize * units.MEGABYTE)
149
            model['ExtentSize'] = pywbem.Uint64(device.peSize * units.MEGABYTE)
150
            model['TotalExtents'] = pywbem.Uint64(device.extents)
151
            model['RemainingExtents'] = pywbem.Uint64(device.peFree)
use storage.to_blivet_size and from_blivet_size

src/lmi/storage/LMI_VGStoragePool.py (Diff revision 1)
def _get_setting_for_device(self, device, setting_provider):
281
        setting['ExtentSize'] = device.peSize * units.MEGABYTE
292
293
        if isinstance(device, blivet.devices.LVMThinPoolDevice):
294
            setting['SpaceLimit'] = device.size * units.MEGABYTE
295
            setting['ThinProvisionedPoolType'] = self.Values.ThinProvisionedPoolType.ThinlyProvisionedLimitlessStoragePool
296
        else:
297
            setting['ExtentSize'] = device.peSize * units.MEGABYTE
282
        setting['ElementName'] = device.path
298
        setting['ElementName'] = device.path
299
use storage.to_blivet_size and from_blivet_size

I miss a XYZCapabilities instance for each (plain) VG, which would say that it's possible to allocated thin pools out of it. It can be either part of LVStorageCapabilities or separate instance of new class.

- Jan Safranek


On January 23rd, 2014, 2:25 p.m. CET, Jan Synacek wrote:

Review request for OpenLMI Developers.
By Jan Synacek.

Updated Jan. 23, 2014, 2:25 p.m.

Repository: openlmi-storage

Description

Add support for thin provisioning.

Currently, only creating / deleting thin pools and thin logical volumes
is supported.

Add some basic tests as well.

Diffs

  • mof/LMI_Storage-BlockStorage.mof (c92081c7539df2409649ad42770f8f9c5e396863)
  • mof/LMI_Storage-MethodParameters.mof (cd3cacff1ad2ff12e72d81f1406ca8ed03ccb95b)
  • src/lmi/storage/LMI_LVAllocatedFromStoragePool.py (98d05d28eb1ed377edd85e1a83fc5be03a872a5a)
  • src/lmi/storage/LMI_LVStorageCapabilities.py (95808296456289b3ec7cbfc0a5c81211a573acc6)
  • src/lmi/storage/LMI_LVStorageExtent.py (c4f258121396807f60fe9e269a9f1ad59fd20113)
  • src/lmi/storage/LMI_StorageConfigurationService.py (e0729cfd0c7ca573af8a8f174dafb13b57932b77)
  • src/lmi/storage/LMI_VGStorageCapabilities.py (966e397d1f961c7ebd6cdbf8fa5707a971b05c0a)
  • src/lmi/storage/LMI_VGStoragePool.py (6a88fdfbca502b0b687e5ec4d891e5b2dc74d0f6)
  • test/test_create_vg.py (e650b8123fe178d5a7957d6e8ec898111d63d177)

View Diff