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

Thu Feb 20 15:42:15 UTC 2014

On 02/20/2014 03:42 PM, Adam Misnyovszki wrote:
> 
> 
> ----- 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
> 

Thanks. ACK.

Pushed to master: 189bdcb95d4d2346ad5859c2717e7b94dcca018b

Martin