On Fri, Jul 26, 2013 at 9:03 PM, Michele Tartara <[email protected]> wrote:
> On Fri, Jul 26, 2013 at 10:51 AM, Weiwei Jia <[email protected]> wrote:
>>
>> Add mount Gluster operation in CreateDisks.
>>
>> Signed-off-by: Weiwei Jia <[email protected]>
>> ---
>> lib/cmdlib/instance_storage.py | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/lib/cmdlib/instance_storage.py
>> b/lib/cmdlib/instance_storage.py
>> index 793cfa6..ff2e585 100644
>> --- a/lib/cmdlib/instance_storage.py
>> +++ b/lib/cmdlib/instance_storage.py
>> @@ -254,6 +254,21 @@ def CreateDisks(lu, instance, to_skip=None,
>> target_node_uuid=None, disks=None):
>> " node %s" % (file_storage_dir,
>> lu.cfg.GetNodeName(pnode_uuid)))
>>
>> + gluster_storage_dir = file_storage_dir
>
>
> Why is this taking the file_storage_dir as the gluster_storage dir? Wasn't
> it supposed to take the value passed at cluster init time through the
> "--gluster-file-storage-dir"?
Oops, we should get the "--gluster-file-storage-dir". I would update it.
>
>> + diskparams = lu.cfg.GetClusterInfo().diskparams
>> + for template, dt_params in diskparams.items():
>>
>> + if template == constants.DT_GLUSTER_FILE:
>
>
> Is it really needed to loop acroos all the items in diskparams to get the
> only one you are interested into?
This would update.
>
> Wouldn't something like this
>
> if constants.DT_GLUSTER_FILE in diskparams:
> dt_params = diskparams[constants.DT_GLUSTER_FILE]
> ...
>
> be better?
>
"if instance.disk_template in constants.DTS_FILEBASED" would be
better.
"if constants.DT_GLUSTER_FILE in diskparams" would be always true.
Please see the CreateDisk func in lib/cmdlib/instance_storage.py. My patch's
codes are after the if condition "if instance.disk_template in
constants.DTS_FILEBASED".
>
>> + gluster_volname = dt_params[constants.LDP_VOLNAME]
>> + gluster_hostname = dt_params[constants.LDP_HOSTNAME]
>> + result = lu.rpc.call_mount_gluster_storage_dir(pnode_uuid,
>> + gluster_hostname,
>> + gluster_volname,
>> + gluster_storage_dir)
>> +
>> + result.Raise("Failed to mount directory '%s' on"
>> + " node %s" % (gluster_storage_dir,
>> + lu.cfg.GetNodeName(pnode_uuid)))
>> +
>
>
> The entire block of code you added is executed unconditionally.
> This means that it would fail if the instance is not actually using gluster
> disks.
>
> It should be executed conditionally, only if gluster is actually being used.
>
Please see the location after merge my patch. In fact, they are after
"if instance.disk_template in constants.DTS_FILEBASED" condition.
Please see the CreateDisk func in lib/cmdlib/instance_storage.py.
--
Thanks
Weiwei Jia (Harry Wei)
