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

Mon Mar 16 12:56:02 UTC 2015

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.

-- 
Martin^3 Babinsky