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

Mon Mar 16 14:58:34 UTC 2015

On 03/16/2015 01:56 PM, Martin Babinsky wrote:
> On 03/13/2015 10:13 AM, Martin Kosek wrote:
>> On 03/12/2015 09:43 PM, Nathan Kinder wrote:
>>>
>>>
>>> On 03/04/2015 11:25 AM, Nathan Kinder wrote:
>>>>
>>>>
>>>> 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.
>>>
>>> Bump.  Does anyone else have any review feedback on this?  It would be
>>> nice to get this in soon since we currently have the potential of just
>>> hanging when installing clients on F21+.
>>
>> I am ok with the approach, if the patches work. I agree it would be nice
>> to have this fixed in F21 and F22 soon.
>>
>> Martin, could you please take a look? This one should be easy to test.
>>
>> Thanks,
>> Martin
> 
> I have tested the patches on F21 client and they work as expected.
> 

I take that as an ACK. Before pushing the change, I just changed one print
format from "%s" to "%d" given a number was printed.

Pushed to:
master: a58b77ca9cd3620201306258dd6bd05ea1c73c73
ipa-4-1: 80aeb445e2034776f08668bf04dfd711af477b25

Petr1, it would be nice to get this one built on F21+, to unblock Ipsilon project.