Re: [Freeipa-devel] [PATCH]Add -f option to ipactl
On 02/19/2014 10:58 PM, Dmitri Pal wrote: On 02/19/2014 03:13 PM, Petr Spacek wrote: On 19.2.2014 21:10, Dmitri Pal wrote: On 02/19/2014 11:58 AM, Adam Misnyovszki wrote: Hi, I reviewed this old patch: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force/-f option prevents stopping of services that have successfully started. https://fedorahosted.org/freeipa/ticket/3509 I have not read the code but looked at the patch and bug. I do not understand the -force option especially with help string being: If ipactl action fails, do not stop the services that are already running force usually means the reverse: if something did not happen force it to happen. I am not sure that --force option is the right name for the option with this help. I have proposed --no-rollback but it didn't fit to habits in IPA: https://fedorahosted.org/freeipa/ticket/3509#comment:2 then it should be -fs/--force-start or something of this kind. IMO --force is not that bad, it forces start procedure to finish regardless of the result (if some service started or not). --force-start may be redundant: # ipactl start --force-start # ipactl restart --force-start But I am not insisting on --force, if there is better option I am open to it. Few comments for the patch itself: 1) Please update it so that it does not use this construct: +try: +svc_off.stop(capture_output=False) +except: +pass Bare except clauses also catch for example KeyboardInterrupt. Then if the stop command is stuck, I would not be able to CTRL+C it. except Exception: is better. 2) The --force does not work as I would wish. When --force option is used, I would like ipactl to try to start all services it can, regardless to if they failed or not. Now it just does not rollback, but it still does not start all services it can: # ipactl start --force Existing service file detected! Assuming stale, cleaning and proceeding Starting Directory Service Starting krb5kdc Service Starting kadmin Service Starting named Service Starting ipa_memcached Service Starting httpd Service Job for httpd.service failed. See 'systemctl status httpd.service' and 'journalctl -xn' for details. Failed to start httpd Service Shutting down == Aborting ipactl See? HTTPD did not start, ipactl stopped and it did not start pki-tomcatd or ipa-otpd even though they do not use HTTPD when starting. 3) I see we still write Shutting down even though we use --force. I would rather print Shutting down suppressed or Forced start, no service rollback. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH]Add -f option to ipactl
- Original Message - From: Martin Kosek mko...@redhat.com To: d...@redhat.com, Petr Spacek pspa...@redhat.com Cc: freeipa-devel@redhat.com Sent: Thursday, February 20, 2014 9:18:37 AM Subject: Re: [Freeipa-devel] [PATCH]Add -f option to ipactl On 02/19/2014 10:58 PM, Dmitri Pal wrote: On 02/19/2014 03:13 PM, Petr Spacek wrote: On 19.2.2014 21:10, Dmitri Pal wrote: On 02/19/2014 11:58 AM, Adam Misnyovszki wrote: Hi, I reviewed this old patch: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force/-f option prevents stopping of services that have successfully started. https://fedorahosted.org/freeipa/ticket/3509 I have not read the code but looked at the patch and bug. I do not understand the -force option especially with help string being: If ipactl action fails, do not stop the services that are already running force usually means the reverse: if something did not happen force it to happen. I am not sure that --force option is the right name for the option with this help. I have proposed --no-rollback but it didn't fit to habits in IPA: https://fedorahosted.org/freeipa/ticket/3509#comment:2 then it should be -fs/--force-start or something of this kind. IMO --force is not that bad, it forces start procedure to finish regardless of the result (if some service started or not). --force-start may be redundant: # ipactl start --force-start # ipactl restart --force-start But I am not insisting on --force, if there is better option I am open to it. Few comments for the patch itself: 1) Please update it so that it does not use this construct: +try: +svc_off.stop(capture_output=False) +except: +pass Bare except clauses also catch for example KeyboardInterrupt. Then if the stop command is stuck, I would not be able to CTRL+C it. except Exception: is better. 2) The --force does not work as I would wish. When --force option is used, I would like ipactl to try to start all services it can, regardless to if they failed or not. Now it just does not rollback, but it still does not start all services it can: # ipactl start --force Existing service file detected! Assuming stale, cleaning and proceeding Starting Directory Service Starting krb5kdc Service Starting kadmin Service Starting named Service Starting ipa_memcached Service Starting httpd Service Job for httpd.service failed. See 'systemctl status httpd.service' and 'journalctl -xn' for details. Failed to start httpd Service Shutting down == Aborting ipactl See? HTTPD did not start, ipactl stopped and it did not start pki-tomcatd or ipa-otpd even though they do not use HTTPD when starting. 3) I see we still write Shutting down even though we use --force. I would rather print Shutting down suppressed or Forced start, no service rollback. Martin Hi, - Corrected to the desired behaviour - ipactl status now shows stopped services also, if the directory server is running. Please review! Thanks: Adam ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel From a6353bd516ade894ae1ee78b7a765a7f628126d7 Mon Sep 17 00:00:00 2001 From: Misnyovszki Adam amisn...@redhat.com Date: Thu, 20 Feb 2014 14:11:11 +0100 Subject: [PATCH] If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force option prevents stopping of services that have successfully started, just skips the services which can not be started. ipactl status now shows stopped services also, if the directory server is running. With the contribution of Ana Krivokapic --- install/tools/ipactl | 108 + install/tools/man/ipactl.8 | 6 +++ 2 files changed, 66 insertions(+), 48 deletions(-) diff --git a/install/tools/ipactl b/install/tools/ipactl index 58c7f316d54c7a75d321f39c6974e902b86aaa7c..fb1f271cfa3e8cd64986f2594ca683e78ce5d0dc 100755 --- a/install/tools/ipactl +++ b/install/tools/ipactl @@ -87,6 +87,8 @@ def parse_options(): parser.add_option(-d, --debug, action=store_true, dest=debug, help=Display debugging information) +parser.add_option(-f, --force, action=store_true, dest=force, + help=If any service start fails, do not rollback the services, continue with the operation) options, args = parser.parse_args() safe_options = parser.get_safe_opts(options) @@ -189,6 +191,23 @@ def get_config_from_file(): return ordered_list + +def stop_services(svc_list): +for svc in svc_list: +svc_off = ipaservices.service(svc) +try: +svc_off.stop(capture_output=False) +except Exception: +pass + + +def stop_dirsrv(dirsrv): +try
Re: [Freeipa-devel] [PATCH]Add -f option to ipactl
On 02/20/2014 02:15 PM, Adam Misnyovszki wrote: - Original Message - From: Martin Kosek mko...@redhat.com To: d...@redhat.com, Petr Spacek pspa...@redhat.com Cc: freeipa-devel@redhat.com Sent: Thursday, February 20, 2014 9:18:37 AM Subject: Re: [Freeipa-devel] [PATCH]Add -f option to ipactl On 02/19/2014 10:58 PM, Dmitri Pal wrote: On 02/19/2014 03:13 PM, Petr Spacek wrote: On 19.2.2014 21:10, Dmitri Pal wrote: On 02/19/2014 11:58 AM, Adam Misnyovszki wrote: Hi, I reviewed this old patch: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force/-f option prevents stopping of services that have successfully started. https://fedorahosted.org/freeipa/ticket/3509 I have not read the code but looked at the patch and bug. I do not understand the -force option especially with help string being: If ipactl action fails, do not stop the services that are already running force usually means the reverse: if something did not happen force it to happen. I am not sure that --force option is the right name for the option with this help. I have proposed --no-rollback but it didn't fit to habits in IPA: https://fedorahosted.org/freeipa/ticket/3509#comment:2 then it should be -fs/--force-start or something of this kind. IMO --force is not that bad, it forces start procedure to finish regardless of the result (if some service started or not). --force-start may be redundant: # ipactl start --force-start # ipactl restart --force-start But I am not insisting on --force, if there is better option I am open to it. Few comments for the patch itself: 1) Please update it so that it does not use this construct: +try: +svc_off.stop(capture_output=False) +except: +pass Bare except clauses also catch for example KeyboardInterrupt. Then if the stop command is stuck, I would not be able to CTRL+C it. except Exception: is better. 2) The --force does not work as I would wish. When --force option is used, I would like ipactl to try to start all services it can, regardless to if they failed or not. Now it just does not rollback, but it still does not start all services it can: # ipactl start --force Existing service file detected! Assuming stale, cleaning and proceeding Starting Directory Service Starting krb5kdc Service Starting kadmin Service Starting named Service Starting ipa_memcached Service Starting httpd Service Job for httpd.service failed. See 'systemctl status httpd.service' and 'journalctl -xn' for details. Failed to start httpd Service Shutting down == Aborting ipactl See? HTTPD did not start, ipactl stopped and it did not start pki-tomcatd or ipa-otpd even though they do not use HTTPD when starting. 3) I see we still write Shutting down even though we use --force. I would rather print Shutting down suppressed or Forced start, no service rollback. Martin Hi, - Corrected to the desired behaviour - ipactl status now shows stopped services also, if the directory server is running. Please review! Thanks: Adam Functionally works fine, thanks. I am personally ok with --force option, so if anyone still objects to that, please yell. I still see few process issues though: 1) Please use FirstName LastName format of your name in From field to make it consistent with all others. Unfortunately, I did not notice that in previous review so it was commited wrongly. Thus I fixed that in .mailmap with a one-liner (attached). 2) Patch file name should contain patch version. See http://www.freeipa.org/page/Contribute/Patch_Format#Naming 3) Use shorter patch titles This is what happens now: $ git am /tmp/freeipa-amisnyov-0003-Add-force-option-to-ipactl.patch Applying: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force option prevents stopping of services that have successfully started, just skips the services which can not be started. 4) Wrap commit description to 80 chars. See `git log` for examples. 5) Try to keep your lines in code max 80 chars, when possible. This one is too long: +parser.add_option(-f, --force, action=store_true, dest=force, + help=If any service start fails, do not rollback the services, continue with the operation) Martin From 6bbc3368544fb898d5191ae1c19b0ef1e1809eb0 Mon Sep 17 00:00:00 2001 From: Martin Kosek mko...@redhat.com Date: Thu, 20 Feb 2014 14:49:39 +0100 Subject: [PATCH] .mailmap: use correct name format for Adam Name should be First-Name Last-Name. Map all Adam's contributions to this preferred format. --- .mailmap | 1 + 1 file changed, 1 insertion(+) diff --git a/.mailmap b/.mailmap index dba55678cf0291469d4264fe9b385ac1db95d19f..70774eb15d380cbd642fe865e07bf1678613c16a 100644 --- a/.mailmap +++ b/.mailmap @@ -1,4 +1,5 @@ Ana KrivokapiÄ akriv...@redhat.com Ana Krivokapic akriv...@redhat.com
Re: [Freeipa-devel] [PATCH]Add -f option to ipactl
- Original Message - From: Martin Kosek mko...@redhat.com To: Adam Misnyovszki amisn...@redhat.com, freeipa-devel@redhat.com Sent: Thursday, February 20, 2014 3:00:39 PM Subject: Re: [Freeipa-devel] [PATCH]Add -f option to ipactl On 02/20/2014 02:15 PM, Adam Misnyovszki wrote: - Original Message - From: Martin Kosek mko...@redhat.com To: d...@redhat.com, Petr Spacek pspa...@redhat.com Cc: freeipa-devel@redhat.com Sent: Thursday, February 20, 2014 9:18:37 AM Subject: Re: [Freeipa-devel] [PATCH]Add -f option to ipactl On 02/19/2014 10:58 PM, Dmitri Pal wrote: On 02/19/2014 03:13 PM, Petr Spacek wrote: On 19.2.2014 21:10, Dmitri Pal wrote: On 02/19/2014 11:58 AM, Adam Misnyovszki wrote: Hi, I reviewed this old patch: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force/-f option prevents stopping of services that have successfully started. https://fedorahosted.org/freeipa/ticket/3509 I have not read the code but looked at the patch and bug. I do not understand the -force option especially with help string being: If ipactl action fails, do not stop the services that are already running force usually means the reverse: if something did not happen force it to happen. I am not sure that --force option is the right name for the option with this help. I have proposed --no-rollback but it didn't fit to habits in IPA: https://fedorahosted.org/freeipa/ticket/3509#comment:2 then it should be -fs/--force-start or something of this kind. IMO --force is not that bad, it forces start procedure to finish regardless of the result (if some service started or not). --force-start may be redundant: # ipactl start --force-start # ipactl restart --force-start But I am not insisting on --force, if there is better option I am open to it. Few comments for the patch itself: 1) Please update it so that it does not use this construct: +try: +svc_off.stop(capture_output=False) +except: +pass Bare except clauses also catch for example KeyboardInterrupt. Then if the stop command is stuck, I would not be able to CTRL+C it. except Exception: is better. 2) The --force does not work as I would wish. When --force option is used, I would like ipactl to try to start all services it can, regardless to if they failed or not. Now it just does not rollback, but it still does not start all services it can: # ipactl start --force Existing service file detected! Assuming stale, cleaning and proceeding Starting Directory Service Starting krb5kdc Service Starting kadmin Service Starting named Service Starting ipa_memcached Service Starting httpd Service Job for httpd.service failed. See 'systemctl status httpd.service' and 'journalctl -xn' for details. Failed to start httpd Service Shutting down == Aborting ipactl See? HTTPD did not start, ipactl stopped and it did not start pki-tomcatd or ipa-otpd even though they do not use HTTPD when starting. 3) I see we still write Shutting down even though we use --force. I would rather print Shutting down suppressed or Forced start, no service rollback. Martin Hi, - Corrected to the desired behaviour - ipactl status now shows stopped services also, if the directory server is running. Please review! Thanks: Adam Functionally works fine, thanks. I am personally ok with --force option, so if anyone still objects to that, please yell. I still see few process issues though: 1) Please use FirstName LastName format of your name in From field to make it consistent with all others. Unfortunately, I did not notice that in previous review so it was commited wrongly. Thus I fixed that in .mailmap with a one-liner (attached). 2) Patch file name should contain patch version. See http://www.freeipa.org/page/Contribute/Patch_Format#Naming 3) Use shorter patch titles This is what happens now: $ git am /tmp/freeipa-amisnyov-0003-Add-force-option-to-ipactl.patch Applying: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force option prevents stopping of services that have successfully started, just skips the services which can not be started. 4) Wrap commit description to 80 chars. See `git log` for examples. 5) Try to keep your lines in code max 80 chars, when possible. This one is too long: +parser.add_option(-f, --force, action=store_true, dest=force, + help=If any service start fails, do not rollback the services, continue with the operation) Martin Hi, corrected all above. Thanks Adam From 542995aa3087af5ba983b22c6eae89095f718b69 Mon Sep 17 00:00:00 2001 From: Adam Misnyovszki amisn...@redhat.com Date: Thu, 20 Feb 2014 15:34:05 +0100
Re: [Freeipa-devel] [PATCH]Add -f option to ipactl
On 02/20/2014 03:42 PM, Adam Misnyovszki wrote: - Original Message - From: Martin Kosek mko...@redhat.com To: Adam Misnyovszki amisn...@redhat.com, freeipa-devel@redhat.com Sent: Thursday, February 20, 2014 3:00:39 PM Subject: Re: [Freeipa-devel] [PATCH]Add -f option to ipactl On 02/20/2014 02:15 PM, Adam Misnyovszki wrote: - Original Message - From: Martin Kosek mko...@redhat.com To: d...@redhat.com, Petr Spacek pspa...@redhat.com Cc: freeipa-devel@redhat.com Sent: Thursday, February 20, 2014 9:18:37 AM Subject: Re: [Freeipa-devel] [PATCH]Add -f option to ipactl On 02/19/2014 10:58 PM, Dmitri Pal wrote: On 02/19/2014 03:13 PM, Petr Spacek wrote: On 19.2.2014 21:10, Dmitri Pal wrote: On 02/19/2014 11:58 AM, Adam Misnyovszki wrote: Hi, I reviewed this old patch: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force/-f option prevents stopping of services that have successfully started. https://fedorahosted.org/freeipa/ticket/3509 I have not read the code but looked at the patch and bug. I do not understand the -force option especially with help string being: If ipactl action fails, do not stop the services that are already running force usually means the reverse: if something did not happen force it to happen. I am not sure that --force option is the right name for the option with this help. I have proposed --no-rollback but it didn't fit to habits in IPA: https://fedorahosted.org/freeipa/ticket/3509#comment:2 then it should be -fs/--force-start or something of this kind. IMO --force is not that bad, it forces start procedure to finish regardless of the result (if some service started or not). --force-start may be redundant: # ipactl start --force-start # ipactl restart --force-start But I am not insisting on --force, if there is better option I am open to it. Few comments for the patch itself: 1) Please update it so that it does not use this construct: +try: +svc_off.stop(capture_output=False) +except: +pass Bare except clauses also catch for example KeyboardInterrupt. Then if the stop command is stuck, I would not be able to CTRL+C it. except Exception: is better. 2) The --force does not work as I would wish. When --force option is used, I would like ipactl to try to start all services it can, regardless to if they failed or not. Now it just does not rollback, but it still does not start all services it can: # ipactl start --force Existing service file detected! Assuming stale, cleaning and proceeding Starting Directory Service Starting krb5kdc Service Starting kadmin Service Starting named Service Starting ipa_memcached Service Starting httpd Service Job for httpd.service failed. See 'systemctl status httpd.service' and 'journalctl -xn' for details. Failed to start httpd Service Shutting down == Aborting ipactl See? HTTPD did not start, ipactl stopped and it did not start pki-tomcatd or ipa-otpd even though they do not use HTTPD when starting. 3) I see we still write Shutting down even though we use --force. I would rather print Shutting down suppressed or Forced start, no service rollback. Martin Hi, - Corrected to the desired behaviour - ipactl status now shows stopped services also, if the directory server is running. Please review! Thanks: Adam Functionally works fine, thanks. I am personally ok with --force option, so if anyone still objects to that, please yell. I still see few process issues though: 1) Please use FirstName LastName format of your name in From field to make it consistent with all others. Unfortunately, I did not notice that in previous review so it was commited wrongly. Thus I fixed that in .mailmap with a one-liner (attached). 2) Patch file name should contain patch version. See http://www.freeipa.org/page/Contribute/Patch_Format#Naming 3) Use shorter patch titles This is what happens now: $ git am /tmp/freeipa-amisnyov-0003-Add-force-option-to-ipactl.patch Applying: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force option prevents stopping of services that have successfully started, just skips the services which can not be started. 4) Wrap commit description to 80 chars. See `git log` for examples. 5) Try to keep your lines in code max 80 chars, when possible. This one is too long: +parser.add_option(-f, --force, action=store_true, dest=force, + help=If any service start fails, do not rollback the services, continue with the operation) Martin Hi, corrected all above. Thanks Adam Thanks. ACK. Pushed to master: 189bdcb95d4d2346ad5859c2717e7b94dcca018b Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo
Re: [Freeipa-devel] [PATCH]Add -f option to ipactl
On 19.2.2014 21:10, Dmitri Pal wrote: On 02/19/2014 11:58 AM, Adam Misnyovszki wrote: Hi, I reviewed this old patch: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force/-f option prevents stopping of services that have successfully started. https://fedorahosted.org/freeipa/ticket/3509 I have not read the code but looked at the patch and bug. I do not understand the -force option especially with help string being: If ipactl action fails, do not stop the services that are already running force usually means the reverse: if something did not happen force it to happen. I am not sure that --force option is the right name for the option with this help. I have proposed --no-rollback but it didn't fit to habits in IPA: https://fedorahosted.org/freeipa/ticket/3509#comment:2 -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH]Add -f option to ipactl
On 02/19/2014 11:58 AM, Adam Misnyovszki wrote: Hi, I reviewed this old patch: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force/-f option prevents stopping of services that have successfully started. https://fedorahosted.org/freeipa/ticket/3509 I have not read the code but looked at the patch and bug. I do not understand the -force option especially with help string being: If ipactl action fails, do not stop the services that are already running force usually means the reverse: if something did not happen force it to happen. I am not sure that --force option is the right name for the option with this help. Seems like fine to me. Thanks Adam ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- Thank you, Dmitri Pal Sr. Engineering Manager for IdM portfolio Red Hat Inc. --- Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH]Add -f option to ipactl
On 02/19/2014 03:13 PM, Petr Spacek wrote: On 19.2.2014 21:10, Dmitri Pal wrote: On 02/19/2014 11:58 AM, Adam Misnyovszki wrote: Hi, I reviewed this old patch: If an error occurs in the start up sequence in ipactl start/restart, all the services are stopped. Using the --force/-f option prevents stopping of services that have successfully started. https://fedorahosted.org/freeipa/ticket/3509 I have not read the code but looked at the patch and bug. I do not understand the -force option especially with help string being: If ipactl action fails, do not stop the services that are already running force usually means the reverse: if something did not happen force it to happen. I am not sure that --force option is the right name for the option with this help. I have proposed --no-rollback but it didn't fit to habits in IPA: https://fedorahosted.org/freeipa/ticket/3509#comment:2 then it should be -fs/--force-start or something of this kind. -- Thank you, Dmitri Pal Sr. Engineering Manager for IdM portfolio Red Hat Inc. --- Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
