On 03/04/2015 10:58 AM, Martin Basti wrote:
> On 04/03/15 19:56, Nathan Kinder wrote:
>>
>> On 03/04/2015 10:41 AM, Rob Crittenden wrote:
>>> Nathan Kinder wrote:
>>>>
>>>> On 02/28/2015 01:13 PM, Nathan Kinder wrote:
>>>>>
>>>>> On 02/28/2015 01:07 PM, Rob Crittenden wrote:
>>>>>> Nathan Kinder wrote:
>>>>>>>
>>>>>>> On 02/27/2015 01:18 PM, Nathan Kinder wrote:
>>>>>>>>
>>>>>>>> On 02/27/2015 01:08 PM, Rob Crittenden wrote:
>>>>>>>>> Nathan Kinder wrote:
>>>>>>>>>>
>>>>>>>>>> On 02/27/2015 12:20 PM, Rob Crittenden wrote:
>>>>>>>>>>> Nathan Kinder wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 02/26/2015 12:55 AM, Martin Kosek wrote:
>>>>>>>>>>>>> On 02/26/2015 03:28 AM, Nathan Kinder wrote:
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The two attached patches address some issues that affect
>>>>>>>>>>>>>> ipa-client-install when syncing time from the NTP server. 
>>>>>>>>>>>>>> Now that we
>>>>>>>>>>>>>> use ntpd to perform the time sync, the client install can
>>>>>>>>>>>>>> end up hanging
>>>>>>>>>>>>>> forever when the server is not reachable (firewall issues,
>>>>>>>>>>>>>> etc.).  These
>>>>>>>>>>>>>> patches address the issues in two different ways:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1 - Don't attempt to sync time when --no-ntp is specified.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2 - Implement a timeout capability that is used when we
>>>>>>>>>>>>>> run ntpd to
>>>>>>>>>>>>>> perform the time sync to prevent indefinite hanging.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The one potentially contentious issue is that this
>>>>>>>>>>>>>> introduces a new
>>>>>>>>>>>>>> dependency on python-subprocess32 to allow us to have
>>>>>>>>>>>>>> timeout support
>>>>>>>>>>>>>> when using Python 2.x.  This is packaged for Fedora, but I
>>>>>>>>>>>>>> don't see it
>>>>>>>>>>>>>> on RHEL or CentOS currently.  It would need to be packaged
>>>>>>>>>>>>>> there.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4842
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> -NGK
>>>>>>>>>>>>> Thanks for Patches. For the second patch, I would really
>>>>>>>>>>>>> prefer to avoid new
>>>>>>>>>>>>> dependency, especially if it's not packaged in RHEL/CentOS.
>>>>>>>>>>>>> Maybe we could use
>>>>>>>>>>>>> some workaround instead, as in:
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://stackoverflow.com/questions/3733270/python-subprocess-timeout
>>>>>>>>>>>>>
>>>>>>>>>>>> I don't like having to add an additional dependency either,
>>>>>>>>>>>> but the
>>>>>>>>>>>> alternative seems more risky.  Utilizing the subprocess32
>>>>>>>>>>>> module (which
>>>>>>>>>>>> is really just a backport of the normal subprocess module
>>>>>>>>>>>> from Python
>>>>>>>>>>>> 3.x) is not invasive for our code in ipautil.run().  Adding
>>>>>>>>>>>> some sort of
>>>>>>>>>>>> a thread that has to kill the spawned subprocess seems more
>>>>>>>>>>>> risky (see
>>>>>>>>>>>> the discussion about a race condition in the stackoverflow
>>>>>>>>>>>> thread
>>>>>>>>>>>> above).  That said, I'm sure the thread/poll method can be
>>>>>>>>>>>> made to work
>>>>>>>>>>>> if you and others feel strongly that this is a better
>>>>>>>>>>>> approach than
>>>>>>>>>>>> adding a new dependency.
>>>>>>>>>>> Why not use /usr/bin/timeout from coreutils?
>>>>>>>>>> That sounds like a perfectly good idea.  I wasn't aware of it's
>>>>>>>>>> existence (or it's possible that I forgot about it).  Thanks
>>>>>>>>>> for the
>>>>>>>>>> suggestion!  I'll test out a reworked version of the patch.
>>>>>>>>>>
>>>>>>>>>> Do you think that there is value in leaving the timeout
>>>>>>>>>> capability
>>>>>>>>>> centrally in ipautil.run()?  We only need it for the call to
>>>>>>>>>> 'ntpd'
>>>>>>>>>> right now, but there might be a need for using a timeout for
>>>>>>>>>> other
>>>>>>>>>> commands in the future.  The alternative is to just modify
>>>>>>>>>> synconce_ntp() to use /usr/bin/timeout and leave ipautil.run()
>>>>>>>>>> alone.
>>>>>>>>> I think it would require a lot of research. One of the programs
>>>>>>>>> spawned
>>>>>>>>> by this is pkicreate which could take quite some time, and
>>>>>>>>> spawning a
>>>>>>>>> clone in particular.
>>>>>>>>>
>>>>>>>>> It is definitely an interesting idea but I think it is safest
>>>>>>>>> for now to
>>>>>>>>> limit it to just NTP for now.
>>>>>>>> What I meant was that we would have an optional keyword "timeout"
>>>>>>>> parameter to ipautil.run() that defaults to None, just like my
>>>>>>>> subprocess32 approach.  If a timeout is not passed in, we would use
>>>>>>>> subprocess.Popen() to run the specified command just like we do
>>>>>>>> today.
>>>>>>>> We would only actually pass the timeout parameter to
>>>>>>>> ipautil.run() in
>>>>>>>> synconce_ntp() for now, so no other commands would have a
>>>>>>>> timeout in
>>>>>>>> effect.  The capability would be available for other commands
>>>>>>>> this way
>>>>>>>> though.
>>>>>>>>
>>>>>>>> Let me propose a patch with this implementation, and if you
>>>>>>>> don't like
>>>>>>>> it, we can leave ipautil.run() alone and restrict the changes to
>>>>>>>> synconce_ntp().
>>>>>>> An updated patch 0002 is attached that uses the approach
>>>>>>> mentioned above.
>>>>>> Looks good. Not to nitpick to death but...
>>>>>>
>>>>>> Can you add timeout to ipaplatform/base/paths.py as BIN_TIMEOUT =
>>>>>> "/usr/bin/timeout" and reference that instead? It's for portability.
>>>>> Sure.  I was wondering if we should do something around a full path.
>>>>>
>>>>>> And a question. I'm impatient. Should there be a notice that it will
>>>>>> timeout after n seconds somewhere so people like me don't ^C after 2
>>>>>> seconds? Or is that just overkill and I need to learn patience?
>>>>> Probably both. :)  There's always going to be someone out there who
>>>>> will
>>>>> do ctrl-C, so I think printing out a notice is a good idea.  I'll
>>>>> add this.
>>>>>
>>>>>> Stylistically, should we prefer p.returncode is 15 or p.returncode
>>>>>> == 15?
>>>>> After some reading, it seems that '==' should be used.  Small integers
>>>>> work with 'is', but '==' is the consistent way that equality of
>>>>> integers
>>>>> should be checked.  I'll modify this.
>>>> Another updated patch 0002 is attached that addresses Rob's review
>>>> comments.
>>>>
>>>> Thanks,
>>>> -NGK
>>>>
>>> LGTM. Does someone else have time to test this?
>>>
>>> I also don't know if there is a policy on placement of new items in
>>> paths.py. Things are all over the place and some have BIN_ prefix and
>>> some don't.
>> Yeah, I noticed this too.  It didn't look like there was any
>> organization.
>>
>> -NGK
>>> rob
>>>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
> paths are (should be) ordered alphabetically by value, not by variable
> name.
> I see there are last 2 lines ordered incorrectly, but please try to keep
> order as I wrote above.

