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.

> rob

Freeipa-devel mailing list

Reply via email to