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@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
