[Freeipa-devel] [PATCHES 0001-0002] ipa-client-install NTP fixes

[Nathan Kinder]

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.

-NGK

> 
> rob
>