----- 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 Subject: [PATCH] Add --force option to ipactl 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 https://fedorahosted.org/freeipa/ticket/3509 --- install/tools/ipactl | 109 +++++++++++++++++++++++++-------------------- install/tools/man/ipactl.8 | 6 +++ 2 files changed, 67 insertions(+), 48 deletions(-) diff --git a/install/tools/ipactl b/install/tools/ipactl index 58c7f316d54c7a75d321f39c6974e902b86aaa7c..202081d45a023ba1ebe7394eb581049f09742290 100755 --- a/install/tools/ipactl +++ b/install/tools/ipactl @@ -87,6 +87,9 @@ 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 +192,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: + dirsrv.stop(capture_output=False) + except Exception: + pass + + def ipa_start(options): if os.path.isfile(ipaservices.get_svc_list_file()): @@ -214,10 +234,10 @@ def ipa_start(options): except Exception, e: emit_err("Failed to data from service file: " + str(e)) emit_err("Shutting down") - try: - dirsrv.stop(capture_output=False) - except: - pass + + if not options.force: + stop_dirsrv(dirsrv) + if isinstance(e, IpactlError): # do not display any other error message raise IpactlError(rval=e.rval) @@ -233,19 +253,17 @@ def ipa_start(options): try: print "Starting %s Service" % svc svchandle.start(capture_output=get_capture_output(svc, options.debug)) - except: + except Exception: emit_err("Failed to start %s Service" % svc) + #if force start specified, skip rollback and continue with the next service + if options.force: + emit_err("Forced start, ignoring %s Service, continuing normal operation" % svc) + continue + emit_err("Shutting down") - for svc in svc_list: - svc_off = ipaservices.service(svc) - try: - svc_off.stop(capture_output=False) - except: - pass - try: - dirsrv.stop(capture_output=False) - except: - pass + stop_services(svc_list) + stop_dirsrv(dirsrv) + raise IpactlError("Aborting ipactl") def ipa_stop(options): @@ -354,16 +372,11 @@ def ipa_restart(options): except Exception, e: emit_err("Failed to restart Directory Service: " + str(e)) emit_err("Shutting down") - for svc in reversed(svc_list): - svc_off = ipaservices.service(svc) - try: - svc_off.stop(capture_output=False) - except: - pass - try: - dirsrv.stop(capture_output=False) - except: - pass + + if not options.force: + stop_services(reversed(svc_list)) + stop_dirsrv(dirsrv) + raise IpactlError("Aborting ipactl") if len(svc_list) != 0: @@ -374,19 +387,17 @@ def ipa_restart(options): try: print "Restarting %s Service" % svc svchandle.restart(capture_output=get_capture_output(svc, options.debug)) - except: + except Exception: emit_err("Failed to restart %s Service" % svc) + #if force start specified, skip rollback and continue with the next service + if options.force: + emit_err("Forced restart, ignoring %s Service, continuing normal operation" % svc) + continue + emit_err("Shutting down") - for svc in reversed(svc_list): - svc_off = ipaservices.service(svc) - try: - svc_off.stop(capture_output=False) - except: - pass - try: - dirsrv.stop(capture_output=False) - except: - pass + stop_services(svc_list) + stop_dirsrv(dirsrv) + raise IpactlError("Aborting ipactl") if len(new_svc_list) != 0: @@ -396,25 +407,27 @@ def ipa_restart(options): try: print "Starting %s Service" % svc svchandle.start(capture_output=get_capture_output(svc, options.debug)) - except: + except Exception: emit_err("Failed to start %s Service" % svc) + #if force start specified, skip rollback and continue with the next service + if options.force: + emit_err("Forced start, ignoring %s Service, continuing normal operation" % svc) + continue + emit_err("Shutting down") - for svc in reversed(svc_list): - svc_off = ipaservices.service(svc) - try: - svc_off.stop(capture_output=False) - except: - pass - try: - dirsrv.stop(capture_output=False) - except: - pass + stop_services(svc_list) + stop_dirsrv(dirsrv) + raise IpactlError("Aborting ipactl") def ipa_status(options): try: - svc_list = get_config_from_file() + dirsrv = ipaservices.knownservices.dirsrv + if dirsrv.is_running(): + svc_list = get_config(dirsrv) + else: + svc_list = get_config_from_file() except IpactlError, e: if os.path.exists(ipaservices.get_svc_list_file()): raise e diff --git a/install/tools/man/ipactl.8 b/install/tools/man/ipactl.8 index 05be8e0e29f792ad2a2159ca3f8f38624a42ffa4..5a1fd27ad6cb88877589173709c6cf0afa357fe1 100644 --- a/install/tools/man/ipactl.8 +++ b/install/tools/man/ipactl.8 @@ -37,3 +37,9 @@ Stop all of the services that make up IPA .TP restart Stop then start all of the services that make up IPA +.TP +\fB\-d\fR, \fB\-\-debug\fR +Display debugging information +.TP +\fB\-f\fR, \fB\-\-force\fR +If any service start fails, do not rollback the services, continue with the operation -- 1.8.5.3
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
