On Sat, Jul 27, 2013 at 12:28 AM, Michele Tartara <[email protected]> wrote:
> On Fri, Jul 26, 2013 at 3:52 PM, harryxiyou <[email protected]> wrote:
>>
>> 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.
>
>
> Right, that would always be true. My fault.
>
> Even more so, then, there is no need for looping across the diskparams to
> get the data you need for dt_params. Just access them directly.
>
ACK. These would be accessed directly in my next version of patch set.
>>
>>
>> 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.
>
>
> I was looking at the patch only through the email and I didn't see the
> correct indentation.
> This way it's a bit better, but still it doesn't seem to me that it is
> correct.
> If you only check for "if instance.disk_template in constants.DTS_FILEBASED"
> it means that the call_mount_gluster_storage_dir() function will be called
> for every template that is file based. But it should be actually called only
> for gluster. So, you still need to specify that these instructions are to be
> executed only if the disk template is actually gluster.
>
ACK. This would be added in my next version of patch.
Thanks very much ;)
--
Thanks
Weiwei Jia (Harry Wei)
