On 03/30/2016 02:32 PM, Petr Vobornik wrote:
On 03/30/2016 02:04 PM, Martin Babinsky wrote:
On 03/17/2016 04:15 PM, Petr Vobornik wrote:
On 03/17/2016 04:10 PM, Martin Babinsky wrote:
On 03/17/2016 03:37 PM, Petr Vobornik wrote:
On 03/17/2016 03:17 PM, Martin Babinsky wrote:
On 03/17/2016 02:55 PM, Petr Vobornik wrote:
On 03/17/2016 02:39 PM, Martin Babinsky wrote:
Hi list,

I would like to discuss the merge of `del_master_managed()`
function
from `ipa-replica-manage` command into the server_del API call that
is a
part of the managed replication topology design update[1] (see also
the
corresponding upstream ticket [2]).

Before I head down into coding I want to be sure that everyone is
one
the same page regarding the expected use-cases which govern the API
design.

IIUC, there are two main uses of the new functionality according to
design document:

1.) run 'server_del' when 'ipa-replica-manage del' is run in
domain-level 1

Right, this is for backwards compatibility(BC).


2.) during 'ipa-server-install --uninstall', 'server_del' should be
called on one of remote masters to remove the uninstalled server
from
the managed topology

What I didn't get from the design document is whether the method
should
have some kind of 'force' option which should bypass all topology
connectivity checks. Currently both `ipa-replica-manage del` and
server
uninstaller have options which will force the removal even if it
disconnects the topology ('--force' in the former,
'--ignore-disconnected-topology' in the latter).

I would say that uninstaller should do checks in validate method
therefore the subsequent `server-del` doesn't need to do it again
but it
shouldn't harm. I.e. it should follow what the user specified. If
user
wants to skip (--ignore-d..-t..) then skip. If not then it will
fail in
validate method.

Only issue might be error state where servers have different
picture of
the topology.

If the view of the topology is not self-consistent then you have
plenty
of other issues to take care of and that may include some forced
removal
and recreation of nodes.


I guess the 'server_del' method should inherit this flag so that we
retain the original functionality (for better or worse). I
propose to
name this option 'ignore_topology_disconnect' because it is more
descriptive than plain 'force'.

+1

And in BC case, `ipa-replica-manage --force` would call `server-del
--ig..-d..-t...`

Yes.

I would also like to ask whether 'server_del' (which is currently
NO_CLI) should be usable also from command line.

IMO yes, it should mostly as a couterpart of `ipa-replica-manage
--force
--clean`

Which bring us to --clean option and what it should do...

According to the design, '--clean' should be used as a cleanup of
leftovers after deleted servers. How I image it from the
implementation
point of view is that when '--clean' is specified and the server was
already deleted, the NotFound error raised from the framework
should be
ignored and the code should continue in clean up. (I assume that
segment/service/dns cleanup will be done in post_callback portion and
the topology connectivity/sanity checks in the pre_callback).

When thinking about it, clean could be a separate command which
would be
called internally in post callback of server-del. It would reduce the
number of ifs in server-del and simplify it in general. It would work
only if server entry doesn't exists.

That was my original idea. I also thought that
'check_last_link_managed'
could be a separate command, but it is probably not a very good idea to
add the overhead of calling two separate commands to a single API call.
OTOH it would improve the code organization IMHO.

Not sure if check_last_link_managed should be an API command. It is
already a separate function. What would be the use case for it as a
command?

Maybe the function should be moved from ipaserver/install/replication to
a more suitable place given that it's used from API - it's up to you.


That means that '--clean' has no additional effect when the server
exists.

Right



[1] http://www.freeipa.org/page/V4/Manage_replication_topology_4_4
[2] https://fedorahosted.org/freeipa/ticket/5588




There are two more things I would like to clarify before I can finish
the patch:

1.) We have discussed offline that the ensure_last_services function[1]
shall be reimplemented in the API command using server roles (I will
open a ticket for this when time comes). Currently the code works
interactively, prompting the user about removing last DNS/CA/etc server.
The behavior is overriden by --force option.

I guess we do not want to have interactive prompting in the API command,
so we will have to handle this somehow. A proposal would be to by
default abort removal of last DNS/CA server and add a '--force' option
which will override this check and also override the disconnected
topology check (IMHO it is not of much use to keep both options in this
case). What do you think?

In general +1.

* move services automatically but fail if it is the last
* have override param to force it. It can be mapped to `ipa server-del
--force`. Or maybe other name than --force (too easy to use, very general).


2.) Removal of DNS entries is handled by directly calling
bindinstance/dnskeysyncinstance code[2]. Obviously this is not very
desirable in the context of API code since we would then need to
conditionally import these modules on server side (one option but not
very nice IMHO). Should I reimplement this code using API commands or
move the relevant bits into ipapython/ipalib?

Both methods (ensure_last_services, removal of DNS records) should be
moved. There is no need for separate API calls.

But we might want to consider other option if simple move is not so simple.

well in both ways the move requires a conditional import of server-side modules like this so that it does not break clients:
"""
if api.env.in_server:
    from ipaserver.install import bindinstance, dnskeysyncinstance
    # do magic
"""

which will make people like Jan Cholasta cringe mightily.

I will try to move the code around so that we can avoid these hacks.



[1]
https://git.fedorahosted.org/cgit/freeipa.git/tree/install/tools/ipa-replica-manage?h=ipa-4-3#n753



[2]
https://git.fedorahosted.org/cgit/freeipa.git/tree/install/tools/ipa-replica-manage?h=ipa-4-3#n810







--
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

Reply via email to