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. 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? Stylistically, should we prefer p.returncode is 15 or p.returncode == 15? rob _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel