Re: [PATCH stable-2.15 16/17] Reduce nesting in import-export ProcessChildIO

['Brian Foley' via ganeti-devel]

On Tue, Dec 06, 2016 at 10:36:49PM +0100, Iustin Pop wrote:
> On 2016-12-06 17:30:24, Ganeti Development List wrote:
> > This avoids some pylint too-many-nested-blocks warnings. Do this by
> > flattening some 'arrow antipattern code' inside the polling loop to use
> > guards instead.
> 
> Was not sure what arrow antipattern is, but after squinting at the diff
> I'll remember it :)

Woo. I get to point you at Ward's Wiki (the first wiki ever):
http://wiki.c2.com/?ArrowAntiPattern

Don't stay too long, I find it almost as much of a time sink as tvtropes.org
(see also http://tvtropes.org/pmwiki/pmwiki.php/main/tvtropeswillruinyourlife )

Cheers,
Brian.

Re: [PATCH stable-2.15 01/17] Cleanup: Use new-style classes everywhere

[Iustin Pop]

On 2016-12-06 21:51:49, Ganeti Development List wrote:
> On Tue, Dec 06, 2016 at 09:34:39PM +0100, Iustin Pop wrote:
> > On 2016-12-06 17:29:56, Ganeti Development List wrote:
> > > This quells pylint's old-style-class warning. For all classes changed by
> > > this commit, none of the differences between new-style and old classes
> > > matter: we don't subclass any of these classes, or use super()/type().
> > 
> > I don't know anymore in Python 2.6/2.7 what the status is, but very old
> > Python version had the issue that the internals of the class were
> > different, e.g. __slots__ only works for new-style classes. Is that
> > still the case, or not?
> 
> It's still the case. __slots__ was introduced in 2.2 (along with new-style
> classes), and only works with new-style classes. Assigning to __class__
> breaks with Python <2.6 for classes with __slots__.
> 
> See https://docs.python.org/2/reference/datamodel.html#slots
> 
> However the classes in this patch are just used for simple encapsulation, and
> there's nothing tricky going on with them -- no subclassing, no metaclass 
> games,
> no overriding of any of the __dunder__ methods, so I think we're pretty safe.

LGTM, although honestly given Python, I'm not sure this is best committed
on 2.15. With all your other cleanups though, next 2.15 looks to be a
more significant "internal cleanup" release though, so fine :)

iustin

Re: [PATCH stable-2.15 15/17] Reduce nesting in LUOobCommand.Exec

['Brian Foley' via ganeti-devel]

On Tue, Dec 06, 2016 at 10:34:17PM +0100, Iustin Pop wrote:
> On 2016-12-06 17:30:22, Ganeti Development List wrote:
> > This avoids a pylint too-many-nested-blocks warning.
> > 
> > NB it's hard to see from the diff because of all the whitespace, but
> > this just turns the if result.fail_msg check into a guard that continues
> > the next iteration of the loop, and dedends the rest of the loop body
> > rather than having it as an else:
> 
> Actually this is almost readable, because of the empty lines that break
> the patch into multiple chunks.

You're better at this than me then! These kinds of diffs confuse the hell out
of me. :)

Cheers,
Brian.

Re: [PATCH stable-2.15 14/17] Reduce nesting in LUInstanceCreate.RunOsScripts

['Brian Foley' via ganeti-devel]

