[Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands
Martin Basti
mbasti at redhat.com
Wed Mar 23 19:13:40 UTC 2016
----- Original Message -----
From: "Martin Basti" <mbasti at redhat.com>
To: "Stanislav Laznicka" <slaznick at redhat.com>, "freeipa-devel" <freeipa-devel at redhat.com>
Sent: Wednesday, March 23, 2016 4:17:20 PM
Subject: Re: [Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands
On 16.03.2016 14:45, Stanislav Laznicka wrote:
> On 03/16/2016 10:22 AM, Jan Cholasta wrote:
>> On 16.3.2016 08:33, Stanislav Laznicka wrote:
>>> On 03/15/2016 12:47 PM, Petr Vobornik wrote:
>>>> On 03/15/2016 07:25 AM, Jan Cholasta wrote:
>>>>> On 14.3.2016 17:18, Petr Vobornik wrote:
>>>>>> On 03/14/2016 04:55 PM, Jan Cholasta wrote:
>>>>>>> On 14.3.2016 16:26, Petr Vobornik wrote:
>>>>>>>> On 03/14/2016 12:57 PM, Jan Cholasta wrote:
>>>>>>>>> On 14.3.2016 12:50, Martin Basti wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 14.03.2016 12:05, Jan Cholasta wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 11.3.2016 10:39, Stanislav Laznicka wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Please see the patch attached. Contrary to the discussion at
>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4987 I also added the
>>>>>>>>>>>> suffix
>>>>>>>>>>>> option for clean_ruv command. If this command is available for
>>>>>>>>>>>> normal
>>>>>>>>>>>> RUVs, it should probably be available for CS-RUVs as well (or
>>>>>>>>>>>> deprecated
>>>>>>>>>>>> for both with advised use of clean_dangling_ruv).
>>>>>>>>>>>
>>>>>>>>>>> ipa-csreplica-manage is used to manage the CA suffix, so
>>>>>>>>>>> ipa-csreplica-manage should be extended instead of adding
>>>>>>>>>>> --suffix
>>>>>>>>>>> option to ipa-replica-manage. Having half of the CA suffix
>>>>>>>>>>> managed by
>>>>>>>>>>> ipa-replica-manage and the other half by ipa-replica-manage is
>>>>>>>>>>> confusing.
>>>>>>>>>>>
>>>>>>>>>>> Honza
>>>>>>>>>>>
>>>>>>>>>> There is a design document about deprecating
>>>>>>>>>> ipa-csreplica-manage and
>>>>>>>>>> move part of its responsibilities to ipa-replica-manage.
>>>>>>>>>>
>>>>>>>>>> http://www.freeipa.org/page/V4/Manage_replication_topology_4_4#ipa.28cs.29replica_manange_changes
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> So patch is compatible with design.
>>>>>>>>>
>>>>>>>>> The design is wrong then.
>>>>>>>>
>>>>>>>> I don't agree.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Either do it in ipa-csreplica-manage, or make *all*
>>>>>>>>> ipa-replica-manage
>>>>>>>>> sub-commands respect the --suffix option. Anything else is
>>>>>>>>> inconsistent
>>>>>>>>> mess.
>>>>>>>>
>>>>>>>> That's the idea for domain level 1. There is little value in
>>>>>>>> extending
>>>>>>>> behavior(managing replication agreements) in domain level 0.
>>>>>>>
>>>>>>> Domain level 0 is still relevant, it won't go away anytime soon.
>>>>>>>>
>>>>>>>> Main idea is to not care about suffixes and work with all suffixes
>>>>>>>> right
>>>>>>>> away. This is reflected in clean-dangling-ruv command and these
>>>>>>>> extensions are its counterpart - to enable disabling the run. We
>>>>>>>> mostly
>>>>>>>> care about replica IDs not suffixes they belong to. IMO --suffix
>>>>>>>> option
>>>>>>>> is not necessary and is mostly for debugging.
>>>>>>>>
>>>>>>>> One of the reasons why we have all the RUV commands is a mess
>>>>>>>> after
>>>>>>>> uninstallation when somebody forgets/ignores to run
>>>>>>>> `ipa-csreplica-manage del $server` or also `ipa-replica-manage del
>>>>>>>> $server` before uninstallation of replica. Users then usually run
>>>>>>>> `ipa-replica-manage del $server` --force --clean` but
>>>>>>>> `ipa-csreplica-manage del $server` can't be run after it.
>>>>>>>> Changes in
>>>>>>>> 4.3 and 4.4 tries to prevent this situation (e.g. by calling
>>>>>>>> equivalent
>>>>>>>> of `ipa-cs+replica-manage del` from `ipa-server-install
>>>>>>>> --uninstall`).
>>>>>>>> But until then mess is cleaned on all servers, we should deal
>>>>>>>> with it
>>>>>>>> with the most convenient way - hiding implementation details.
>>>>>>>>
>>>>>>>
>>>>>>> This is actually exposing implementation details by forcing the
>>>>>>> user to
>>>>>>> use a different command based on the domain level.
>>>>>>
>>>>>> What different commands?
>>>>>
>>>>> ipa-replica-manage vs ipa-csreplica-manage cs API commands.
>>>>>
>>>>>>
>>>>>>> Please explain to me how any of the above requires us to introduce
>>>>>>> additional inconsistencies and bad UX to IPA.
>>>>>>
>>>>>> What bad UX?
>>>>>
>>>>> This is how replicas are managed in domain level 0 without the patch:
>>>>>
>>>>> suffix both domain ca
>>>>>
>>>>> list - i-r-m i-c-m
>>>>>
>>>>> list-ruv - i-r-m -
>>>>>
>>>>> connect - i-r-m i-c-m
>>>>>
>>>>> diconnect - i-r-m i-c-m
>>>>>
>>>>> del - i-r-m i-c-m
>>>>>
>>>>> re-initialize - i-r-m i-c-m
>>>>>
>>>>> force-sync - i-r-m i-c-m
>>>>>
>>>>> clean-ruv - i-r-m -
>>>>>
>>>>> abort-clean-ruv - i-r-m -
>>>>>
>>>>> list-clean-ruv i-r-m - -
>>>>
>>>> isnt' it?:
>>>> - i-r-m -
>>>>
>>> It is AFAIK.
>>
>> It's not, the command searches all 'cleanallruv' and 'abort
>> cleanallruv' tasks without filtering by suffix.
> My bad, misread it and thought it was list-ruv.
>>
>>>>
>>>>>
>>>>> clean-dangling-ruv i-r-m - -
>>>>>
>>>>> (i-r-m == ipa-replica-manage, etc.)
>>>>>
>>>>>
>>>>> This is how replicas are managed in domain level 1 with the patch:
>>>>>
>>>>> suffix both domain ca
>>>>>
>>>>> list - i-r-m i-c-m
>>>>> s-f s-f -ts=d s-f -ts=c
>>>>>
>>>>> list-ruv i-r-m i-r-m -s=d i-r-m -s=c
>>>>>
>>>>> connect - ts-a d ts-a c
>>>>>
>>>>> diconnect - ts-d d ts-d c
>>>>>
>>>>> del i-r-m - -
>>>>> s-d - -
>>>>>
>>>>> re-initialize - i-r-m i-c-m
>>>>> - ts-r d ts-r c
>>>>>
>>>>> force-sync - i-r-m i-c-m
>>>>>
>>>>> clean-ruv i-r-m i-r-m -s=d i-r-m -s=c
>>>>>
>>>>> abort-clean-ruv i-r-m i-r-m -s=d i-r-m -s=c
>>>>>
>>>>> list-clean-ruv i-r-m - -
>>>>>
>>>>> clean-dangling-ruv i-r-m - -
>>>>>
>>>>> (s-f -ts=d == server-find --topologysuffixes=domain, etc.)
>>>>>
>>>>>
>>>>> Maybe it's just me, but I fail to see the pattern here and find this
>>>>> very confusing. (Note that I'm not trying to blame this particular
>>>>> patch
>>>>> for this, I'm just frustrated from the overall state.)
>>>>
>>>> Yes, backwards compatibility(bc) makes a mess there. But look at the
>>>> state in following way (bc hidden):
>>>>
>>>> suffix both domain ca
>>>>
>>>> == Normal operations (i.e. all in API) ==
>>>>
>>>> list s-f s-f -ts=d s-f -ts=c
>>>>
>>>>
>>>>
>>>> connect - ts-a d ts-a c
>>>>
>>>> diconnect - ts-d d ts-d c
>>>>
>>>> del s-d - -
>>>>
>>>> == Debugging & Fixing ==
>>>>
>>>> re-initialize ts-r d ts-r c
>>>> - i-r-m i-c-m
>>>>
>>>> force-sync - i-r-m i-c-m
>>>>
>>>>
>>>> list-ruv i-r-m
>>>>
>>>> clean-ruv i-r-m
>>>>
>>>> abort-clean-ruv i-r-m
>>>>
>>>> list-clean-ruv i-r-m - -
>>>>
>>>> clean-dangling-ruv i-r-m - -
>>>>
>>>>
>>>> Then we can see that only issue is force-sync operations which use
>>>> case I don't really understand and with re-initialize which should be
>>>> improved in API to be more usable (currently there is no progress
>>>> status).
>>>>
>>>> Note: "debugging and fixing" is basically the same on both domain
>>>> levels.
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> It is supposed to be used in following way:
>>>>>> ipa-replica-manage clean-dangling-ruvs
>>>>>>
>>>>>> If from whatever reason some clean ruv task is not finished then:
>>>>>> ipa-replica-manage list-clean-ruv
>>>>>> [all running task for all suffixes]
>>>>>> ipa-replica-manage abort-clean-ruv REPLICATION_ID
>>>>>>
>>>>>> Nothing else. Works for both domain levels and suffixes from a
>>>>>> single
>>>>>> tool. Again, --suffix option is not important.
>>>>>
>>>>> This changes the default behavior in domain level 0. I though we
>>>>> are not
>>>>> extending domain level 0 anymore, you said it yourself in a comment
>>>>> above.
>>>>
>>>> I meant that we don't need to invest into new features in domain level
>>>> 0 but RUV commands doesn't need to behave differently on various
>>>> domain levels. There is no reason.
>>>>
>>>>>
>>>>>>
>>>>>> Note: clean-ruv subcommand could be probably marked as deprecated
>>>>>> or be
>>>>>> discouraged to use.
>>>>>
>>>>> If the commands are deprecated, why further extend them?
>>>>
>>>> No reason, clean-ruv subcommand doesn't need to be extended. Maybe to
>>>> have similar behavior as rest of ruv commands.
>>>>
>>> It was exactly for that reason. If there's abort-clean-ruv which allows
>>> aborting the clean operation for both suffixes, it seems rather natural
>>> to have its counterpart to be able to do the same (as long as it's not
>>> deprecated, which we might do right now if it seems like a good
>>> thing to
>>> do).
>>>>>
>>>>>>
>>>>>> If the patch doesn't implement it, then it's wrong.
>>>>>
>>>>> The patch changes the default behavior of the sub-commands and
>>>>> extends
>>>>> them even in domain level 0. I would think at least that should be
>>>>> fixed.
>>>>
>>>> Why?
>>>>
>>>>
>>> Given the question of deprecating clean-ruv is answered by now, I
>>> should
>>> also ask why.
>>
>> We discussed this with Petr offline. We agreed that it's actually
>> desirable to make all clean-ruv commands behave the same on all
>> domain levels. We also agreed that it's desirable to make the normal
>> operation commands behave the same on all domain levels, which is
>> currently not true for the connect and disconnect commands, but
>> that's unrelated to this patch.
>>
>> Therefore, I'm OK with the approach, as long as you either remove the
>> --suffix option altogether, or add it to the remaining clean-ruv
>> commands (list-clean-ruv and clean-dangling-ruv). I would personally
>> just remove it, because as Petr pointed out, it's not actually
>> necessary for anything.
>>
> Modified the patch (removed the --suffix option) and added password
> check for clean_dangling_ruv command to be in the same spot as for the
> other commands.
Can you please update design
http://www.freeipa.org/page/V4/Manage_replication_topology_4_4 (mainly
the --suffix option)? Also there are missing clean-ruv and list-ruv
commands in design, and fix usage at the bottom.
1)
I don't understand this expression
+ if dirman_passwd is None or (
+ not dirman_passwd and args[0] in cs_enabled_commands):
You already tested if subcommand belongs to cs_enabled_commands few
lines above, IMO the 'dirman_password is None' expression is enough.
2)
+# tuple of commands that work with ca tree and need Directory Manager
password
+cs_enabled_commands = ("list-ruv", "clean-ruv", "abort-clean-ruv")
this variable is used only toi detect if dirman passwd is needed, I
suggest to rename it to commands_req_dirman_passwd, or something better.
3)
Q: Do we need is_cs_set() function?
A: Yes!
I wanted to give you ultimate NACK, but then I checked how get_ruv code
works and I changed my mind.
Please write a comment where is_cs_set function is used, why we need
extra function instead of catching an exception, possibly you can open a
refactoring ticket.
Shame:
1)
+ if not test_connection(realm, host, options.nolookup) or\
Please use parentheses instead of backslash
2)
+ args[0] in cs_enabled_commands:
+ not dirman_passwd and args[0] in cs_enabled_commands):
Indentation is not multiplication of 4
Nitpicks (I don't insist on fixing these):
1)
+ if servers.get('ca', None):
None is default
2)
+ for (netloc, rid) in servers['ca']:
parentheses are not needed
3)
+ print("\t%s: %s" % (netloc, rid))
Would be nice to use .format() instead of %
Martin^2
I changed my mind, ultimate NACK.
Please fix get_ruv function, is_cs_set will not help. In case there are no RUVs but CA is installed, sys.exit there prevents us from removing RUVs (or any else operation) on domain suffix, and vice versa.
I propose to move ticket to 4.4 milestone because I would like to avoid breaking something in 4.3, as this will hit many places in ipa-replica-manage.
Please provide the refactoring of get_ruv as separate patch a put these patches on top of it.
Martin2
More information about the Freeipa-devel
mailing list