[PATCH stable-2.11] Read if IPv6 is used directly in SshRunner

[Neal Oakey]

Instead of using an parameter which isn't beeing set everywhere

Fixing issue #892

Signed-off-by: Neal Oakey <neal....@googlemail.com>
---
lib/bootstrap.py | 5 ++---
lib/ssh.py | 8 ++++----
2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/lib/bootstrap.py b/lib/bootstrap.py
index 96627e2..99aa7c1 100644
--- a/lib/bootstrap.py
+++ b/lib/bootstrap.py
@@ -344,9 +344,7 @@ def RunNodeSetupCmd(cluster_name, node, basecmd, debug, verbose,
if port is None:
port = netutils.GetDaemonPort(constants.SSH)

- family = ssconf.SimpleStore().GetPrimaryIPFamily()
- srun = ssh.SshRunner(cluster_name,
- ipv6=(family == netutils.IP6Address.family))
+ srun = ssh.SshRunner(cluster_name)
scmd = srun.BuildCmd(node, constants.SSH_LOGIN_USER,
utils.ShellQuoteArgs(
utils.ShellCombineCommands(all_cmds)),
@@ -368,6 +366,7 @@ def RunNodeSetupCmd(cluster_name, node, basecmd, debug, verbose,
raise errors.OpExecError("Command '%s' failed: %s" %
(result.cmd, result.fail_reason))

+ family = ssconf.SimpleStore().GetPrimaryIPFamily()
_WaitForSshDaemon(node, port, family)


diff --git a/lib/ssh.py b/lib/ssh.py
index af64f3e..599b7cc 100644
--- a/lib/ssh.py
+++ b/lib/ssh.py
@@ -43,6 +43,7 @@ from ganeti import netutils
from ganeti import pathutils
from ganeti import vcluster
from ganeti import compat
+from ganeti import ssconf


def GetUserFiles(user, mkdir=False, dircheck=True, kind=constants.SSHK_DSA,
@@ -119,17 +120,16 @@ class SshRunner(object):
"""Wrapper for SSH commands.

"""
- def __init__(self, cluster_name, ipv6=False):
+ def __init__(self, cluster_name):
"""Initializes this class.

@type cluster_name: str
@param cluster_name: name of the cluster
- @type ipv6: bool
- @param ipv6: If true, force ssh to use IPv6 addresses only

"""
self.cluster_name = cluster_name
- self.ipv6 = ipv6
+ family = ssconf.SimpleStore().GetPrimaryIPFamily()
+ self.ipv6 = (family == netutils.IP6Address.family)

def _BuildSshOptions(self, batch, ask_key, use_cluster_key,
strict_host_check, private_key=None, quiet=True,
--
1.7.10.4

[Klaus Aehlig]

Dear Neal,

I'm a bit confused by your patch. You say..

On Mon, Oct 13, 2014 at 09:32:04AM +0000, Neal Oakey wrote:
> Instead of using an parameter which isn't beeing set everywhere

....and with that justification one the one hand...

> if port is None:
> port = netutils.GetDaemonPort(constants.SSH)
>
> - family = ssconf.SimpleStore().GetPrimaryIPFamily()
> - srun = ssh.SshRunner(cluster_name,
> - ipv6=(family == netutils.IP6Address.family))
> + srun = ssh.SshRunner(cluster_name)

....don't trust ssconf to provide the primary family, whereas, in
the same function, ...

> raise errors.OpExecError("Command '%s' failed: %s" %
> (result.cmd, result.fail_reason))
>
> + family = ssconf.SimpleStore().GetPrimaryIPFamily()
> _WaitForSshDaemon(node, port, family)

....you use it to determine to where to wait for the ssh daemon
to come up. Since the code inbetween does not change anything
related to ssconf, why can't it be used earlier in that function?

Thanks,
Klaus

--
Klaus Aehlig
Google Germany GmbH, Dienerstr. 12, 80331 Muenchen
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschaeftsfuehrer: Graham Law, Christine Elizabeth Flores

[Neal Oakey]

Hi Klaus,

well I moved the check if we are using a IPv6 Network into the SshRunner,
as this was only checked in the bootstrap.py currently and missing in
the 4(?) other places.

On 13/10/14 12:07, Klaus Aehlig wrote:
> Dear Neal,
>
> I'm a bit confused by your patch. You say..
>
> On Mon, Oct 13, 2014 at 09:32:04AM +0000, Neal Oakey wrote:
>> Instead of using an parameter which isn't beeing set everywhere
> ....and with that justification one the one hand...

I don't quite get what you mean.
And I think the message isn't formulated to good, because one has to
have read the Subject first.

>
>> if port is None:
>> port = netutils.GetDaemonPort(constants.SSH)
>>
>> - family = ssconf.SimpleStore().GetPrimaryIPFamily()
>> - srun = ssh.SshRunner(cluster_name,
>> - ipv6=(family == netutils.IP6Address.family))
>> + srun = ssh.SshRunner(cluster_name)
> ....don't trust ssconf to provide the primary family, whereas, in
> the same function, ...
>
>> raise errors.OpExecError("Command '%s' failed: %s" %
>> (result.cmd, result.fail_reason))
>>
>> + family = ssconf.SimpleStore().GetPrimaryIPFamily()
>> _WaitForSshDaemon(node, port, family)
> ....you use it to determine to where to wait for the ssh daemon
> to come up. Since the code inbetween does not change anything
> related to ssconf, why can't it be used earlier in that function?

I only moved the "family =" down, as it is no longer used above,
ok maybe I should / could move it into the WaitForSshDaemon().
>
> Thanks,
> Klaus
>

[Klaus Aehlig]

> I only moved the "family =" down, as it is no longer used above,
> ok maybe I should / could move it into the WaitForSshDaemon().

Well, the point is: you use what you claim to not be
reliable---and that doesn't seem consistent.

[Neal Oakey]

Hi,

on the other side, WaitForSshDaemon() requires the parameter,
this means you will have to define it.

but I'll change it and then commit the patch again, ok?

Greetings,
Neal

[Neal Oakey]

Moved check if the Primary Network is running on IPv6 into the SshRunner.
Until now SshRunner was only initialized correctly in the bootstrap.py
and not in the other 5 places.

Fixing issue #892

Signed-off-by: Neal Oakey <neal....@googlemail.com>
---

lib/bootstrap.py | 9 ++++-----
lib/ssh.py | 8 ++++----
2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/lib/bootstrap.py b/lib/bootstrap.py
index 96627e2..40a09df 100644
--- a/lib/bootstrap.py
+++ b/lib/bootstrap.py
@@ -261,10 +261,11 @@ def _WaitForMasterDaemon():
" %s seconds" % _DAEMON_READY_TIMEOUT)


-def _WaitForSshDaemon(hostname, port, family):
+def _WaitForSshDaemon(hostname, port):
"""Wait for SSH daemon to become responsive.

"""
+ family = ssconf.SimpleStore().GetPrimaryIPFamily()
hostip = netutils.GetHostname(name=hostname, family=family).ip

def _CheckSshDaemon():
@@ -344,9 +345,7 @@ def RunNodeSetupCmd(cluster_name, node, basecmd, debug, verbose,

if port is None:
port = netutils.GetDaemonPort(constants.SSH)

- family = ssconf.SimpleStore().GetPrimaryIPFamily()
- srun = ssh.SshRunner(cluster_name,
- ipv6=(family == netutils.IP6Address.family))
+ srun = ssh.SshRunner(cluster_name)

scmd = srun.BuildCmd(node, constants.SSH_LOGIN_USER,
utils.ShellQuoteArgs(
utils.ShellCombineCommands(all_cmds)),

@@ -368,7 +367,7 @@ def RunNodeSetupCmd(cluster_name, node, basecmd, debug, verbose,

raise errors.OpExecError("Command '%s' failed: %s" %
(result.cmd, result.fail_reason))

- _WaitForSshDaemon(node, port, family)
+ _WaitForSshDaemon(node, port)


def _InitFileStorageDir(file_storage_dir):

[Klaus Aehlig]

On Mon, Oct 13, 2014 at 03:43:30PM +0000, Neal Oakey wrote:
> Moved check if the Primary Network is running on IPv6 into the SshRunner.
> Until now SshRunner was only initialized correctly in the bootstrap.py
> and not in the other 5 places.
>
> Fixing issue #892
>
> Signed-off-by: Neal Oakey <neal....@googlemail.com>
> ---
> lib/bootstrap.py | 9 ++++-----
> lib/ssh.py | 8 ++++----
> 2 files changed, 8 insertions(+), 9 deletions(-)

LGTM