On Apr 17 17:01, Ilias Tsitsimpis wrote:
> 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?
I think I was able to take care of the merge.
Thanks,
Jose
> 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
--
Jose Antonio Lopes
Ganeti Engineering
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
Steuernummer: 48/725/00206
Umsatzsteueridentifikationsnummer: DE813741370
