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.
>>>>>>>> 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:
>>>>>> 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  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  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 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 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 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 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/ as BIN_TIMEOUT =
"/usr/bin/timeout" and reference that instead? It's for portability.

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?

Stylistically, should we prefer p.returncode is 15 or p.returncode == 15?


Freeipa-devel mailing list

Reply via email to