[Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands
Martin Basti
mbasti at redhat.com
Wed Mar 23 15:17:20 UTC 2016
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
More information about the Freeipa-devel
mailing list