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@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

--
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

Reply via email to