On Tue, Dec 06, 2016 at 10:32:48PM +0100, Iustin Pop wrote:
> On 2016-12-06 17:30:20, Ganeti Development List wrote:
> > This avoids a pylint too-many-nested-blocks warning.
> > 
> > NB it's hard to see from the diff because of all the whitespace, but
> > this just replaces the top "if iobj.disk and not self.adopt_disks" with
> > a pair of guards that return early, and dedents the rest of the fn.
> 
> The only question is why:
> 
> > +if not iobj.disks:
> > +  return
> > +
> > +if self.adopt_disks:
> > +  return
> 
> Instead of a single if? LGTM either way (didn't validate git -w though).

No particular reason, I just thought that they were distinct cases that I might
want to document separately (and then didn't get around to commenting because I
wasn't 100% sure why the checks were there).

Re: [PATCH stable-2.15 09/17] Reduce nesting in StartDaemon

['Brian Foley' via ganeti-devel]

On Tue, Dec 06, 2016 at 10:16:47PM +0100, Iustin Pop wrote:
> On 2016-12-06 17:30:11, Ganeti Development List wrote:
> > This avoids a pylint too-many-nested-blocks warning.
> > 
> > The extra try: finally: os._exit(1) is unnecessary as _StartDaemonChild
> > already catches all its exceptions and if it ever finishes, calls
> > os._exit(1) anyways.
> 
> LGTM, although this wording:
> 
> > +# Child process, can't return.
> 
> Seems to me a bit misleanding - I was confused why can't return, as exec
> can fail. I'd suggest "Try to start child process, will either execve or
> exit".
> 
> But nitpicking, LGTM either way.

Entirely misleading. Fixed now.

Thanks,
Brian.

Re: [PATCH stable-2.15 07/17] Disable pylint superfluous-parens warning

['Brian Foley' via ganeti-devel]

On Tue, Dec 06, 2016 at 10:12:09PM +0100, Iustin Pop wrote:
> On 2016-12-06 17:30:08, Ganeti Development List wrote:
> > There are too many cases where we deliberately wrap expressions in
> > parens, either to indicate comparisons, or to allow multiline
> > expressions without line continuation chars, or to clarify confusing
> > precedence.
> > 
> > While here, clean up a few unpythonic cases.
> 
> Nice cleanup, LGTM but one bug below:
> 
> > diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py
> > index 3eaf292..b32e62e 100644
> > --- a/lib/client/gnt_cluster.py
> > +++ b/lib/client/gnt_cluster.py
> > @@ -195,7 +195,7 @@ def InitCluster(opts, args):
> ># check the disk template types here, as we cannot rely on the type 
> > check done
> ># by the opcode parameter types
> >diskparams_keys = set(diskparams.keys())
> > -  if not (diskparams_keys <= constants.DISK_TEMPLATES):
> > +  if not diskparams_keys > constants.DISK_TEMPLATES:
> 
> You probably wanted to drop the 'not' but missed it.

D'oh! Indeed I did. Thankyou.

Re: [PATCH stable-2.15 01/17] Cleanup: Use new-style classes everywhere

['Brian Foley' via ganeti-devel]

On Tue, Dec 06, 2016 at 09:34:39PM +0100, Iustin Pop wrote:
> On 2016-12-06 17:29:56, Ganeti Development List wrote:
> > This quells pylint's old-style-class warning. For all classes changed by
> > this commit, none of the differences between new-style and old classes
> > matter: we don't subclass any of these classes, or use super()/type().
> 
> I don't know anymore in Python 2.6/2.7 what the status is, but very old
> Python version had the issue that the internals of the class were
> different, e.g. __slots__ only works for new-style classes. Is that
> still the case, or not?

It's still the case. __slots__ was introduced in 2.2 (along with new-style
classes), and only works with new-style classes. Assigning to __class__
breaks with Python <2.6 for classes with __slots__.

See https://docs.python.org/2/reference/datamodel.html#slots

However the classes in this patch are just used for simple encapsulation, and
there's nothing tricky going on with them -- no subclassing, no metaclass games,
no overriding of any of the __dunder__ methods, so I think we're pretty safe.

Cheers,
Brian.

Re: [PATCH stable-2.15 17/17] Disable pylint too-many-nested-blocks in _RunCmdPipe

[Iustin Pop]

On 2016-12-06 17:30:26, Ganeti Development List wrote:
> There doesn't appear to be any easy way of reducing the complexity
> of this function without moving it elsewhere or completely reorganising
> the function, so disable this warning for the time being.

LGTM.

Re: [PATCH stable-2.15 16/17] Reduce nesting in import-export ProcessChildIO

[Iustin Pop]

On 2016-12-06 17:30:24, Ganeti Development List wrote:
> This avoids some pylint too-many-nested-blocks warnings. Do this by
> flattening some 'arrow antipattern code' inside the polling loop to use
> guards instead.

Was not sure what arrow antipattern is, but after squinting at the diff
I'll remember it :)

LGTM.

Re: [PATCH stable-2.15 15/17] Reduce nesting in LUOobCommand.Exec

[Iustin Pop]

On 2016-12-06 17:30:22, Ganeti Development List wrote:
> This avoids a pylint too-many-nested-blocks warning.
> 
> NB it's hard to see from the diff because of all the whitespace, but
> this just turns the if result.fail_msg check into a guard that continues
> the next iteration of the loop, and dedends the rest of the loop body
> rather than having it as an else:

Actually this is almost readable, because of the empty lines that break
the patch into multiple chunks.

LGTM.

Re: [PATCH stable-2.15 14/17] Reduce nesting in LUInstanceCreate.RunOsScripts

[Iustin Pop]

On 2016-12-06 17:30:20, Ganeti Development List wrote:
> This avoids a pylint too-many-nested-blocks warning.
> 
> NB it's hard to see from the diff because of all the whitespace, but
> this just replaces the top "if iobj.disk and not self.adopt_disks" with
> a pair of guards that return early, and dedents the rest of the fn.

The only question is why:

> +if not iobj.disks:
> +  return
> +
> +if self.adopt_disks:
> +  return

Instead of a single if? LGTM either way (didn't validate git -w though).

iustin

Re: [PATCH stable-2.15 13/17] Reduce nesting in RemoveNodeSshKeyBulk key calculation

[Iustin Pop]

On 2016-12-06 17:30:18, Ganeti Development List wrote:
> This avoids a pylint too-many-nested-blocks warning. This removes the
> 6th level of nesting in the function, but may also be marginally faster
> by turning the calculation into the set difference operation it really
> is.
> 
> No functional change.

I had to read the code to confirm this, as this replaces
keys[node_info.uuid] rather than updating it. And while reading I was
wondering why this isn't a set in the first place, but that's another
issue.

LGTM

Re: [PATCH stable-2.15 12/17] Reduce nesting in RemoveNodeSshKeyBulk ssh logic

[Iustin Pop]

On 2016-12-06 17:30:17, Ganeti Development List wrote:
> This avoids a pylint too-many-nested-blocks warning. It also removes
> some copy & paste code, showing that the master candidate and ordinary
> node case are the same apart from the logging.

As common with cleanup, one has to wonder how come it got to that much
duplication in the first place :)

LGTM, thanks.

Re: [PATCH stable-2.15 11/17] Reduce nesting in gnt-cluster VerifyDisks missing disk loop

[Iustin Pop]

On 2016-12-06 17:30:15, Ganeti Development List wrote:
> To avoid pylint's too-many-nested-blocks warning.

Also improves readability a bit, LGTM.

Re: [PATCH stable-2.15 10/17] Reduce nesting in _CheckVLANArguments

[Iustin Pop]

On 2016-12-06 17:30:13, Ganeti Development List wrote:
> This avoids a pylint too-many-nested-blocks warning. It also has the
> happy side effect of removing some duplicated code.
> 
> No functional change.

Nice cleanup, LGTM.

Re: [PATCH stable-2.15 09/17] Reduce nesting in StartDaemon

[Iustin Pop]

On 2016-12-06 17:30:11, Ganeti Development List wrote:
> This avoids a pylint too-many-nested-blocks warning.
> 
> The extra try: finally: os._exit(1) is unnecessary as _StartDaemonChild
> already catches all its exceptions and if it ever finishes, calls
> os._exit(1) anyways.

LGTM, although this wording:

> +# Child process, can't return.

Seems to me a bit misleanding - I was confused why can't return, as exec
can fail. I'd suggest "Try to start child process, will either execve or
exit".

But nitpicking, LGTM either way.

iustin

Re: [PATCH stable-2.15 08/17] Disable pylint bad-continuation warning

[Iustin Pop]

On 2016-12-06 17:30:09, Ganeti Development List wrote:
> pylint is much more strict than pep8, and it would be too invasive
> (and arguably pointless) to update these right now.

LGTM.

Re: [PATCH stable-2.15 07/17] Disable pylint superfluous-parens warning

[Iustin Pop]

On 2016-12-06 17:30:08, Ganeti Development List wrote:
> There are too many cases where we deliberately wrap expressions in
> parens, either to indicate comparisons, or to allow multiline
> expressions without line continuation chars, or to clarify confusing
> precedence.
> 
> While here, clean up a few unpythonic cases.

Nice cleanup, LGTM but one bug below:

> diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py
> index 3eaf292..b32e62e 100644
> --- a/lib/client/gnt_cluster.py
> +++ b/lib/client/gnt_cluster.py
> @@ -195,7 +195,7 @@ def InitCluster(opts, args):
># check the disk template types here, as we cannot rely on the type check 
> done
># by the opcode parameter types
>diskparams_keys = set(diskparams.keys())
> -  if not (diskparams_keys <= constants.DISK_TEMPLATES):
> +  if not diskparams_keys > constants.DISK_TEMPLATES:

You probably wanted to drop the 'not' but missed it.

iustin

Re: [PATCH stable-2.15 03/17] Disable incorrect pylint assigning-non-slot warning

[Iustin Pop]

On 2016-12-06 17:30:00, Ganeti Development List wrote:
> This occurs pretty heavily in lib/objects.py, where pylint isn't
> correctly detecting the __slots__ assignment. This appears to be
> a known issue: https://github.com/PyCQA/pylint/issues/379

LGTM.

Re: [PATCH stable-2.15 06/17] Disable pylint redefined-variable-type warning

[Iustin Pop]

On 2016-12-06 17:30:06, Ganeti Development List wrote:
> There are a large number of cases where Ganeti assigns multiple types
> (eg set/list, float/int) to the same variable at different times, and
> although these would make a type checking tool very unhappy, they are
> benign here (and besides, cleaning all this up would be too invasive).

Sadly, true. LGTM.

Re: [PATCH stable-2.15 05/17] Disable pylint too-many-branches warnings

[Iustin Pop]

On 2016-12-06 17:30:04, Ganeti Development List wrote:
> This is useful, but in some cases is a little too conservative. A fn
> can have a lot of branches, but very little nesting, and can still be
> easy to understand. This occurs in, eg, XenPvmHypervisor._GetConfig.

LGTM.

Re: [PATCH stable-2.15 04/17] Disable pylint broad-except warnings

[Iustin Pop]

On 2016-12-06 17:30:02, Ganeti Development List wrote:
> These are all deliberate top-level catch-any-unhandled-exception cases,
> used for logging and error handling so just get pylint to ignore them.

LGTM.

Re: [PATCH stable-2.15 02/17] Quell pylint unbalanced-tuple-unpacking warning

[Iustin Pop]

On 2016-12-06 17:29:58, Ganeti Development List wrote:
> Both of these functions return a list, not a tuple, and by manually
> tracing the code, we can see they always return non-empty lists.
> 
> Change the callers to get the first element of the list rather than
> using destructuring.

Someday this will break at runtime, but that was true before as well, so
LGTM.

iustin

Re: [PATCH stable-2.15 01/17] Cleanup: Use new-style classes everywhere

[Iustin Pop]

On 2016-12-06 17:29:56, Ganeti Development List wrote:
> This quells pylint's old-style-class warning. For all classes changed by
> this commit, none of the differences between new-style and old classes
> matter: we don't subclass any of these classes, or use super()/type().

I don't know anymore in Python 2.6/2.7 what the status is, but very old
Python version had the issue that the internals of the class were
different, e.g. __slots__ only works for new-style classes. Is that
still the case, or not?

iustin

[PATCH stable-2.15 09/17] Reduce nesting in StartDaemon

['Brian Foley' via ganeti-devel]

This avoids a pylint too-many-nested-blocks warning.

The extra try: finally: os._exit(1) is unnecessary as _StartDaemonChild
already catches all its exceptions and if it ever finishes, calls
os._exit(1) anyways.

Signed-off-by: Brian Foley 
---
 lib/utils/process.py | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/lib/utils/process.py b/lib/utils/process.py
index 9183e9f..bb5aed2 100644
--- a/lib/utils/process.py
+++ b/lib/utils/process.py
@@ -360,15 +360,11 @@ def StartDaemon(cmd, env=None, cwd="/", output=None, 
output_fd=None,
   # First fork
   pid = os.fork()
   if pid == 0:
-try:
-  # Child process, won't return
-  _StartDaemonChild(errpipe_read, errpipe_write,
-pidpipe_read, pidpipe_write,
-cmd, cmd_env, cwd,
-output, output_fd, pidfile)
-finally:
-  # Well, maybe child process failed
-  os._exit(1) # pylint: disable=W0212
+# Child process, can't return.
+_StartDaemonChild(errpipe_read, errpipe_write,
+  pidpipe_read, pidpipe_write,
+  cmd, cmd_env, cwd,
+  output, output_fd, pidfile)
 finally:
   utils_wrapper.CloseFdNoError(errpipe_write)
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 12/17] Reduce nesting in RemoveNodeSshKeyBulk ssh logic

['Brian Foley' via ganeti-devel]

This avoids a pylint too-many-nested-blocks warning. It also removes
some copy & paste code, showing that the master candidate and ordinary
node case are the same apart from the logging.

No funtional change.

Signed-off-by: Brian Foley 
---
 lib/backend.py | 27 +++
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/lib/backend.py b/lib/backend.py
index 634f2e5..37ce31a 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -1864,9 +1864,13 @@ def RemoveNodeSshKeyBulk(node_list,
 error_msg_final = ("When removing the key of node '%s', updating the"
" SSH key files of node '%s' failed. Last error"
" was: %s.")
-if node in potential_master_candidates:
-  logging.debug("Updating key setup of potential master candidate node"
-" %s.", node)
+
+if node in potential_master_candidates or from_authorized_keys:
+  if node in potential_master_candidates:
+node_desc = "potential master candidate"
+  else:
+node_desc = "normal"
+  logging.debug("Updating key setup of %s node %s.", node_desc, node)
   try:
 utils.RetryByNumberOfTimes(
 constants.SSHS_MAX_RETRIES,
@@ -1881,23 +1885,6 @@ def RemoveNodeSshKeyBulk(node_list,
 result_msgs.append((node, error_msg))
 logging.error(error_msg)
 
-else:
-  if from_authorized_keys:
-logging.debug("Updating key setup of normal node %s.", node)
-try:
-  utils.RetryByNumberOfTimes(
-  constants.SSHS_MAX_RETRIES,
-  errors.SshUpdateError,
-  run_cmd_fn, cluster_name, node, pathutils.SSH_UPDATE,
-  ssh_port, base_data,
-  debug=False, verbose=False, use_cluster_key=False,
-  ask_key=False, strict_host_check=False)
-except errors.SshUpdateError as last_exception:
-  error_msg = error_msg_final % (
-  node_info.name, node, last_exception)
-  result_msgs.append((node, error_msg))
-  logging.error(error_msg)
-
   for node_info in node_list:
 if node_info.clear_authorized_keys or node_info.from_public_keys or \
 node_info.clear_public_keys:
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 05/17] Disable pylint too-many-branches warnings

['Brian Foley' via ganeti-devel]

This is useful, but in some cases is a little too conservative. A fn
can have a lot of branches, but very little nesting, and can still be
easy to understand. This occurs in, eg, XenPvmHypervisor._GetConfig.

Ganeti has many functions that fail this check, so disable it for now
until we get a chance to clean up the codebase.

Signed-off-by: Brian Foley 
---
 Makefile.am | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index 108cbba..ba99fb8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2686,7 +2686,9 @@ PEP8_EXCLUDE = $(subst $(space),$(comma),$(strip $(notdir 
$(built_python_sources
 
 # A space-separated list of pylint warnings to completely ignore:
 # I0013 = disable warnings for ignoring whole files
-LINT_DISABLE = I0013
+# R0912 = disable too many branches warning. It's useful, but ganeti requires
+# a lot of refactoring to fix this.
+LINT_DISABLE = I0013 R0912
 # Additional pylint options
 LINT_OPTS =
 # The combined set of pylint options
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 01/17] Cleanup: Use new-style classes everywhere

['Brian Foley' via ganeti-devel]

This quells pylint's old-style-class warning. For all classes changed by
this commit, none of the differences between new-style and old classes
matter: we don't subclass any of these classes, or use super()/type().

Signed-off-by: Brian Foley 
---
 lib/cmdlib/instance_storage.py | 2 +-
 lib/jqueue/__init__.py | 6 +++---
 lib/rpc/node.py| 6 +++---
 lib/rpc/transport.py   | 4 ++--
 lib/ssh.py | 2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/cmdlib/instance_storage.py b/lib/cmdlib/instance_storage.py
index 0093edc..af1c2e8 100644
--- a/lib/cmdlib/instance_storage.py
+++ b/lib/cmdlib/instance_storage.py
@@ -3014,7 +3014,7 @@ class TLReplaceDisks(Tasklet):
   self._RemoveOldStorage(self.target_node_uuid, iv_names)
 
 
-class TemporaryDisk():
+class TemporaryDisk(object):
   """ Creates a new temporary bootable disk, and makes sure it is destroyed.
 
   Is a context manager, and should be used with the ``with`` statement as such.
diff --git a/lib/jqueue/__init__.py b/lib/jqueue/__init__.py
index 1aec8ae..d996461 100644
--- a/lib/jqueue/__init__.py
+++ b/lib/jqueue/__init__.py
@@ -704,7 +704,7 @@ def _EncodeOpError(err):
   return errors.EncodeException(to_encode)
 
 
-class _TimeoutStrategyWrapper:
+class _TimeoutStrategyWrapper(object):
   def __init__(self, fn):
 """Initializes this class.
 
@@ -736,7 +736,7 @@ class _TimeoutStrategyWrapper:
 return result
 
 
-class _OpExecContext:
+class _OpExecContext(object):
   def __init__(self, op, index, log_prefix, timeout_strategy_factory):
 """Initializes this class.
 
@@ -1279,7 +1279,7 @@ class _JobQueueWorkerPool(workerpool.WorkerPool):
 self.queue = queue
 
 
-class _JobDependencyManager:
+class _JobDependencyManager(object):
   """Keeps track of job dependencies.
 
   """
diff --git a/lib/rpc/node.py b/lib/rpc/node.py
index e65d998..cd2050d 100644
--- a/lib/rpc/node.py
+++ b/lib/rpc/node.py
@@ -305,7 +305,7 @@ def _SsconfResolver(ssconf_ips, node_list, _,
   return result
 
 
-class _StaticResolver:
+class _StaticResolver(object):
   def __init__(self, addresses):
 """Initializes this class.
 
@@ -363,7 +363,7 @@ def _NodeConfigResolver(single_node_fn, all_nodes_fn, 
node_uuids, opts):
 for uuid in node_uuids]
 
 
-class _RpcProcessor:
+class _RpcProcessor(object):
   def __init__(self, resolver, port, lock_monitor_cb=None):
 """Initializes this class.
 
@@ -472,7 +472,7 @@ class _RpcProcessor:
 return self._CombineResults(results, requests, procedure)
 
 
-class _RpcClientBase:
+class _RpcClientBase(object):
   def __init__(self, resolver, encoder_fn, lock_monitor_cb=None,
_req_process_fn=None):
 """Initializes this class.
diff --git a/lib/rpc/transport.py b/lib/rpc/transport.py
index 8271016..ee5516d 100644
--- a/lib/rpc/transport.py
+++ b/lib/rpc/transport.py
@@ -52,7 +52,7 @@ DEF_CTMO = constants.LUXI_DEF_CTMO
 DEF_RWTO = constants.LUXI_DEF_RWTO
 
 
-class Transport:
+class Transport(object):
   """Low-level transport class.
 
   This is used on the client side.
@@ -243,7 +243,7 @@ class Transport:
   self.socket = None
 
 
-class FdTransport:
+class FdTransport(object):
   """Low-level transport class that works on arbitrary file descriptors.
 
   Unlike L{Transport}, this doesn't use timeouts.
diff --git a/lib/ssh.py b/lib/ssh.py
index 6e16674..9ce4e1b 100644
--- a/lib/ssh.py
+++ b/lib/ssh.py
@@ -721,7 +721,7 @@ def InitPubKeyFile(master_uuid, 
key_file=pathutils.SSH_PUB_KEYS):
   AddPublicKey(master_uuid, key, key_file=key_file)
 
 
-class SshRunner:
+class SshRunner(object):
   """Wrapper for SSH commands.
 
   """
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 15/17] Reduce nesting in LUOobCommand.Exec

['Brian Foley' via ganeti-devel]

This avoids a pylint too-many-nested-blocks warning.

NB it's hard to see from the diff because of all the whitespace, but
this just turns the if result.fail_msg check into a guard that continues
the next iteration of the loop, and dedends the rest of the loop body
rather than having it as an else:

git diff -w will show this.

No functional change.

Signed-off-by: Brian Foley 
---
 lib/cmdlib/misc.py | 65 +++---
 1 file changed, 33 insertions(+), 32 deletions(-)

diff --git a/lib/cmdlib/misc.py b/lib/cmdlib/misc.py
index 62bff52..68ba70f 100644
--- a/lib/cmdlib/misc.py
+++ b/lib/cmdlib/misc.py
@@ -147,43 +147,44 @@ class LUOobCommand(NoHooksLU):
 self.LogWarning("Out-of-band RPC failed on node '%s': %s",
 node.name, result.fail_msg)
 node_entry.append((constants.RS_NODATA, None))
+continue
+
+  try:
+self._CheckPayload(result)
+  except errors.OpExecError, err:
+self.LogWarning("Payload returned by node '%s' is not valid: %s",
+node.name, err)
+node_entry.append((constants.RS_NODATA, None))
   else:
-try:
-  self._CheckPayload(result)
-except errors.OpExecError, err:
-  self.LogWarning("Payload returned by node '%s' is not valid: %s",
-  node.name, err)
-  node_entry.append((constants.RS_NODATA, None))
-else:
-  if self.op.command == constants.OOB_HEALTH:
-# For health we should log important events
-for item, status in result.payload:
-  if status in [constants.OOB_STATUS_WARNING,
-constants.OOB_STATUS_CRITICAL]:
-self.LogWarning("Item '%s' on node '%s' has status '%s'",
-item, node.name, status)
+if self.op.command == constants.OOB_HEALTH:
+  # For health we should log important events
+  for item, status in result.payload:
+if status in [constants.OOB_STATUS_WARNING,
+  constants.OOB_STATUS_CRITICAL]:
+  self.LogWarning("Item '%s' on node '%s' has status '%s'",
+  item, node.name, status)
 
-  if self.op.command == constants.OOB_POWER_ON:
-node.powered = True
-  elif self.op.command == constants.OOB_POWER_OFF:
-node.powered = False
-  elif self.op.command == constants.OOB_POWER_STATUS:
-powered = result.payload[constants.OOB_POWER_STATUS_POWERED]
-if powered != node.powered:
-  logging.warning(("Recorded power state (%s) of node '%s' does 
not"
-   " match actual power state (%s)"), node.powered,
-  node.name, powered)
+if self.op.command == constants.OOB_POWER_ON:
+  node.powered = True
+elif self.op.command == constants.OOB_POWER_OFF:
+  node.powered = False
+elif self.op.command == constants.OOB_POWER_STATUS:
+  powered = result.payload[constants.OOB_POWER_STATUS_POWERED]
+  if powered != node.powered:
+logging.warning(("Recorded power state (%s) of node '%s' does not"
+ " match actual power state (%s)"), node.powered,
+node.name, powered)
 
-  # For configuration changing commands we should update the node
-  if self.op.command in (constants.OOB_POWER_ON,
- constants.OOB_POWER_OFF):
-self.cfg.Update(node, feedback_fn)
+# For configuration changing commands we should update the node
+if self.op.command in (constants.OOB_POWER_ON,
+   constants.OOB_POWER_OFF):
+  self.cfg.Update(node, feedback_fn)
 
-  node_entry.append((constants.RS_NORMAL, result.payload))
+node_entry.append((constants.RS_NORMAL, result.payload))
 
-  if (self.op.command == constants.OOB_POWER_ON and
-  idx < len(self.nodes) - 1):
-time.sleep(self.op.power_delay)
+if (self.op.command == constants.OOB_POWER_ON and
+idx < len(self.nodes) - 1):
+  time.sleep(self.op.power_delay)
 
 return ret
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 02/17] Quell pylint unbalanced-tuple-unpacking warning

['Brian Foley' via ganeti-devel]

Both of these functions return a list, not a tuple, and by manually
tracing the code, we can see they always return non-empty lists.

Change the callers to get the first element of the list rather than
using destructuring.

No functional change.

Signed-off-by: Brian Foley 
---
 lib/cmdlib/instance_storage.py | 4 ++--
 lib/rpc/node.py| 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/cmdlib/instance_storage.py b/lib/cmdlib/instance_storage.py
index af1c2e8..3abc5f8 100644
--- a/lib/cmdlib/instance_storage.py
+++ b/lib/cmdlib/instance_storage.py
@@ -596,8 +596,8 @@ def GenerateDiskTemplate(
   raise errors.ProgrammerError("Wrong template configuration")
 remote_node_uuid = secondary_node_uuids[0]
 
-(drbd_params, _, _) = objects.Disk.ComputeLDParams(template_name,
-   full_disk_params)
+drbd_params = objects.Disk.ComputeLDParams(template_name,
+   full_disk_params)[0]
 drbd_default_metavg = drbd_params[constants.LDP_DEFAULT_METAVG]
 
 names = []
diff --git a/lib/rpc/node.py b/lib/rpc/node.py
index cd2050d..f8bfa79 100644
--- a/lib/rpc/node.py
+++ b/lib/rpc/node.py
@@ -953,7 +953,7 @@ class RpcRunner(_RpcClientBase,
 """Wrapper for L{AnnotateDiskParams}.
 
 """
-(anno_disk,) = self._DisksDictDP(node, ([disk], instance))
+anno_disk = self._DisksDictDP(node, ([disk], instance))[0]
 return anno_disk
 
   def _EncodeNodeToDiskDictDP(self, node, value):
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 08/17] Disable pylint bad-continuation warning

['Brian Foley' via ganeti-devel]

pylint is much more strict than pep8, and it would be too invasive
(and arguably pointless) to update these right now.

Signed-off-by: Brian Foley 
---
 Makefile.am | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index 88d30c6..c4a54fc 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2695,7 +2695,9 @@ PEP8_EXCLUDE = $(subst $(space),$(comma),$(strip $(notdir 
$(built_python_sources
 # overzealous, eg where we use parens to make it clear that we're
 # deliberately doing a comparison that should yield bool, or are using
 # parens clarify precedence or to allow multi-line expressions.
-LINT_DISABLE = I0013 R0912 R0204 C0325
+# C0330 = disable wrong indentation warnings. pylint is much more strict than
+# pep8, and it would be too invasive to fix all these.
+LINT_DISABLE = I0013 R0912 R0204 C0325 C0330
 # Additional pylint options
 LINT_OPTS =
 # The combined set of pylint options
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 14/17] Reduce nesting in LUInstanceCreate.RunOsScripts

['Brian Foley' via ganeti-devel]

This avoids a pylint too-many-nested-blocks warning.

NB it's hard to see from the diff because of all the whitespace, but
this just replaces the top "if iobj.disk and not self.adopt_disks" with
a pair of guards that return early, and dedents the rest of the fn.

git diff -w will show this.

No functional change.

Signed-off-by: Brian Foley 
---
 lib/cmdlib/instance_create.py | 229 +-
 1 file changed, 117 insertions(+), 112 deletions(-)

diff --git a/lib/cmdlib/instance_create.py b/lib/cmdlib/instance_create.py
index fe6c71f..1a43e10 100644
--- a/lib/cmdlib/instance_create.py
+++ b/lib/cmdlib/instance_create.py
@@ -1193,122 +1193,127 @@ class LUInstanceCreate(LogicalUnit):
 @param iobj: instance object
 
 """
-if iobj.disks and not self.adopt_disks:
-  disks = self.cfg.GetInstanceDisks(iobj.uuid)
-  if self.op.mode == constants.INSTANCE_CREATE:
-os_image = objects.GetOSImage(self.op.osparams)
-
-if os_image is None and not self.op.no_install:
-  pause_sync = (not self.op.wait_for_sync and
-utils.AnyDiskOfType(disks, constants.DTS_INT_MIRROR))
-  if pause_sync:
-feedback_fn("* pausing disk sync to install instance OS")
-result = self.rpc.call_blockdev_pause_resume_sync(self.pnode.uuid,
-  (disks, iobj),
-  True)
-for idx, success in enumerate(result.payload):
-  if not success:
-logging.warn("pause-sync of instance %s for disk %d failed",
- self.op.instance_name, idx)
-
-  feedback_fn("* running the instance OS create scripts...")
+if not iobj.disks:
+  return
+
+if self.adopt_disks:
+  return
+
+disks = self.cfg.GetInstanceDisks(iobj.uuid)
+if self.op.mode == constants.INSTANCE_CREATE:
+  os_image = objects.GetOSImage(self.op.osparams)
+
+  if os_image is None and not self.op.no_install:
+pause_sync = (not self.op.wait_for_sync and
+  utils.AnyDiskOfType(disks, constants.DTS_INT_MIRROR))
+if pause_sync:
+  feedback_fn("* pausing disk sync to install instance OS")
+  result = self.rpc.call_blockdev_pause_resume_sync(self.pnode.uuid,
+(disks, iobj),
+True)
+  for idx, success in enumerate(result.payload):
+if not success:
+  logging.warn("pause-sync of instance %s for disk %d failed",
+   self.op.instance_name, idx)
+
+feedback_fn("* running the instance OS create scripts...")
+# FIXME: pass debug option from opcode to backend
+os_add_result = \
+  self.rpc.call_instance_os_add(self.pnode.uuid,
+(iobj, self.op.osparams_secret),
+False,
+self.op.debug_level)
+if pause_sync:
+  feedback_fn("* resuming disk sync")
+  result = self.rpc.call_blockdev_pause_resume_sync(self.pnode.uuid,
+(disks, iobj),
+False)
+  for idx, success in enumerate(result.payload):
+if not success:
+  logging.warn("resume-sync of instance %s for disk %d failed",
+   self.op.instance_name, idx)
+
+os_add_result.Raise("Could not add os for instance %s"
+" on node %s" % (self.op.instance_name,
+ self.pnode.name))
+
+else:
+  if self.op.mode == constants.INSTANCE_IMPORT:
+feedback_fn("* running the instance OS import scripts...")
+
+transfers = []
+
+for idx, image in enumerate(self.src_images):
+  if not image:
+continue
+
+  if iobj.os:
+dst_io = constants.IEIO_SCRIPT
+dst_ioargs = ((disks[idx], iobj), idx)
+  else:
+dst_io = constants.IEIO_RAW_DISK
+dst_ioargs = (disks[idx], iobj)
+
   # FIXME: pass debug option from opcode to backend
-  os_add_result = \
-self.rpc.call_instance_os_add(self.pnode.uuid,
-  (iobj, self.op.osparams_secret),
-  False,
-  self.op.debug_level)
-  if pause_sync:
-feedback_fn("* resuming disk sync")
-result = self.rpc.call_blockdev_pause_resume_sync(self.pnode.uuid,
-  (disks, iobj),
-

[PATCH stable-2.15 10/17] Reduce nesting in _CheckVLANArguments

['Brian Foley' via ganeti-devel]

This avoids a pylint too-many-nested-blocks warning. It also has the
happy side effect of removing some duplicated code.

No functional change.

Signed-off-by: Brian Foley 
---
 lib/cmdlib/instance_create.py | 19 ---
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/lib/cmdlib/instance_create.py b/lib/cmdlib/instance_create.py
index 462522c..fe6c71f 100644
--- a/lib/cmdlib/instance_create.py
+++ b/lib/cmdlib/instance_create.py
@@ -77,6 +77,12 @@ from ganeti.cmdlib.instance_utils import \
 import ganeti.masterd.instance
 
 
+def _ValidateTrunkVLAN(vlan):
+  if not compat.all(vl.isdigit() for vl in vlan[1:].split(':')):
+raise errors.OpPrereqError("Specified VLAN parameter is invalid"
+   " : %s" % vlan, errors.ECODE_INVAL)
+
+
 class LUInstanceCreate(LogicalUnit):
   """Create an instance.
 
@@ -152,19 +158,10 @@ class LUInstanceCreate(LogicalUnit):
   # vlan starting with dot means single untagged vlan,
   # might be followed by trunk (:)
   if not vlan[1:].isdigit():
-vlanlist = vlan[1:].split(':')
-for vl in vlanlist:
-  if not vl.isdigit():
-raise errors.OpPrereqError("Specified VLAN parameter is "
-   "invalid : %s" % vlan,
- errors.ECODE_INVAL)
+_ValidateTrunkVLAN(vlan)
 elif vlan[0] == ":":
   # Trunk - tagged only
-  vlanlist = vlan[1:].split(':')
-  for vl in vlanlist:
-if not vl.isdigit():
-  raise errors.OpPrereqError("Specified VLAN parameter is invalid"
-   " : %s" % vlan, errors.ECODE_INVAL)
+  _ValidateTrunkVLAN(vlan)
 elif vlan.isdigit():
   # This is the simplest case. No dots, only single digit
   # -> Create untagged access port, dot needs to be added
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 03/17] Disable incorrect pylint assigning-non-slot warning

['Brian Foley' via ganeti-devel]

This occurs pretty heavily in lib/objects.py, where pylint isn't
correctly detecting the __slots__ assignment. This appears to be
a known issue: https://github.com/PyCQA/pylint/issues/379

Signed-off-by: Brian Foley 
---
 lib/objects.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/objects.py b/lib/objects.py
index d6e64c9..23b349c 100644
--- a/lib/objects.py
+++ b/lib/objects.py
@@ -35,11 +35,14 @@ pass to and from external parties.
 
 """
 
-# pylint: disable=E0203,W0201,R0902
+# pylint: disable=E0203,E0237,W0201,R0902
 
 # E0203: Access to member %r before its definition, since we use
 # objects.py which doesn't explicitly initialise its members
 
+# E0237: Assigning to attribute not defined in class slots. pylint doesn't
+# appear to notice many of the slots defined in __slots__ for several objects.
+
 # W0201: Attribute '%s' defined outside __init__
 
 # R0902: Allow instances of these objects to have more than 20 attributes
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 13/17] Reduce nesting in RemoveNodeSshKeyBulk key calculation

['Brian Foley' via ganeti-devel]

This avoids a pylint too-many-nested-blocks warning. This removes the
6th level of nesting in the function, but may also be marginally faster
by turning the calculation into the set difference operation it really
is.

No functional change.

Signed-off-by: Brian Foley 
---
 lib/backend.py | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/backend.py b/lib/backend.py
index 37ce31a..b262de7 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -1799,9 +1799,10 @@ def RemoveNodeSshKeyBulk(node_list,
 if master_uuid:
   master_keys = ssh.QueryPubKeyFile([master_uuid],
 key_file=pub_key_file)
-  for master_key in master_keys:
-if master_key in keys[node_info.uuid]:
-  keys[node_info.uuid].remove(master_key)
+
+  # Remove any master keys from the list of keys to remove from the 
node
+  keys[node_info.uuid] = list(
+  set(keys[node_info.uuid]) - set(master_keys))
 
   all_keys_to_remove.update(keys)
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 00/17] Cleanup for pylint 1.6.4, part two

['Brian Foley' via ganeti-devel]

This second set of patches cleans up enough code and disables enough checks
to get Ganeti to run through pylint 1.6.4 with no warnings.

Most of the patches are straightforward, but I separated out each of the 'reduce
nesting' patches to make them easier to review individually, as the diff itself
is often difficult to understand. Generally it's easier to compare the before
and after side by side with diff -y and diff -w to show that the difference is
mostly whitespace.

Brian Foley (17):
  Cleanup: Use new-style classes everywhere
  Quell pylint unbalanced-tuple-unpacking warning
  Disable incorrect pylint assigning-non-slot warning
  Disable pylint broad-except warnings
  Disable pylint too-many-branches warnings
  Disable pylint redefined-variable-type warning
  Disable pylint superfluous-parens warning
  Disable pylint bad-continuation warning
  Reduce nesting in StartDaemon
  Reduce nesting in _CheckVLANArguments
  Reduce nesting in gnt-cluster VerifyDisks missing disk loop
  Reduce nesting in RemoveNodeSshKeyBulk ssh logic
  Reduce nesting in RemoveNodeSshKeyBulk key calculation
  Reduce nesting in LUInstanceCreate.RunOsScripts
  Reduce nesting in LUOobCommand.Exec
  Reduce nesting in import-export ProcessChildIO
  Disable pylint too-many-nested-blocks in _RunCmdPipe

 Makefile.am   |  13 +-
 daemons/import-export |  46 ---
 lib/backend.py|  34 ++
 lib/bootstrap.py  |   2 +-
 lib/cli.py|   2 +-
 lib/client/gnt_cluster.py |  19 +--
 lib/cmdlib/instance_create.py | 248 +++---
 lib/cmdlib/instance_set_params.py |   4 +-
 lib/cmdlib/instance_storage.py|   6 +-
 lib/cmdlib/misc.py|  65 +-
 lib/jqueue/__init__.py|   6 +-
 lib/netutils.py   |   2 +-
 lib/objects.py|   5 +-
 lib/rpc/node.py   |   8 +-
 lib/rpc/transport.py  |   4 +-
 lib/server/noded.py   |   2 +-
 lib/ssh.py|   2 +-
 lib/utils/process.py  |  19 ++-
 lib/watcher/__init__.py   |   2 +-
 tools/ganeti-listrunner   |   2 +-
 tools/move-instance   |   2 +-
 21 files changed, 253 insertions(+), 240 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 07/17] Disable pylint superfluous-parens warning

['Brian Foley' via ganeti-devel]

There are too many cases where we deliberately wrap expressions in
parens, either to indicate comparisons, or to allow multiline
expressions without line continuation chars, or to clarify confusing
precedence.

While here, clean up a few unpythonic cases.

Signed-off-by: Brian Foley 
---
 Makefile.am   | 6 +-
 lib/bootstrap.py  | 2 +-
 lib/cli.py| 2 +-
 lib/client/gnt_cluster.py | 2 +-
 lib/cmdlib/instance_set_params.py | 4 ++--
 lib/netutils.py   | 2 +-
 lib/utils/process.py  | 4 ++--
 7 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 19101ca..88d30c6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2691,7 +2691,11 @@ PEP8_EXCLUDE = $(subst $(space),$(comma),$(strip 
$(notdir $(built_python_sources
 # R0204 = disable redefined-variable-type warning. There are a large number of
 # cases where Ganeti assigns multiple types (eg set/list, float/int) to
 # the same variable, and these are benign.
-LINT_DISABLE = I0013 R0912 R0204
+# C0325 = disable superfluous-parens. There are a lot of cases where this is
+# overzealous, eg where we use parens to make it clear that we're
+# deliberately doing a comparison that should yield bool, or are using
+# parens clarify precedence or to allow multi-line expressions.
+LINT_DISABLE = I0013 R0912 R0204 C0325
 # Additional pylint options
 LINT_OPTS =
 # The combined set of pylint options
diff --git a/lib/bootstrap.py b/lib/bootstrap.py
index 7b6fbfe..0a382c5 100644
--- a/lib/bootstrap.py
+++ b/lib/bootstrap.py
@@ -678,7 +678,7 @@ def InitCluster(cluster_name, mac_prefix, # pylint: 
disable=R0913, R0914
   for template, dt_params in diskparams.items():
 param_keys = set(dt_params.keys())
 default_param_keys = set(constants.DISK_DT_DEFAULTS[template].keys())
-if not (param_keys <= default_param_keys):
+if param_keys > default_param_keys:
   unknown_params = param_keys - default_param_keys
   raise errors.OpPrereqError("Invalid parameters for disk template %s:"
  " %s" % (template,
diff --git a/lib/cli.py b/lib/cli.py
index f01d8d9..14bd4ea 100644
--- a/lib/cli.py
+++ b/lib/cli.py
@@ -2944,7 +2944,7 @@ def _NotAContainer(data):
   @rtype: bool
 
   """
-  return not (isinstance(data, (list, dict, tuple)))
+  return not isinstance(data, (list, dict, tuple))
 
 
 def _GetAlignmentMapping(data):
diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py
index 3eaf292..b32e62e 100644
--- a/lib/client/gnt_cluster.py
+++ b/lib/client/gnt_cluster.py
@@ -195,7 +195,7 @@ def InitCluster(opts, args):
   # check the disk template types here, as we cannot rely on the type check 
done
   # by the opcode parameter types
   diskparams_keys = set(diskparams.keys())
-  if not (diskparams_keys <= constants.DISK_TEMPLATES):
+  if not diskparams_keys > constants.DISK_TEMPLATES:
 unknown = utils.NiceSort(diskparams_keys - constants.DISK_TEMPLATES)
 ToStderr("Disk templates unknown: %s" % utils.CommaJoin(unknown))
 return 1
diff --git a/lib/cmdlib/instance_set_params.py 
b/lib/cmdlib/instance_set_params.py
index a35e95c..9ffbfdf 100644
--- a/lib/cmdlib/instance_set_params.py
+++ b/lib/cmdlib/instance_set_params.py
@@ -1260,7 +1260,7 @@ class LUInstanceSetParams(LogicalUnit):
   res_min = ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_min,
 new_disk_types)
 
-  if (res_max or res_min):
+  if res_max or res_min:
 # FIXME: Improve error message by including information about whether
 # the upper or lower limit of the parameter fails the ipolicy.
 msg = ("Instance allocation to group %s (%s) violates policy: %s" %
@@ -1628,7 +1628,7 @@ class LUInstanceSetParams(LogicalUnit):
 disk = self.GenericGetDiskInfo(uuid, name)
 
 # Rename disk before attaching (if disk is filebased)
-if disk.dev_type in (constants.DTS_INSTANCE_DEPENDENT_PATH):
+if disk.dev_type in constants.DTS_INSTANCE_DEPENDENT_PATH:
   # Add disk size/mode, else GenerateDiskTemplate will not work.
   params[constants.IDISK_SIZE] = disk.size
   params[constants.IDISK_MODE] = str(disk.mode)
diff --git a/lib/netutils.py b/lib/netutils.py
index 2eada25..ab94723 100644
--- a/lib/netutils.py
+++ b/lib/netutils.py
@@ -428,7 +428,7 @@ class IPAddress(object):
 @return: True if valid, False otherwise
 
 """
-assert (isinstance(netmask, (int, long)))
+assert isinstance(netmask, (int, long))
 
 return 0 < netmask <= cls.iplen
 
diff --git a/lib/utils/process.py b/lib/utils/process.py
index 29d8b0c..9183e9f 100644
--- a/lib/utils/process.py
+++ b/lib/utils/process.py
@@ -934,12 +934,12 @@ def Daemonize(logfile):
 
   # this might fail
   pid = os.fork()
-  if (pid == 0):  # The first child.
+  if pid == 0:  # The first child.
 

[PATCH stable-2.15 16/17] Reduce nesting in import-export ProcessChildIO

['Brian Foley' via ganeti-devel]

This avoids some pylint too-many-nested-blocks warnings. Do this by
flattening some 'arrow antipattern code' inside the polling loop to use
guards instead.

This is hard to see from diff, but git difftool -y -x 'diff -y' will
show the before and after side by side.

Sprinkle some comments while we're here.

Signed-off-by: Brian Foley 
---
 daemons/import-export | 46 --
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/daemons/import-export b/daemons/import-export
index 6c794f6..2dad4e6 100755
--- a/daemons/import-export
+++ b/daemons/import-export
@@ -323,27 +323,37 @@ def ProcessChildIO(child, socat_stderr_read_fd, 
dd_stderr_read_fd,
 
   # Read up to 1 KB of data
   data = from_.read(1024)
-  if data:
-if to:
-  to.write(data)
-elif fd == signal_notify.fileno():
-  # Signal handling
-  if signal_handler.called:
-signal_handler.Clear()
-if exit_timeout:
-  logging.info("Child process still has about %0.2f seconds"
-   " to exit", exit_timeout.Remaining())
-else:
-  logging.info("Giving child process %0.2f seconds to exit",
-   constants.CHILD_LINGER_TIMEOUT)
-  exit_timeout = \
-utils.RunningTimeout(constants.CHILD_LINGER_TIMEOUT, True)
-  else:
+
+  # On error, remove the mapping
+  if not data:
 poller.unregister(fd)
 del fdmap[fd]
+continue
 
-elif event & (select.POLLNVAL | select.POLLHUP |
-  select.POLLERR):
+  # If the data needs to be sent to another fd, write it
+  if to:
+to.write(data)
+continue
+
+  # Did we get a signal?
+  if fd != signal_notify.fileno():
+continue
+
+  # Has it been handled?
+  if not signal_handler.called:
+continue
+
+  # If so, clean up after it.
+  signal_handler.Clear()
+  if exit_timeout:
+logging.info("Child process still has about %0.2f seconds"
+ " to exit", exit_timeout.Remaining())
+  else:
+logging.info("Giving child process %0.2f seconds to exit",
+ constants.CHILD_LINGER_TIMEOUT)
+exit_timeout = \
+  utils.RunningTimeout(constants.CHILD_LINGER_TIMEOUT, True)
+elif event & (select.POLLNVAL | select.POLLHUP | select.POLLERR):
   poller.unregister(fd)
   del fdmap[fd]
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 17/17] Disable pylint too-many-nested-blocks in _RunCmdPipe

['Brian Foley' via ganeti-devel]

There doesn't appear to be any easy way of reducing the complexity
of this function without moving it elsewhere or completely reorganising
the function, so disable this warning for the time being.

Signed-off-by: Brian Foley 
---
 lib/utils/process.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/utils/process.py b/lib/utils/process.py
index bb5aed2..05553fc 100644
--- a/lib/utils/process.py
+++ b/lib/utils/process.py
@@ -530,6 +530,7 @@ def _RunCmdPipe(cmd, env, via_shell, cwd, interactive, 
timeout, noclose_fds,
   @return: (out, err, status)
 
   """
+  # pylint: disable=R0101
   poller = select.poll()
 
   if interactive:
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 04/17] Disable pylint broad-except warnings

['Brian Foley' via ganeti-devel]

These are all deliberate top-level catch-any-unhandled-exception cases,
used for logging and error handling so just get pylint to ignore them.

Signed-off-by: Brian Foley 
---
 lib/server/noded.py | 2 +-
 lib/watcher/__init__.py | 2 +-
 tools/ganeti-listrunner | 2 +-
 tools/move-instance | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/server/noded.py b/lib/server/noded.py
index fbf301b..cbf9626 100644
--- a/lib/server/noded.py
+++ b/lib/server/noded.py
@@ -200,7 +200,7 @@ class NodeRequestHandler(http.server.HttpServerHandler):
   # And return the error's arguments, which must be already in
   # correct tuple format
   result = err.args
-except Exception, err:
+except Exception, err: # pylint: disable=W0703
   logging.exception("Error in RPC call")
   result = (False, "Error while executing backend function: %s" % str(err))
 
diff --git a/lib/watcher/__init__.py b/lib/watcher/__init__.py
index 9c2a272..be6da0b 100644
--- a/lib/watcher/__init__.py
+++ b/lib/watcher/__init__.py
@@ -919,7 +919,7 @@ def Main():
 logging.error("Job queue is full, can't query cluster state")
   except errors.JobQueueDrainError:
 logging.error("Job queue is drained, can't maintain cluster state")
-  except Exception, err:
+  except Exception, err: # pylint: disable=W0703
 logging.exception(str(err))
 return constants.EXIT_FAILURE
 
diff --git a/tools/ganeti-listrunner b/tools/ganeti-listrunner
index d8bb2a2..7aebfca 100755
--- a/tools/ganeti-listrunner
+++ b/tools/ganeti-listrunner
@@ -420,7 +420,7 @@ def HostWorker(logdir, username, password, use_agent, 
hostname,
 print "  %s: received KeyboardInterrupt, aborting" % hostname
 WriteLog("ERROR: ABORT_KEYBOARD_INTERRUPT", logfile)
 result = 1
-  except Exception, err:
+  except Exception, err: # pylint: disable=W0703
 result = 1
 trace = traceback.format_exc()
 msg = "ERROR: UNHANDLED_EXECPTION_ERROR: %s\nTrace: %s" % (err, trace)
diff --git a/tools/move-instance b/tools/move-instance
index 84b5972..e43bd5c 100755
--- a/tools/move-instance
+++ b/tools/move-instance
@@ -414,7 +414,7 @@ class MoveRuntime(object):
   errmsg = None
 except Abort:
   errmsg = "Aborted"
-except Exception, err:
+except Exception, err:  # pylint: disable=W0703
   logging.exception("Caught unhandled exception")
   errmsg = str(err)
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 11/17] Reduce nesting in gnt-cluster VerifyDisks missing disk loop

['Brian Foley' via ganeti-devel]

To avoid pylint's too-many-nested-blocks warning.

No functional change.

Signed-off-by: Brian Foley 
---
 lib/client/gnt_cluster.py | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py
index b32e62e..ad02400 100644
--- a/lib/client/gnt_cluster.py
+++ b/lib/client/gnt_cluster.py
@@ -829,14 +829,15 @@ def VerifyDisks(opts, args):
 if all_missing:
   ToStdout("Instance %s cannot be verified as it lives on"
" broken nodes", iname)
-else:
-  ToStdout("Instance %s has missing logical volumes:", iname)
-  ival.sort()
-  for node, vol in ival:
-if node in bad_nodes:
-  ToStdout("\tbroken node %s /dev/%s", node, vol)
-else:
-  ToStdout("\t%s /dev/%s", node, vol)
+  continue
+
+ToStdout("Instance %s has missing logical volumes:", iname)
+ival.sort()
+for node, vol in ival:
+  if node in bad_nodes:
+ToStdout("\tbroken node %s /dev/%s", node, vol)
+  else:
+ToStdout("\t%s /dev/%s", node, vol)
 
   ToStdout("You need to replace or recreate disks for all the above"
" instances if this message persists after fixing broken 
nodes.")
-- 
2.8.0.rc3.226.g39d4020

[PATCH stable-2.15 06/17] Disable pylint redefined-variable-type warning

['Brian Foley' via ganeti-devel]

There are a large number of cases where Ganeti assigns multiple types
(eg set/list, float/int) to the same variable at different times, and
although these would make a type checking tool very unhappy, they are
benign here (and besides, cleaning all this up would be too invasive).

Signed-off-by: Brian Foley 
---
 Makefile.am | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index ba99fb8..19101ca 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2688,7 +2688,10 @@ PEP8_EXCLUDE = $(subst $(space),$(comma),$(strip 
$(notdir $(built_python_sources
 # I0013 = disable warnings for ignoring whole files
 # R0912 = disable too many branches warning. It's useful, but ganeti requires
 # a lot of refactoring to fix this.
-LINT_DISABLE = I0013 R0912
+# R0204 = disable redefined-variable-type warning. There are a large number of
+# cases where Ganeti assigns multiple types (eg set/list, float/int) to
+# the same variable, and these are benign.
+LINT_DISABLE = I0013 R0912 R0204
 # Additional pylint options
 LINT_OPTS =
 # The combined set of pylint options
-- 
2.8.0.rc3.226.g39d4020

Re: Issue 1180 in ganeti: Ceph/RBD support is broken with Ceph 'Jewel' due to incorrect showmapped command

[ganeti]

Updates:
Cc: da...@mcbf.net

Comment #3 on issue 1180 by bpfo...@google.com: Ceph/RBD support is broken  
with Ceph 'Jewel' due to incorrect showmapped command

https://code.google.com/p/ganeti/issues/detail?id=1180

I know almost nothing about ceph, but just looking at the sources for  
hammer and jewel, it does appear that the rbd command line tool in jewel  
lost the --pool/-p option that hammer had.


I think your fix isn't going to work entirely properly though. What happens  
if you have two mappings in different pools that happen to have the same  
name? I think you're going to have to change _ParseRbdShowmappedJson to  
filter out mappings from the specified pool.


Also, it looks like the rbd subcommands create, rm, map, resize, import and  
export also have the same problem. In hammer you can do rbd $SUB_CMD -p  
mypool mymap or rbd $SUB_CMD mypool/mymap (the parsing of the second is  
implemented by set_pool_image_name(...) in src/rbd.cc).


In jewel only the second syntax appears to be supported (again, just by  
eyeballing the source code).


I think that  you're going to need to update Create, Remove,  
_MapVolumeToBlockdev, Close, Import and Export in RADOSBlockDevice to use  
the mypool/mymap syntax.


David, do you have any input on this?

--
You received this message because this project is configured to send all  
issue notifications to this address.

You may adjust your notification preferences at:
https://code.google.com/hosting/settings

Re: Issue 1024 in ganeti: instance-wide kvm cpu pinning doesn't work

[ganeti]

Comment #3 on issue 1024 by a.jazdze...@googlemail.com: instance-wide kvm  
cpu pinning doesn't work

https://code.google.com/p/ganeti/issues/detail?id=1024

the path lead to an error on ubuntu 16.04:

Could not start instance 'db01': Error while executing backend  
function: 'Process' object has no attribute 'set_cpu_affinity'


i uncommet it on my side and use taskset -pac  

--
You received this message because this project is configured to send all  
issue notifications to this address.

You may adjust your notification preferences at:
https://code.google.com/hosting/settings

Re: Issue 1180 in ganeti: Ceph/RBD support is broken with Ceph 'Jewel' due to incorrect showmapped command

[ganeti]

Comment #2 on issue 1180 by a.jazdze...@googlemail.com: Ceph/RBD support is  
broken with Ceph 'Jewel' due to incorrect showmapped command

https://code.google.com/p/ganeti/issues/detail?id=1180

this will fix it since the "-p option" no loger exists in "rbd showmapped"

--- a/lib/storage/bdev.py
+++ b/lib/storage/bdev.py
@@ -1020,8 +1020,6 @@ class RADOSBlockDevice(base.BlockDev):
   showmap_cmd = [
 constants.RBD_CMD,
 "showmapped",
-"-p",
-pool,
 "--format",
 "json"
 ]


--
You received this message because this project is configured to send all  
issue notifications to this address.

You may adjust your notification preferences at:
https://code.google.com/hosting/settings