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

Wed Mar 4 18:58:50 UTC 2015

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 at 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.

Martin^2

-- 
Martin Basti