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

[Nathan Kinder]

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.

Thanks,
-NGK

> 
>>
>> rob
>>
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Timeout-when-performing-time-sync-during-client-inst.patch
Type: text/x-patch
Size: 3175 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150228/6da4339e/attachment.bin>