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

2015-03-26 Thread Petr Vobornik

On 03/16/2015 03:58 PM, Martin Kosek wrote:

On 03/16/2015 01:56 PM, Martin Babinsky wrote:


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.



I've noticed that patch 0001 was not pushed.

Pushed to:
master:
* f0c1daf7a2a8c88f6d84d81d66c7e39f571e0894 Skip time sync during client 
install when using --no-ntp

ipa-4-1:
* b5969c1d1ae6eb1e392e0420fcbf094ae7b34102 Skip time sync during client 
install when using --no-ntp


Note: it's part of Fedora builds as a separate patch (that's why I noticed)
--
Petr Vobornik

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


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

2015-03-17 Thread Petr Vobornik

On 03/16/2015 03:58 PM, Martin Kosek wrote:

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:


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.



F21 update:
- https://admin.fedoraproject.org/updates/freeipa-4.1.3-3.fc21

F22 update:
- https://admin.fedoraproject.org/updates/freeipa-4.1.3-3.fc22
--
Petr Vobornik

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


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

2015-03-16 Thread Martin Babinsky

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 

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

2015-03-16 Thread Martin Kosek
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 

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

2015-03-13 Thread Martin Kosek

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 

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

2015-03-12 Thread Nathan Kinder


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 

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

2015-03-04 Thread Nathan Kinder


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 

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

2015-03-04 Thread Rob Crittenden
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.

rob

___
Freeipa-devel mailing 

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

2015-03-04 Thread Martin Basti

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



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

2015-02-28 Thread Nathan Kinder


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.

Thanks,
-NGK

 

 rob

 
 ___
 Freeipa-devel mailing list
 Freeipa-devel@redhat.com
 https://www.redhat.com/mailman/listinfo/freeipa-devel
 
From 4585810dcf0ff27e2026a5afd446f428942b25a7 Mon Sep 17 00:00:00 2001
From: Nathan Kinder nkin...@redhat.com
Date: Wed, 25 Feb 2015 15:19:47 -0800
Subject: [PATCH 2/2] Timeout when performing time sync during client install

We use ntpd now to sync time before fetching a TGT during client
install.  Unfortuantely, ntpd will hang forever if it is unable to
reach the NTP server.

This patch adds the ability for commands run via ipautil.run() to
have an optional timeout.  This capability is used by the NTP sync
code that is run during ipa-client-install.

Ticket: https://fedorahosted.org/freeipa/ticket/4842
---
 ipa-client/ipaclient/ntpconf.py |  4 +++-
 ipapython/ipautil.py| 12 +++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/ipa-client/ipaclient/ntpconf.py b/ipa-client/ipaclient/ntpconf.py
index e1ac55a..2cc3332 100644
--- a/ipa-client/ipaclient/ntpconf.py
+++ b/ipa-client/ipaclient/ntpconf.py
@@ -149,7 +149,9 @@ def synconce_ntp(server_fqdn):
 
 tmp_ntp_conf = ipautil.write_tmp_file('server %s' % server_fqdn)
 try:
-ipautil.run([ntpd, '-qgc', tmp_ntp_conf.name])
+# The ntpd command will never exit if it is unable to reach the
+# 

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

2015-02-28 Thread Nathan Kinder


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

From b9ecde14c990a20195c5759537fff47101e14bd5 Mon Sep 17 00:00:00 2001
From: Nathan Kinder nkin...@redhat.com
Date: Wed, 25 Feb 2015 15:19:47 -0800
Subject: [PATCH 2/2] Timeout when performing time sync during client install

We use ntpd now to sync time before fetching a TGT during client
install.  

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

2015-02-28 Thread Rob Crittenden
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


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

2015-02-28 Thread Nathan Kinder


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.

Thanks,
-NGK

 
 rob
 

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


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

2015-02-27 Thread Rob Crittenden
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?

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


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

2015-02-27 Thread Nathan Kinder


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.

-NGK

 
 ?
 
 Martin
 

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


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

2015-02-27 Thread Nathan Kinder


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.

-NGK

 
 rob
 

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


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

2015-02-27 Thread Nathan Kinder


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().

 
 rob
 

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


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

2015-02-27 Thread Rob Crittenden
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.

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


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

2015-02-26 Thread Martin Kosek
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

?

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel