Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Martin Basti]

On 29.01.2016 08:42, Stanislav Laznicka wrote:

On 01/26/2016 06:56 PM, Martin Basti wrote:



On 25.01.2016 16:41, Stanislav Laznicka wrote:

Hi,

Worked those comments into the code. Also added a bit different info 
message in clean_ruv with ca=True (ipa-replica-manage:430).


Also adding stepst to reproduce:
1. Create a master and some replica (3 replicas is a good solution - 
1 with CA, 1 without, 1 to be dangling (with CA))

2. Change domain level to 0 and ipactl restart
3. Remove the "dangling-to-be" replica from masters.ipa.etc and from 
both ipaca and domain subtrees in mapping tree.config

4. Try to remove the dangling ruvs with the command

Cheers,
Standa


On 01/22/2016 01:22 PM, Martin Basti wrote:

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




Hello,

1)
I accept your silence as the following code cannot use nothing from 
api.env

+# 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]))

but then please use:
s = ['dc={dc}'.format(dc=x.lower()) for x in s]
realm_config = DN(('cn', ','.join(s)))

But I still think that api.env.basedn can be used, because it 
contains the same format as you need

realm_config = DN(('cn', api.env.basedn))

Sorry, forgot to mention that in the last email. However, turns out 
you are right. I didn't think DN works like this but it does, so this 
is now in it.

2) nitpick
ca_dn = DN(('cn', 'ca'), DN(master.dn))

AFAIK can be just

ca_dn = DN(('cn', 'ca'), master.dn)


My bad, fixed.

3) uber nitpick
This is PEP8 valid, but somehow inconsistent with the rest of code 
and it hit my eyes


print('\t\tid: {id}, hostname: {host}'
.format(id=csruv[1], host=csruv[0])
)

we use in code

print(
   something1,
   something2
)

or

print(something1,
something2)


And that shouldn't be there anymore, too :)

Otherwise LGTM




ACK

Pushed to:
ipa-4-3: 44018142747e933f7d680c8d7bacfbd883ffddf6
master: c8eabaff9ef49d3f51b02d613c25c09bf155bb3c

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

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Stanislav Laznicka]

On 01/26/2016 06:56 PM, Martin Basti wrote:



On 25.01.2016 16:41, Stanislav Laznicka wrote:

Hi,

Worked those comments into the code. Also added a bit different info 
message in clean_ruv with ca=True (ipa-replica-manage:430).


Also adding stepst to reproduce:
1. Create a master and some replica (3 replicas is a good solution - 
1 with CA, 1 without, 1 to be dangling (with CA))

2. Change domain level to 0 and ipactl restart
3. Remove the "dangling-to-be" replica from masters.ipa.etc and from 
both ipaca and domain subtrees in mapping tree.config

4. Try to remove the dangling ruvs with the command

Cheers,
Standa


On 01/22/2016 01:22 PM, Martin Basti wrote:

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




Hello,

1)
I accept your silence as the following code cannot use nothing from 
api.env

+# 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]))

but then please use:
s = ['dc={dc}'.format(dc=x.lower()) for x in s]
realm_config = DN(('cn', ','.join(s)))

But I still think that api.env.basedn can be used, because it contains 
the same format as you need

realm_config = DN(('cn', api.env.basedn))

Sorry, forgot to mention that in the last email. However, turns out you 
are right. I didn't think DN works like this but it does, so this is now 
in it.

2) nitpick
ca_dn = DN(('cn', 'ca'), DN(master.dn))

AFAIK can be just

ca_dn = DN(('cn', 'ca'), master.dn)


My bad, fixed.

3) uber nitpick
This is PEP8 valid, but somehow inconsistent with the rest of code and 
it hit my eyes


print('\t\tid: {id}, hostname: {host}'
.format(id=csruv[1], host=csruv[0])
)

we use in code

print(
   something1,
   something2
)

or

print(something1,
something2)


And that shouldn't be there anymore, too :)

Otherwise LGTM


From a1421841c88ab233179f175f49000995b2db4acc Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 18 Dec 2015 10:30:44 +0100
Subject: [PATCH 1/2] Listing and cleaning RUV extended for CA suffix

https://fedorahosted.org/freeipa/ticket/5411
---
 install/tools/ipa-replica-manage | 44 ++--
 ipaserver/install/replication.py |  2 +-
 

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Martin Basti]

On 25.01.2016 16:41, Stanislav Laznicka wrote:

Hi,

Worked those comments into the code. Also added a bit different info 
message in clean_ruv with ca=True (ipa-replica-manage:430).


Also adding stepst to reproduce:
1. Create a master and some replica (3 replicas is a good solution - 1 
with CA, 1 without, 1 to be dangling (with CA))

