[Freeipa-devel] [PATCH 0063] Raise error on topology disconnect/last-role-host removal during server uninstall

[Martin Babinsky]

On 08/17/2016 02:38 PM, Stanislav Laznicka wrote:
> On 08/17/2016 02:17 PM, Martin Babinsky wrote:
>> On 08/16/2016 03:47 PM, Stanislav Laznicka wrote:
>>> On 08/15/2016 02:20 PM, Martin Babinsky wrote:
>>>> On 08/15/2016 02:13 PM, Martin Babinsky wrote:
>>>>> On 08/12/2016 12:08 PM, Stanislav Laznicka wrote:
>>>>>> Hello,
>>>>>>
>>>>>> topology disconnect/last-role-host removal errors would just be
>>>>>> logged
>>>>>> during server uninstall even if ignore options are not present. The
>>>>>> host
>>>>>> would still appear in the topology even after "successful" uninstall.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/6168
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> The patch seems to be ok, however shouldn't we use sys.exit() when
>>>>> handling ServerRemovalError? Yes raising SystemExit from within a
>>>>> function is a horrible practice, but it is already done on several
>>>>> other
>>>>> places instead of letting the exception bubble up to the main handler.
>>>>>
>>>>> CC'ing Jan for his thoughts on this since I may be wrong.
>>>>>
>>>> Hmm, you will definitely need sys.exit() here since otherwise
>>>> ipa-server-install reports 0 exit code even if there was an exception
>>>> thrown:
>>>>
>>>> """
>>>> [root at master1 ~]# ipa-server-install --uninstall -U
>>>> ipa         : ERROR    Server removal aborted: Deleting this server
>>>> will leave your installation without a DNS..
>>>> [root at master1 ~]# echo $?
>>>> 0
>>>> """
>>>>
>>> Because of #5750 (remove sys.exit() calls from installer modules) I
>>> rather raise ScriptError in this case. See the modified patch. It
>>> depends on either of my 48-4 or 48-5 patches (pick one).
>>>
>>
>> Make sure you raise it using str(e) because ScriptError does not
>> coerce the argument to string/unicode:
>>
>> @@ -303,7 +303,7 @@ def
>> remove_master_from_managed_topology(api_instance, options):
>>          replication.run_server_del_as_cli(
>>              api_instance, api_instance.env.host, **server_del_options)
>>      except errors.ServerRemovalError as e:
>> -        raise ScriptError(e)
>> +        raise ScriptError(str(e))
>>      except Exception as e:
>>          # if the master was already deleted we will just get a warning
>>          root_logger.warning("Failed to delete master: {}".format(e))
>>
> Oops, sorry, attached is the fixed patch.
>> Regardless of this fix, I still get exit code 0 after exception is
>> raised:
>>
>> """
>> [root at client1 ~]# ipa-server-install --uninstall -U
>> This server is active DNSSEC key master. Uninstall could break your
>> DNS system.
>> ipa         : ERROR    Server removal aborted: Replica is active
>> DNSSEC key master. Uninstall could break your DNS system. Please
>> disable or replace DNSSEC key master first..
>> [root at client1 ~]# echo $?
>> 0
>> """
>>
>> I guess there is nothing we can do about this now as the fix for this
>> seems to be beyond the scope of this patch ( or am I mistaken?).
>>
> Dandy. The actual relevant ticket to zero return value no matter what is
> already there - https://fedorahosted.org/freeipa/ticket/5725.
>

Ok then. ACK.

Pushed to master: fea56fefff48b0d8eb147c2c2c511c869a1eadf0

-- 
Martin^3 Babinsky