[Freeipa-devel] [PATCH]Add -f option to ipactl

Thu Feb 20 14:42:19 UTC 2014


----- Original Message -----
> From: "Martin Kosek" <mkosek at redhat.com>
> To: "Adam Misnyovszki" <amisnyov at redhat.com>, freeipa-devel at 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" <mkosek at redhat.com>
> >> To: dpal at redhat.com, "Petr Spacek" <pspacek at redhat.com>
> >> Cc: freeipa-devel at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-amisnyov-0003-2-Add-force-option-to-ipactl.patch
Type: text/x-patch
Size: 7418 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140220/48960693/attachment.bin>