On 08/08/2013 02:32 PM, Tomas Babej wrote:
On 08/07/2013 09:57 PM, Nathaniel McCallum wrote:
In the interest of getting ticket 3777 moving, I have created the
attached patch. It is *NOT* tested as I don't have a replica setup at
the moment and won't have time until after Flock. If someone else has a
replica setup and has a few minutes to burn, you are welcome to test the
patch.

Nathaniel


_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

The replica installation fails with:

Connection from replica to master is OK.
Start listening on required ports for remote master check
Get credentials to log in to remote master
Check SSH connection to remote master
Traceback (most recent call last):
   File "/usr/sbin/ipa-replica-conncheck", line 418, in <module>
     sys.exit(main())
   File "/usr/sbin/ipa-replica-conncheck", line 392, in main
     stdout, stderr, returncode = ssh('echo OK', verbose=True)
   File "/usr/sbin/ipa-replica-conncheck", line 70, in __call__
     return ipautil.run(cmd, env=env, raiseonerr=False)
   File "/usr/lib/python2.7/site-packages/ipapython/ipautil.py", line
303, in run
     close_fds=True, env=env, cwd=cwd)
   File "/usr/lib64/python2.7/subprocess.py", line 711, in __init__
     errread, errwrite)
   File "/usr/lib64/python2.7/subprocess.py", line 1308, in _execute_child
     raise child_exception
TypeError: execve() arg 3 must be a mapping object
Connection check failed!
Please fix your network settings according to error messages above.
If the check results are not valid it can be skipped with
--skip-conncheck parameter.

The culprit here is the comma after the definition of env:

+        env = {'KRB5_CONFIG': KRB5_CONFIG, 'KRB5CCNAME': CCACHE_FILE},
+        return ipautil.run(cmd, env=env, raiseonerr=False)

With that fixed the patch works as expected.

I have addtitional minor comments:

+class SshExec(object):
+    def __init__(self, user, addr):
+        self.user = user
+        self.addr = addr
+        self.cmd =
+
[...]

The refactored way of calling commands using SSH is nice, so why keep it
hidden in ipa-replica-connecheck? I'd rather see this as part of
ipapython.ipautil.

I don't think this is generic enough for ipautil. For example it doesn't do custom options (e.g. custom SSH keys), you can't feed in custom stdin, it (almost silently) does nothing when SSH is not installed. If we need it elsewhere we can refactor and move it to a more general place -- hopefully, its own new module -- but let's not get ahead of ourselves.

ipautil is currently a mess with lots of unrelated functions, many of which we only use at one place, some probably aren't used at all. Let's not continue dumping anything that looks a bit reusable in there.

Also,

+    def __call__(self, command, verbose=False):
+        # Bail if ssh is not installed
+        if self.cmd is None:
+            print "WARNING: ssh not installed, skipping ssh test"

The following message needs to be changed for allowing more generic
use-case.

+            print "WARNING: ssh not installed, skipping"

Should be sufficient.

Not necessary, if you agree with my rant above...
(In a reusable solution, this should raise an error that would be handled outside of the class.)

--
PetrĀ³

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to