Re: [Freeipa-devel] [PATCHES 0001-0002] ipa-client-install NTP fixes
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
[Freeipa-devel] [PATCHES 0001-0002] ipa-client-install NTP fixes
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 From 80514f225f628f7c7993b85e562a851e7ee40644 Mon Sep 17 00:00:00 2001 From: Nathan Kinder nkin...@redhat.com Date: Wed, 25 Feb 2015 14:22:02 -0800 Subject: [PATCH 1/2] Skip time sync during client install when using --no-ntp When --no-ntp is specified during ipa-client-install, we still attempt to perform a time sync before obtaining a TGT from the KDC. We should not be attempting to sync time with the KDC if we are explicitly told to not configure ntp. Ticket: https://fedorahosted.org/freeipa/ticket/4842 --- ipa-client/ipa-install/ipa-client-install | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index ccaab55..a625fbd 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -2324,8 +2324,9 @@ def install(options, env, fstore, statestore): # hostname if different from system hostname tasks.backup_and_replace_hostname(fstore, statestore, options.hostname) -if not options.on_master: +if not options.on_master and options.conf_ntp: # Attempt to sync time with IPA server. +# If we're skipping NTP configuration, we also skip the time sync here. # We assume that NTP servers are discoverable through SRV records in the DNS # If that fails, we try to sync directly with IPA server, assuming it runs NTP root_logger.info('Synchronizing time with KDC...') -- 1.9.3 From 7ba0745b10145f516536a2b1b711203b5bb8cb09 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. This new timeout capability requires a new dependency on the subprocess32 module, which is a backport of the Python 3.x subprocess module for Python 2.x. Ticket: https://fedorahosted.org/freeipa/ticket/4842 --- freeipa.spec.in | 1 + ipa-client/ipaclient/ntpconf.py | 4 +++- ipapython/ipautil.py| 21 +++-- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/freeipa.spec.in b/freeipa.spec.in index b186d9f..8379b31 100644 --- a/freeipa.spec.in +++ b/freeipa.spec.in @@ -285,6 +285,7 @@ Requires: libipa_hbac-python Requires: python-qrcode-core = 5.0.0 Requires: python-pyasn1 Requires: python-dateutil +Requires: python-subprocess32 Requires: python-yubico Requires: wget Requires: dbus-python 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 +# server, so timeout after 15 seconds. +ipautil.run([ntpd, '-qgc', tmp_ntp_conf.name], timeout=15) return True except ipautil.CalledProcessError: return False diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py index 4116d97..41046fc 100644 --- a/ipapython/ipautil.py +++ b/ipapython/ipautil.py @@ -20,6 +20,7 @@ import string import tempfile import subprocess +import subprocess32 import random import os, sys, traceback import copy @@ -38,6 +39,7 @@ import krbV import pwd from dns import resolver, rdatatype from dns.exception import DNSException +from subprocess32 import TimeoutExpired from ipapython.ipa_log_manager import * from ipapython import ipavalidate @@ -249,7 +251,7 @@ def shell_quote(string): def run(args, stdin=None, raiseonerr=True, nolog=(), env=None, capture_output=True,