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

2016-08-17 Thread 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@master1 ~]# ipa-server-install --uninstall -U
ipa : ERRORServer removal aborted: Deleting this server
will leave your installation without a DNS..
[root@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@client1 ~]# ipa-server-install --uninstall -U
This server is active DNSSEC key master. Uninstall could break your
DNS system.
ipa : ERRORServer removal aborted: Replica is active
DNSSEC key master. Uninstall could break your DNS system. Please
disable or replace DNSSEC key master first..
[root@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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-08-17 Thread Stanislav Laznicka 

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@master1 ~]# ipa-server-install --uninstall -U
ipa : ERRORServer removal aborted: Deleting this server
will leave your installation without a DNS..
[root@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@client1 ~]# ipa-server-install --uninstall -U
This server is active DNSSEC key master. Uninstall could break your 
DNS system.
ipa : ERRORServer removal aborted: Replica is active 
DNSSEC key master. Uninstall could break your DNS system. Please 
disable or replace DNSSEC key master first..

[root@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.


From e91f2018016f4f2e3acac492bea2199f699fe6e7 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 12 Aug 2016 11:59:41 +0200
Subject: [PATCH] Fail on topology disconnect/last role removal

Disconnecting topology/removing last-role-host during server
uninstallation should raise error rather than just being logged
if the appropriate ignore settings are not present.

https://fedorahosted.org/freeipa/ticket/6168
---
 ipaserver/install/server/install.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 8dc7a6820d214f15fb80fef29c7296fa237bc2a9..48e0b9b7d6ae460c02db3c50b4ce8d401079bd41 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -294,7 +294,6 @@ def common_cleanup(func):
 def remove_master_from_managed_topology(api_instance, options):
 try:
 # we may force the removal
-# if the master was already deleted we will just get a warning
 server_del_options = dict(
 force=True,
 ignore_topology_disconnect=options.ignore_topology_disconnect,
@@ -303,8 +302,10 @@ 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(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))
 
 
-- 
2.7.4

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-08-17 Thread Martin Babinsky 

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@master1 ~]# ipa-server-install --uninstall -U
ipa : ERRORServer removal aborted: Deleting this server
will leave your installation without a DNS..
[root@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))

Regardless of this fix, I still get exit code 0 after exception is raised:

"""
[root@client1 ~]# ipa-server-install --uninstall -U
This server is active DNSSEC key master. Uninstall could break your DNS 
system.
ipa : ERRORServer removal aborted: Replica is active DNSSEC 
key master. Uninstall could break your DNS system. Please disable or 
replace DNSSEC key master first..

[root@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?).


--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-08-16 Thread Stanislav Laznicka 

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@master1 ~]# ipa-server-install --uninstall -U
ipa : ERRORServer removal aborted: Deleting this server 
will leave your installation without a DNS..

[root@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).


From 6f7683fe0b9da6640a740bedf113bb3a9dc99bd1 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 12 Aug 2016 11:59:41 +0200
Subject: [PATCH] Fail on topology disconnect/last role removal

Disconnecting topology/removing last-role-host during server
uninstallation should raise error rather than just being logged
if the appropriate ignore settings are not present.

https://fedorahosted.org/freeipa/ticket/6168
---
 ipaserver/install/server/install.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 8dc7a6820d214f15fb80fef29c7296fa237bc2a9..7a21126ac017dfad89cda9774fbec31c8818f745 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -294,7 +294,6 @@ def common_cleanup(func):
 def remove_master_from_managed_topology(api_instance, options):
 try:
 # we may force the removal
-# if the master was already deleted we will just get a warning
 server_del_options = dict(
 force=True,
 ignore_topology_disconnect=options.ignore_topology_disconnect,
@@ -303,8 +302,10 @@ 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)
 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))
 
 
-- 
2.7.4

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-08-15 Thread Martin Babinsky 

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@master1 ~]# ipa-server-install --uninstall -U
ipa : ERRORServer removal aborted: Deleting this server will 
leave your installation without a DNS..

[root@master1 ~]# echo $?
0
"""

--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-08-15 Thread Martin Babinsky 

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.

--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-08-12 Thread Stanislav Laznicka 

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

From b8099217336af8ed191bed67303d1e52505c2a86 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 12 Aug 2016 11:59:41 +0200
Subject: [PATCH] Fail on topology disconnect/last role removal

Disconnecting topology/removing last-role-host during server
uninstallation should raise error rather than just being logged
if the appropriate ignore settings are not present.

https://fedorahosted.org/freeipa/ticket/6168
---
 ipaserver/install/server/install.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 94698898934844350488d5fc52d6e1e567624502..7aa2590f65fd0dda7f8faf4056bf7cce7032e1b4 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -294,7 +294,6 @@ def common_cleanup(func):
 def remove_master_from_managed_topology(api_instance, options):
 try:
 # we may force the removal
-# if the master was already deleted we will just get a warning
 server_del_options = dict(
 force=True,
 ignore_topology_disconnect=options.ignore_topology_disconnect,
@@ -303,8 +302,10 @@ 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:
+raise
 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))
 
 
-- 
2.7.4

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code