[Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs
Martin Basti
mbasti at redhat.com
Fri Jan 22 12:22:47 UTC 2016
On 15.01.2016 13:47, Stanislav Laznicka wrote:
>
> On 01/14/2016 04:59 PM, Petr Vobornik wrote:
>> On 01/14/2016 04:16 PM, Ludwig Krispenz wrote:
>>>
>>> On 01/14/2016 03:59 PM, Stanislav Laznicka wrote:
>>>> On 01/14/2016 03:21 PM, Rob Crittenden wrote:
>>>>> Stanislav Laznicka wrote:
>>>>>> Please see the rebased patches attached.
>>>>>>
>>>>>> On 01/13/2016 02:01 PM, Martin Basti wrote:
>>>>>>>
>>>>>>> On 18.12.2015 12:46, Stanislav Laznicka wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Attached are the patches for auto-find and clean of dangling
>>>>>>>> (cs)ruvs. Currently, the cleaning of an RUV waits for all
>>>>>>>> replicas to
>>>>>>>> be online, even on --force. If that were an issue, I can make the
>>>>>>>> command fail before trying to clean any of RUVs. However, the
>>>>>>>> user is
>>>>>>>> shown a replica is offline and is prompted to confirm the
>>>>>>>> cleaning so
>>>>>>>> the possible wait should not be a problem I believe.
>>>>>>>>
>>>>>>>> Standa L.
>>>>>>>>
>>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> patches needs rebase, I cannot apply them.
>>>>> Will this confuse people? Currently, for good or bad, there are two
>>>>> commands for managing the two different topologies. This mixes
>>>>> some CA
>>>>> work into ipa-replica-manage.
>>>>>
>>>>> rob
>>>>>
>>>> Well, in the patch, I was just following the discussion at
>>>> https://fedorahosted.org/freeipa/ticket/5411. Ludwig mentions that
>>>> ipa-csreplica-manage should go deprecated and does not want to enhance
>>>> it. Also, the only thing the code does is removing trash from the ds
>>>> so it makes sense to me to do it in just one command, as well as the
>>>> users might expect that, too.
>>>>
>>>> I guess it would be possible to add an option that would select which
>>>> of the subtrees should be cleaned of RUVs. It should stay as one
>>>> command nonetheless. Adding such an option for this command would then
>>>> probably mean all the commands should have it as it would make more
>>>> sense, though.
>>>>
>>>> Let me add Petr and Ludwig to CC: as they both had inputs on keeping
>>>> the command in just ipa-replica-manage.
>>> yes, that was the idea to keep ipa-csreplica-manage (which does not
>>> have
>>> clean-ruv,..) for domain-level 0, but not add new features. Also
>>> "ipa-replica-manage del" now triggers the ruv cleaning of ipaca
>>>
>>
>> Yes, ipa-csreplica-manage should be deprecated.
>>
>> I think that one of the reasons why dangling CA RUVs are not uncommon
>> is that users forget about `ipa-csreplica-manage del` command when
>> removing a replica.
>>
>> New `ipa-replica-manage del` also removes replication agreements and
>> therefore cleans RUVs of CA suffix (on domain level 1). In this
>> context it is not inconsistent.
>>
>> Btw, one of the good example why this commands will be helpful is
>> following bz, especially a sentence in:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1295971#c5
>> """
>> I had some mistakes to clean some valid RUV, for example, 52 for eupre1
>> """
>>
>> We should think about list-clean-ruv and abort-clean-ruv commands.
>> There is no counterpart for CA suffix now. Could be in different patch.
>>
>> With clean-dangling-ruvs command it would be good to deprecate
>> clean-ruv command of ipa-replica-manage - should be different patch.
>>
>> I'm not sure if it should abort if some replica is down. Maybe yes
>> until https://fedorahosted.org/freeipa/ticket/5396 is fixed.
>>
>> The path set misses update of man page.
> Attached are the patches with the description for the man page. Abort
> of the clean-dangling-ruv operation on any replica offline status was
> also added.
>
>
Hello,
I have a few comments
PATCH Automatically detect and remove dangling RUVs
1)
+ # get the Directory Manager password
+ if options.dirman_passwd:
+ dirman_passwd = options.dirman_passwd
+ else:
+ dirman_passwd = installutils.read_password('Directory Manager',
+ confirm=False, validate=False, retry=False)
+ if dirman_passwd is None:
+ sys.exit('Directory Manager password is required')
+
+ options.dirman_passwd = dirman_passwd
IMO you need only else branch here
if not options.dirman_password:
dirman_passwd = installutils.read_password('Directory Manager',
confirm=False, validate=False, retry=False)
if dirman_passwd is None:
sys.exit('Directory Manager password is required')
options.dirman_passwd = dirman_passwd
2)
We should use new formatting in new code (more times in code)
+ sys.exit(
+ "Failed to get data from '%s' while trying to list
replicas: %s" %
+ (host, e)
+ )
sys.exit(
"Failed to get data from '{host}' while trying to list
replicas: {e}".format(
host=host, e=e
)
)
3)
+ # get all masters
+ masters_dn = DN(('cn', 'masters'), ('cn', 'ipa'), ('cn', 'etc'),
+ ipautil.realm_to_suffix(realm))
IMO you should use constants:
masters_dn = DN(api.env.container_masters, api.env.basedn)
4)
+ # Get realm string for config tree
+ s = realm.split('.')
+ s = ['dc={dc},'.format(dc=x.lower()) for x in s]
+ realm_config = DN(('cn', ''.join(s)[0:-1]))
Can be api.env.basedn used instead of this block of code?
5)
+ masters = [x.single_value['cn'] for x in masters]
....
+ for master in masters:
is there any reason why not iterate over the keys in info dict?
for master_name, master_data/values/whatever in info.items():
master_data['online'] = True
Looks better than: info[master]['online'] = True
6)
I asked python gurus, for empty lists and dicts, please use [] and {}
instead of list() and dict()
It is preferred and faster.
7)
+ if(info[master]['ca']):
+ entry = conn.get_entry(csreplica_dn)
+ csruv = (master, entry.single_value.get('nsDS5ReplicaID'))
+ if csruv not in csruvs:
+ csruvs.append(csruv)
I dont like too much adding tuples into list and then doing search
there, but it is as designed
However can you use set() instead of list when the purpose of variable
is only testing existence?
related to:
csruvs
ruvs
offlines
clean_list
cleaned
8)
conn in finally block may be undefined
9)
unused local variables
clean_list
entry on line 570
10)
optional, comment what keys means in info structure
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160122/cba15be4/attachment.htm>
More information about the Freeipa-devel
mailing list