OK.  A new patch is attached that puts the path to 'timeout' in the
proper location.  Fixing up the order of other paths is unrelated, and
should be handled in a separate patch.

-NGK

> 
> Martin^2
> 
>From 8c6aaa8a9b2829f9cfff402dc65f2b5a9a93813b Mon Sep 17 00:00:00 2001
From: Nathan Kinder <nkin...@redhat.com>
Date: Wed, 25 Feb 2015 15:19:47 -0800
Subject: [PATCH 2/2] Timeout when performing time sync during client install

We use ntpd now to sync time before fetching a TGT during client
install.  Unfortuantely, ntpd will hang forever if it is unable to
reach the NTP server.

This patch adds the ability for commands run via ipautil.run() to
have an optional timeout.  This capability is used by the NTP sync
code that is run during ipa-client-install.

Ticket: https://fedorahosted.org/freeipa/ticket/4842
---
 ipa-client/ipaclient/ntpconf.py |  8 +++++++-
 ipaplatform/base/paths.py       |  1 +
 ipapython/ipautil.py            | 12 +++++++++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/ipa-client/ipaclient/ntpconf.py b/ipa-client/ipaclient/ntpconf.py
index e1ac55a..99e43a6 100644
--- a/ipa-client/ipaclient/ntpconf.py
+++ b/ipa-client/ipaclient/ntpconf.py
@@ -18,6 +18,7 @@
 #
 
 from ipapython import ipautil