2. Change domain level to 0 and ipactl restart
3. Remove the "dangling-to-be" replica from masters.ipa.etc and from 
both ipaca and domain subtrees in mapping tree.config

4. Try to remove the dangling ruvs with the command

Cheers,
Standa


On 01/22/2016 01:22 PM, Martin Basti wrote:

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




Hello,

1)
I accept your silence as the following code cannot use nothing from api.env
+# 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]))

but then please use:
s = ['dc={dc}'.format(dc=x.lower()) for x in s]
realm_config = DN(('cn', ','.join(s)))

But I still think that api.env.basedn can be used, because it contains 
the same format as you need

realm_config = DN(('cn', api.env.basedn))

2) nitpick
ca_dn = DN(('cn', 'ca'), DN(master.dn))

AFAIK can be just

ca_dn = DN(('cn', 'ca'), master.dn)

3) uber nitpick
This is PEP8 valid, but somehow inconsistent with the rest of code and 
it hit my eyes


print('\t\tid: {id}, hostname: {host}'
.format(id=csruv[1], host=csruv[0])
)

we use in code

print(
   something1,
   something2
)

or

print(something1,
something2)

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

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Stanislav Laznicka]

Hi,

Worked those comments into the code. Also added a bit different info 
message in clean_ruv with ca=True (ipa-replica-manage:430).


Also adding stepst to reproduce:
1. Create a master and some replica (3 replicas is a good solution - 1 
with CA, 1 without, 1 to be dangling (with CA))

2. Change domain level to 0 and ipactl restart
3. Remove the "dangling-to-be" replica from masters.ipa.etc and from 
both ipaca and domain subtrees in mapping tree.config

4. Try to remove the dangling ruvs with the command

Cheers,
Standa


On 01/22/2016 01:22 PM, Martin Basti wrote:

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



From a1421841c88ab233179f175f49000995b2db4acc Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 18 Dec 2015 10:30:44 +0100
Subject: [PATCH 1/2] Listing and cleaning RUV extended for CA suffix

https://fedorahosted.org/freeipa/ticket/5411
---
 install/tools/ipa-replica-manage | 44 ++--
 ipaserver/install/replication.py |  2 +-
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index e4af7b2fd9a40482dfa75d275d528221a1bc22ad..d0a9598985a0c43a25c04ba9a0005eb231052fd1 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -345,7 +345,7 @@ def del_link(realm, replica1, replica2, dirman_passwd, force=False):
 
 return True
 
-def get_ruv(realm, host, dirman_passwd, nolookup=False):
+def get_ruv(realm, host, dirman_passwd, nolookup=False, ca=False):
 """
 Return the RUV entries as a list of tuples: (hostname, rid)
 """
@@ -354,7 +354,10 @@ def get_ruv(realm, host, dirman_passwd, nolookup=False):
 enforce_host_existence(host)
 
 try:
-thisrepl = replication.ReplicationManager(realm, host, dirman_passwd)
+if ca:
+thisrepl = replication.get_cs_replication_manager(realm, host, dirman_passwd)
+else:
+thisrepl = replication.ReplicationManager(realm, host, dirman_passwd)
 except Exception as e:
 print("Failed to connect to server %s: %s" % (host, e))
 sys.exit(1)
@@ -362,7 +365,7 @@ def get_ruv(realm, host, dirman_passwd, nolookup=False):
 

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Martin Basti]

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 

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Stanislav Laznicka]

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.
From 3fe1ec52fb222f4b6e3066e61bfd5e3c0f9b7bd7 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 18 Dec 2015 10:30:44 +0100
Subject: [PATCH 1/2] Listing and cleaning RUV extended for CA suffix

https://fedorahosted.org/freeipa/ticket/5411
---
 install/tools/ipa-replica-manage | 36 +++-
 ipaserver/install/replication.py |  2 +-
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index e4af7b2fd9a40482dfa75d275d528221a1bc22ad..188e2c73a41aa1fd476475f74128b85b7383b09e 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -345,7 +345,7 @@ def del_link(realm, replica1, replica2, dirman_passwd, force=False):
 
 return True
 
-def get_ruv(realm, host, dirman_passwd, nolookup=False):
+def get_ruv(realm, host, dirman_passwd, nolookup=False, ca=False):
 """
 Return the RUV entries as a list of tuples: (hostname, rid)
 """
@@ -354,7 +354,10 @@ def get_ruv(realm, host, dirman_passwd, nolookup=False):
 enforce_host_existence(host)
 
 try:
-thisrepl = replication.ReplicationManager(realm, host, dirman_passwd)
+if ca:
+thisrepl = replication.get_cs_replication_manager(realm, host, dirman_passwd)
+else:
+thisrepl = replication.ReplicationManager(realm, host, dirman_passwd)
 except Exception as e:
 print("Failed to connect to server %s: %s" % (host, e))
 sys.exit(1)
