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

Fri Mar 13 09:13:36 UTC 2015

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