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

[Martin Kosek]

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-457-.mailmap-use-correct-name-format-for-adam.patch
Type: text/x-patch
Size: 861 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140220/a02af94a/attachment.bin>