@@ -362,7 +365,7 @@ def get_ruv(realm, host, dirman_passwd, nolookup=False):
 search_filter = '(&(nsuniqueid=---)(objectclass=nstombstone))'
 try:
 entries = thisrepl.conn.get_entries(
-

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Stanislav Laznicka]

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.


From 46ccb4fc174d1cab8b6631e16689a1047b8dbb28 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 18 Dec 2015 10:30:44 +0100
Subject: [PATCH 1/2] Listing and cleaning RUV extended for CA suffix

https://fedorahosted.org/freeipa/ticket/5411
---
 install/tools/ipa-replica-manage | 36 +++-
 ipaserver/install/replication.py |  2 +-
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index e4af7b2fd9a40482dfa75d275d528221a1bc22ad..188e2c73a41aa1fd476475f74128b85b7383b09e 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -345,7 +345,7 @@ def del_link(realm, replica1, replica2, dirman_passwd, force=False):
 
 return True
 
-def get_ruv(realm, host, dirman_passwd, nolookup=False):
+def get_ruv(realm, host, dirman_passwd, nolookup=False, ca=False):
 """
 Return the RUV entries as a list of tuples: (hostname, rid)
 """
@@ -354,7 +354,10 @@ def get_ruv(realm, host, dirman_passwd, nolookup=False):
 enforce_host_existence(host)
 
 try:
-thisrepl = replication.ReplicationManager(realm, host, dirman_passwd)
+if ca:
+thisrepl = replication.get_cs_replication_manager(realm, host, dirman_passwd)
+else:
+thisrepl = replication.ReplicationManager(realm, host, dirman_passwd)
 except Exception as e:
 print("Failed to connect to server %s: %s" % (host, e))
 sys.exit(1)
@@ -362,7 +365,7 @@ def get_ruv(realm, host, dirman_passwd, nolookup=False):
 search_filter = '(&(nsuniqueid=---)(objectclass=nstombstone))'
 try:
 entries = thisrepl.conn.get_entries(
-api.env.basedn, thisrepl.conn.SCOPE_SUBTREE, search_filter,
+thisrepl.db_suffix, thisrepl.conn.SCOPE_SUBTREE, search_filter,
 ['nsds50ruv'])
 except errors.NotFound:
 print("No RUV records found.")
@@ -402,7 +405,7 @@ def get_rid_by_host(realm, sourcehost, host, dirman_passwd, nolookup=False):
 if '%s:389' % host == netloc:
 return int(rid)
 
-def clean_ruv(realm, ruv, options):
+def clean_ruv(realm, ruv, options, ca=False):
 """
 Given an RID create a CLEANALLRUV task to clean it up.
 """
@@ -412,7 +415,7 @@ def clean_ruv(realm, ruv, options):
 sys.exit("Replica ID must be an integer: %s" % ruv)
 
 servers = get_ruv(realm, options.host, options.dirman_passwd,
-  options.nolookup)
+  options.nolookup, ca=ca)
 found = False
 for (netloc, rid) in servers:
 if ruv == int(rid):
@@ -424,14 +427,21 @@ def clean_ruv(realm, ruv, options):
 sys.exit("Replica ID %s not found" % ruv)
 
 print("Clean the Replication Update Vector for %s" % hostname)
-print()
-print("Cleaning the wrong replica ID will cause that server to no")
-print("longer replicate so it may miss updates while the process")
-print("is running. It would need to be re-initialized to maintain")
-print("consistency. Be very careful.")
-if not options.force and not ipautil.user_input("Continue to clean?", False):
-sys.exit("Aborted")
-thisrepl = replication.ReplicationManager(realm, options.host,
+
+if not options.force:
+print()
+print("Cleaning the wrong replica ID will cause that server to no")
+print("longer replicate so it may miss updates while the process")
+print("is running. It would need to be re-initialized to maintain")
+print("consistency. Be very careful.")
+if not ipautil.user_input("Continue to clean?", False):
+sys.exit("Aborted")
+
+if ca:
+thisrepl = replication.get_cs_replication_manager(realm, options.host,
+options.dirman_passwd)
+else:
+thisrepl = replication.ReplicationManager(realm, options.host,
   options.dirman_passwd)
 thisrepl.cleanallruv(ruv)
 print("Cleanup task created")
diff --git a/ipaserver/install/replication.py b/ipaserver/install/replication.py
index 19592e21f32b2013225036b3ce692f6cdee15a73..3221a1bd00bf9375d4348e5ba44d1645f0911b3e 100644
--- a/ipaserver/install/replication.py
+++ 

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Rob Crittenden]

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

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

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Petr Vobornik]

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.
--
Petr Vobornik

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

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Stanislav Laznicka]

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.


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

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Ludwig Krispenz]

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


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

Re: [Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Martin Basti]

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