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 <slazn...@redhat.com>
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):
     search_filter = '(&(nsuniqueid=ffffffff-ffffffff-ffffffff-ffffffff)(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):
@@ -423,16 +426,27 @@ def clean_ruv(realm, ruv, options):
     if not found:
         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,
-                                              options.dirman_passwd)
+    if ca:
+        print("Clean the Certificate Server Replication Update Vector for %s"
+              % hostname)
+    else:
+        print("Clean the Replication Update Vector for %s" % hostname)
+
+    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 49853905f4d61da28e935c00bd931951b3705798..dd9453ce4fdac5d1bc43335fca2d8a96da62ad61 100644
--- a/ipaserver/install/replication.py
+++ b/ipaserver/install/replication.py
@@ -1351,7 +1351,7 @@ class ReplicationManager(object):
             {
                 'objectclass': ['top', 'extensibleObject'],
                 'cn': ['clean %d' % replicaId],
-                'replica-base-dn': [api.env.basedn],
+                'replica-base-dn': [self.db_suffix],
                 'replica-id': [replicaId],
             }
         )
-- 
2.5.0

From 60e1103b4d0e8639cf3a90a7f1e31bffe0043b83 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 18 Dec 2015 10:34:52 +0100
Subject: [PATCH 2/2] Automatically detect and remove dangling RUVs

https://fedorahosted.org/freeipa/ticket/5411
---
 install/tools/ipa-replica-manage       | 168 +++++++++++++++++++++++++++++++++
 install/tools/man/ipa-replica-manage.1 |   3 +
 2 files changed, 171 insertions(+)

diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index d0a9598985a0c43a25c04ba9a0005eb231052fd1..6a50238df78d0a457b75ec35f60e463f9dd5b0ec 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -60,6 +60,7 @@ commands = {
     "clean-ruv":(1, 1, "Replica ID of to clean", "must provide replica ID to clean"),
     "abort-clean-ruv":(1, 1, "Replica ID to abort cleaning", "must provide replica ID to abort cleaning"),
     "list-clean-ruv":(0, 0, "", ""),
+    "clean-dangling-ruv":(0, 0, "", ""),
     "dnarange-show":(0, 1, "[master fqdn]", ""),
     "dnanextrange-show":(0, 1, "", ""),
     "dnarange-set":(2, 2, "<master fqdn> <range>", "must provide a master and ID range"),
@@ -532,6 +533,171 @@ def list_clean_ruv(realm, host, dirman_passwd, verbose, nolookup=False):
                 print(str(dn))
                 print(entry.single_value.get('nstasklog'))
 
+
+def clean_dangling_ruvs(realm, host, options):
+    """
+    Cleans all RUVs and CS-RUVs that are left in the system from
+    uninstalled replicas
+    """
+    # get the Directory Manager password
+    if not options.dirman_passwd:
+        options.dirman_passwd = installutils.read_password('Directory Manager',
+                                                           confirm=False,
+                                                           validate=False,
+                                                           retry=False)
+        if options.dirman_passwd is None:
+            sys.exit('Directory Manager password is required')
+
+    conn = ipaldap.IPAdmin(host, 636, cacert=CACERT)
+    try:
+        conn.do_simple_bind(bindpw=options.dirman_passwd)
+
+        # get all masters
+        masters_dn = DN(api.env.container_masters, api.env.basedn)
+        masters = conn.get_entries(masters_dn, conn.SCOPE_ONELEVEL)
+        info = {}
+
+        # check whether CAs are configured on those masters
+        for master in masters:
+            info[master.single_value['cn']] = {
+                'online': False,       # is the host online?
+                'ca': False,           # does the host have ca configured?
+                'ruvs': set(),         # ruvs on the host
+                'csruvs': set(),       # csruvs on the host
+                'clean_ruv': set(),    # ruvs to be cleaned from the host
+                'clean_csruv': set()   # csruvs to be cleaned from the host
+                }
+            try:
+                ca_dn = DN(('cn', 'ca'), DN(master.dn))
+                conn.get_entry(ca_dn)
+                info[master.single_value['cn']]['ca'] = True
+            except errors.NotFound:
+                continue
+
+    except Exception as e:
+        sys.exit(
+            "Failed to get data from '{host}' while trying to "
+            "list replicas: {error}"
+            .format(host=host, error=e)
+            )
+    finally:
+        conn.unbind()
+
+    # 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]))
+
+    replica_dn = DN(('cn', 'replica'), realm_config,
+                    ('cn', 'mapping tree'), ('cn', 'config'))
+
+    csreplica_dn = DN(('cn', 'replica'), ('cn', 'o=ipaca'),
+                      ('cn', 'mapping tree'), ('cn', 'config'))
+
+    ruvs = set()
+    csruvs = set()
+    offlines = set()
+    for master_cn, master_info in info.items():
+        try:
+            conn = ipaldap.IPAdmin(master_cn, 636, cacert=CACERT)
+            conn.do_simple_bind(bindpw=options.dirman_passwd)
+            master_info['online'] = True
+        except:
+            print("The server '{host}' appears to be offline."
+                  .format(host=master_cn)
+                  )
+            offlines.add(master_cn)
+            continue
+
+        try:
+            entry = conn.get_entry(replica_dn)
+            ruv = (master_cn, entry.single_value.get('nsDS5ReplicaID'))
+            # the check whether ruv is already in ruvs is performed by set type
+            ruvs.add(ruv)
+
+            if(master_info['ca']):
+                entry = conn.get_entry(csreplica_dn)
+                csruv = (master_cn, entry.single_value.get('nsDS5ReplicaID'))
+                csruvs.add(csruv)
+
+            # get_ruv returns server names with :port which needs to be split off
+            ruv_list = get_ruv(realm, master_cn, options.dirman_passwd,
+                               options.nolookup)
+            master_info['ruvs'] = set([
+                (re.sub(':\d+', '', x), y)
+                for (x, y) in ruv_list
+                ])
+
+            if master_info['ca']:
+                ruv_list = get_ruv(realm, master_cn, options.dirman_passwd,
+                                   options.nolookup, ca=True)
+                master_info['csruvs'] = set([
+                    (re.sub(':\d+', '', x), y)
+                    for (x, y) in ruv_list
+                    ])
+        except Exception as e:
+            sys.exit("Failed to obtain information from '{host}': {error}"
+                     .format(host=master_cn, error=str(e)))
+        finally:
+            conn.unbind()
+
+    dangles = False
+    # get the dangling RUVs
+    for master_info in info.values():
+        if master_info['online']:
+            for ruv in master_info['ruvs']:
+                if (ruv not in ruvs) and (ruv[0] not in offlines):
+                    master_info['clean_ruv'].add(ruv)
+                    dangles = True
+
+            # if ca is not configured, there will be no csruvs in master_info
+            for csruv in master_info['csruvs']:
+                if (csruv not in csruvs) and (csruv[0] not in offlines):
+                    master_info['clean_csruv'].add(csruv)
+                    dangles = True
+
+    if not dangles:
+        print('No dangling RUVs found')
+        sys.exit(0)
+
+    print('These RUVs are dangling and will be removed:')
+    for master_cn, master_info in info.items():
+        if master_info['online'] and (master_info['clean_ruv'] or
+                                      master_info['clean_csruv']):
+            print('Host: {m}'.format(m=master_cn))
+            print('\tRUVs:')
+            for ruv in master_info['clean_ruv']:
+                print('\t\tid: {id}, hostname: {host}'
+                      .format(id=ruv[1], host=ruv[0])
+                      )
+
+            print('\tCS-RUVs:')
+            for csruv in master_info['clean_csruv']:
+                print('\t\tid: {id}, hostname: {host}'
+                      .format(id=csruv[1], host=csruv[0])
+                      )
+
+    # TODO: this can be removed when #5396 is fixed
+    if offlines:
+        sys.exit("ERROR: All replicas need to be online to proceed.")
+
+    if not options.force and not ipautil.user_input("Proceed with cleaning?", False):
+        sys.exit("Aborted")
+
+    options.force = True
+    cleaned = set()
+    for master_cn, master_info in info.items():
+        options.host = master_cn
+        for ruv in master_info['clean_ruv']:
+            if ruv[1] not in cleaned:
+                cleaned.add(ruv[1])
+                clean_ruv(realm, ruv[1], options)
+        for csruv in master_info['clean_csruv']:
+            if csruv[1] not in cleaned:
+                cleaned.add(csruv[1])
+                clean_ruv(realm, csruv[1], options, ca=True)
+
+
 def check_last_link(delrepl, realm, dirman_passwd, force):
     """
     We don't want to orphan a server when deleting another one. If you have
@@ -1464,6 +1630,8 @@ def main():
     elif args[0] == "list-clean-ruv":
         list_clean_ruv(realm, host, dirman_passwd, options.verbose,
                        options.nolookup)
+    elif args[0] == "clean-dangling-ruv":
+        clean_dangling_ruvs(realm, host, options)
     elif args[0] == "dnarange-show":
         if len(args) == 2:
             master = args[1]
diff --git a/install/tools/man/ipa-replica-manage.1 b/install/tools/man/ipa-replica-manage.1
index 3ed1c5734e3054501f39ffb4346f05c22361a584..ae109c4c5ff4720eb70b06c6f14a791088696d47 100644
--- a/install/tools/man/ipa-replica-manage.1
+++ b/install/tools/man/ipa-replica-manage.1
@@ -53,6 +53,9 @@ The available commands are:
 \fBclean\-ruv\fR [REPLICATION_ID]
 \- Run the CLEANALLRUV task to remove a replication ID.
 .TP
+\fBclean\-dangling\-ruv\fR
+\- Cleans all RUVs and CS\-RUVs that are left in the system from uninstalled replicas.
+.TP
 \fBabort\-clean\-ruv\fR [REPLICATION_ID]
 \- Abort a running CLEANALLRUV task. With \-\-force option the task does not wait for all the replica servers to have been sent the abort task, or be online, before completing.
 .TP
-- 
2.5.0

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

Reply via email to