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 >>>>>> [email protected] >>>>>> 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. -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
