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

Wed Mar 4 19:25:29 UTC 2015

On 03/04/2015 10:58 AM, Martin Basti wrote:
> 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.

OK.  A new patch is attached that puts the path to 'timeout' in the
proper location.  Fixing up the order of other paths is unrelated, and
should be handled in a separate patch.

-NGK

> 
> Martin^2
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Timeout-when-performing-time-sync-during-client-inst.patch
Type: text/x-patch
Size: 4080 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150304/dfdeb299/attachment.bin>