On Mon, May 23, 2011 at 06:57:03PM +0200, Michael Hanselmann wrote:
> Until now LUNodeMigrate used multiple tasklets to evacuate all primary
> instances on a node. In some cases it would acquire all node locks,
> which isn't good on big clusters. With upcoming improvements to the LUs
> for instance failover and migration, switching to separate jobs looks
> like a better option. This patch changes LUNodeMigrate to use
> LU-generated jobs.
>
> While working on this patch, I identified a race condition in
> LUNodeMigrate.ExpandNames. A node's instances were retrieved without a
> lock and no verification was done.
>
> Commit aac4511a added CheckArguments to LUNodeMigrate with a call to
> _CheckIAllocatorOrNode. I haven't understood why this was done and can't
> see how it would've worked, at least on clusters with default settings.
>
> For RAPI, a new feature string is added and can be used to detect
> clusters which support more parameters for node migration. The client
> is updated.
With the interdiff, LGTM with a remark on this bit:
> @@ -1299,17 +1301,43 @@ class GanetiRapiClient(object): # pylint:
> disable-msg=R0904
> otherwise the hypervisor default will be used
> @type dry_run: bool
> @param dry_run: whether to perform a dry run
> + @type iallocator: string
> + @param iallocator: instance allocator to use
> + @type target_node: string
> + @param target_node: Target node for shared-storage instances
>
> @rtype: string
> @return: job id
>
> """
> query = []
> - if mode is not None:
> - query.append(("mode", mode))
> if dry_run:
> query.append(("dry-run", 1))
>
> + if _NODE_MIGRATE_REQV1 in self.GetFeatures():
> + body = {}
> +
> + if mode is not None:
> + body["mode"] = mode
> + if iallocator is not None:
> + body["iallocator"] = iallocator
> + if target_node is not None:
> + body["target_node"] = target_node
> +
> + assert len(query) <= 1
> +
> + return self._SendRequest(HTTP_POST,
> + ("/%s/nodes/%s/migrate" %
> + (GANETI_RAPI_VERSION, node)), query, body)
> +
> + # Use old request format
> + if target_node is not None:
> + raise GanetiApiError("Server does not support specifying target node"
> + " for node migration")
> +
> + if mode is not None:
> + query.append(("mode", mode))
> +
> return self._SendRequest(HTTP_POST,
> ("/%s/nodes/%s/migrate" %
> (GANETI_RAPI_VERSION, node)), query, None)
This with a return in the middle looks bad style. I'd appreciate either
returning just once at the end or the entire old request format in an
'else:' block.
thanks,
iustin
