On Thu, Apr 17, 2014 at 03:05PM, Jose A. Lopes wrote: > On Apr 17 15:14, Ilias Tsitsimpis wrote: > > On Thu, Apr 17, 2014 at 12:53PM, Jose A. Lopes wrote: > > > On Apr 16 15:19, Ilias Tsitsimpis wrote: > > > > Get all disk-related nodes for an instance. > > > > Also use 'GetInstanceSecondaryNodes' to get the > > > > list of secondary nodes. > > > > > > > > Signed-off-by: Ilias Tsitsimpis <[email protected]> > > > > --- > > > > lib/config.py | 58 > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 58 insertions(+) > > > > > > > > diff --git a/lib/config.py b/lib/config.py > > > > index b86df08..7147b50 100644 > > > > --- a/lib/config.py > > > > +++ b/lib/config.py > > > > @@ -350,6 +350,64 @@ class ConfigWriter(object): > > > > nodegroup = self._UnlockedGetNodeGroup(node.group) > > > > return self._UnlockedGetGroupDiskParams(nodegroup) > > > > > > > > + def _UnlockedGetInstanceNodes(self, inst_uuid): > > > > + """Get all disk-related nodes for an instance. > > > > + > > > > + For non-DRBD, this will be empty, for DRBD it will contain both > > > > + the primary and the secondaries. > > > > + > > > > + @type inst_uuid: string > > > > + @param inst_uuid: The UUID of the instance we want to get nodes for > > > > + @return: A list of names for all the nodes of the instance > > > > + > > > > + """ > > > > > > Missing @rtype in this method and others too. > > > > ACK. > > > > The @rtype is missing for the methods '_UnlockedGetInstanceNodes', > > '_UnlockedGetInstanceSecondaryNodes' and > > '_UnlockedGetInstanceLVsByNode'. I will send an interdiff for this patch > > and the next one (that contains the '_UnlockedGetInstanceLVsByNode' > > method). > > > > > > + instance = self._UnlockedGetInstanceInfo(inst_uuid) > > > > + if instance is None: > > > > + raise errors.ConfigurationError("Unknown instance '%s'" % > > > > inst_uuid) > > > > + > > > > + instance_disks = instance.disks > > > > + all_nodes = [] > > > > + for disk in instance_disks: > > > > + all_nodes.extend(disk.all_nodes) > > > > + all_nodes = set(all_nodes) > > > > + # ensure that primary node is always the first > > > > + all_nodes.discard(instance.primary_node) > > > > + return (instance.primary_node, ) + tuple(all_nodes) > > > > + > > > > + @_ConfigSync(shared=1) > > > > + def GetInstanceNodes(self, inst_uuid): > > > > + """Get all disk-related nodes for an instance. > > > > + > > > > + This is just a wrapper over L{_UnlockedGetInstanceNodes} > > > > + > > > > + """ > > > > + return self._UnlockedGetInstanceNodes(inst_uuid) > > > > + > > > > + def _UnlockedGetInstanceSecondaryNodes(self, inst_uuid): > > > > + """Get the list of secondary nodes. > > > > + > > > > + @type inst_uuid: string > > > > + @param inst_uuid: The UUID of the instance we want to get nodes for > > > > + @return: A list of names for all the secondary nodes of the > > > > instance > > > > + > > > > + """ > > > > + instance = self._UnlockedGetInstanceInfo(inst_uuid) > > > > + if instance is None: > > > > + raise errors.ConfigurationError("Unknown instance '%s'" % > > > > inst_uuid) > > > > + > > > > + all_nodes = set(self._UnlockedGetInstanceNodes(inst_uuid)) > > > > + all_nodes.discard(instance.primary_node) > > > > > > In this logic, we are basically adding the primary node twice and > > > removing it twice from the set. We can definitely improve this. > > > > This is actually the algorithm that is currently used for computing the > > secondary nodes ('_ComputeSecondaryNodes' method in Instance object). > > How do you suggest to improve this? > > A third method/function that is private and from which the list of > nodes actually comes from.
Interdiff:
diff --git a/lib/config.py b/lib/config.py
index 7147b50..f7f0620 100644
--- a/lib/config.py
+++ b/lib/config.py
@@ -350,15 +350,16 @@ class ConfigWriter(object):
nodegroup = self._UnlockedGetNodeGroup(node.group)
return self._UnlockedGetGroupDiskParams(nodegroup)
- def _UnlockedGetInstanceNodes(self, inst_uuid):
- """Get all disk-related nodes for an instance.
+ def _AllInstanceNodes(self, inst_uuid):
+ """Compute the set of all disk-related nodes for an instance.
- For non-DRBD, this will be empty, for DRBD it will contain both
- the primary and the secondaries.
+ This abstracts away some work from '_UnlockedGetInstanceNodes'
+ and '_UnlockedGetInstanceSecondaryNodes'.
@type inst_uuid: string
@param inst_uuid: The UUID of the instance we want to get nodes for
- @return: A list of names for all the nodes of the instance
+ @rtype: set of strings
+ @return: A set of names for all the nodes of the instance
"""
instance = self._UnlockedGetInstanceInfo(inst_uuid)
@@ -369,7 +370,21 @@ class ConfigWriter(object):
all_nodes = []
for disk in instance_disks:
all_nodes.extend(disk.all_nodes)
- all_nodes = set(all_nodes)
+ return set(all_nodes)
+
+ def _UnlockedGetInstanceNodes(self, inst_uuid):
+ """Get all disk-related nodes for an instance.
+
+ For non-DRBD, this will be empty, for DRBD it will contain both
+ the primary and the secondaries.
+
+ @type inst_uuid: string
+ @param inst_uuid: The UUID of the instance we want to get nodes for
+ @rtype: list of strings
+ @return: A list of names for all the nodes of the instance
+
+ """
+ all_nodes = self._AllInstanceNodes(inst_uuid)
# ensure that primary node is always the first
all_nodes.discard(instance.primary_node)
return (instance.primary_node, ) + tuple(all_nodes)
@@ -388,14 +403,11 @@ class ConfigWriter(object):
@type inst_uuid: string
@param inst_uuid: The UUID of the instance we want to get nodes for
+ @rtype: list of strings
@return: A list of names for all the secondary nodes of the instance
"""
- instance = self._UnlockedGetInstanceInfo(inst_uuid)
- if instance is None:
- raise errors.ConfigurationError("Unknown instance '%s'" % inst_uuid)
-
- all_nodes = set(self._UnlockedGetInstanceNodes(inst_uuid))
+ all_nodes = self._AllInstanceNodes(inst_uuid)
all_nodes.discard(instance.primary_node)
return tuple(all_nodes)
The above breaks some of the following patches, and most notably the
'Lift the Disk objects from the Instances' one. How would you like to
proceed?
Thanks for reviewing it so quickly,
Ilias
> Thanks,
> Jose
>
> > Thanks,
> > Ilias
> >
> > > Rest LGTM.
> > >
> > > Thanks,
> > > Jose
> > >
> > > > + return tuple(all_nodes)
> > > > +
> > > > + @_ConfigSync(shared=1)
> > > > + def GetInstanceSecondaryNodes(self, inst_uuid):
> > > > + """Get the list of secondary nodes.
> > > > +
> > > > + This is a simple wrapper over
> > > > L{_UnlockedGetInstanceSecondaryNodes}.
> > > > +
> > > > + """
> > > > + return self._UnlockedGetInstanceSecondaryNodes(inst_uuid)
> > > > +
> > > > @_ConfigSync(shared=1)
> > > > def GetGroupDiskParams(self, group):
> > > > """Get the disk params populated with inherit chain.
> > > > --
> > > > 1.9.1
signature.asc
Description: Digital signature
