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/freeipa-devel