+from ipapython.ipa_log_manager import root_logger
 import shutil
 import os
 from ipaplatform.tasks import tasks
@@ -149,7 +150,12 @@ def synconce_ntp(server_fqdn):
 
     tmp_ntp_conf = ipautil.write_tmp_file('server %s' % server_fqdn)
     try:
-        ipautil.run([ntpd, '-qgc', tmp_ntp_conf.name])
+        # The ntpd command will never exit if it is unable to reach the
+        # server, so timeout after 15 seconds.
+        timeout = 15
+        root_logger.info('Attempting to sync time using ntpd.  '
+                         'Will timeout after %s seconds' % timeout)
+        ipautil.run([ntpd, '-qgc', tmp_ntp_conf.name], timeout=timeout)
         return True
     except ipautil.CalledProcessError:
         return False
diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index 7922e3b..11c7e92 100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -186,6 +186,7 @@ class BasePathNamespace(object):
     SSLGET = "/usr/bin/sslget"
     SSS_SSH_AUTHORIZEDKEYS = "/usr/bin/sss_ssh_authorizedkeys"
     SSS_SSH_KNOWNHOSTSPROXY = "/usr/bin/sss_ssh_knownhostsproxy"
+    BIN_TIMEOUT = "/usr/bin/timeout"
     UPDATE_CA_TRUST = "/usr/bin/update-ca-trust"
     BIN_WGET = "/usr/bin/wget"
     ZIP = "/usr/bin/zip"
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4116d97..6a06a8e 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -249,7 +249,7 @@ def shell_quote(string):
 
 def run(args, stdin=None, raiseonerr=True,
         nolog=(), env=None, capture_output=True, skip_output=False, cwd=None,
-        runas=None):
+        runas=None, timeout=None):
     """
     Execute a command and return stdin, stdout and the process return code.
 
@@ -277,6 +277,8 @@ def run(args, stdin=None, raiseonerr=True,
     :param cwd: Current working directory
     :param runas: Name of a user that the command shold be run as. The spawned
         process will have both real and effective UID and GID set.
+    :param timeout: Timeout if the command hasn't returned within the specified
+        number of seconds.
     """
     p_in = None
     p_out = None
@@ -302,6 +304,11 @@ def run(args, stdin=None, raiseonerr=True,
         p_out = subprocess.PIPE
         p_err = subprocess.PIPE
 
+    if timeout:
+        # If a timeout was provided, use the timeout command
+        # to execute the requested command.
+        args[0:0] = [paths.BIN_TIMEOUT, str(timeout)]
+
     arg_string = nolog_replace(' '.join(shell_quote(a) for a in args), nolog)
     root_logger.debug('Starting external process')
     root_logger.debug('args=%s' % arg_string)
@@ -332,6 +339,9 @@ def run(args, stdin=None, raiseonerr=True,
         if skip_output:
             p_out.close()   # pylint: disable=E1103
 
+    if timeout and p.returncode == 124:
+        root_logger.debug('Process did not complete before timeout')
+
     root_logger.debug('Process finished, return code=%s', p.returncode)
 
     # The command and its output may include passwords that we don't want
-- 
1.9.3

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

Reply via email to