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. > > 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. Thanks, Michele -- Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
