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