On 22/09/14 14:04, Petr Viktorin wrote:
On 09/01/2014 04:31 PM, Martin Basti wrote:
On 24/07/14 09:06, Martin Basti wrote:
On 23/07/14 15:17, Martin Basti wrote:
This patch fixes ordering problem of schema updates

Martin should it be in IPA 4.0.x ? It requires rebased ldap_python
(will be in Fedora 21)

Patch attached


I found a bug there, but before I send updated version, I need to
decide how print modlist:

1. Print modlist before each LDAP update (if changes exist), show
modlist per incremental update

2. Print modlist only at once with all updates

3. Use [1] for live_run, [2] for test

Print modlis before each LDAP update
Updated patch attached.

Thanks! This works nicely, just some comments:

The required python-ldap is quite new, not even built in Koji yet. We need to get it to the IPA COPR repo before this is merged.


The _get_oid_dependency_order function is not indented well.

It would be nice if it was a bit more obvious that the loop in _get_oid_dependency_order is guaranteed to terminate. Could you assert that len(unordered_oids) decreases in each iteration?

+SCHEMA_ELEMENT_CLASSES_KEYS = (x[0] for x in SCHEMA_ELEMENT_CLASSES)

You're creating a generator here, which is only good for one use. Use a list instead. Also, this is used just once so it doesn't need to be a module-level constant.

+ # FIXME: We should have a better way to display the modlist, + # for now display raw output of our internal routine

Not entirely related to your change, but you can drop this comment. Since 61887ac we don't use an internal routine.


Thank you!
updated patch attached.

--
Martin Basti

From 996992efff178b8bd400dcbfe6ab6203b14beefc Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Wed, 23 Jul 2014 14:42:33 +0200
Subject: [PATCH] FIX: ldap schmema updater needs correct ordering of the
 updates

Required bugfix in python-ldap 2.4.15

Updates must respect SUP objectclasses/attributes and update
dependencies first
---
 freeipa.spec.in                   |   2 +-
 ipaserver/install/schemaupdate.py | 125 ++++++++++++++++++++++++++------------
 2 files changed, 87 insertions(+), 40 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index c92c245776c83bb86c78575e0f88bc62b849a831..d692e8c3d054be144298696a9f1a383a1f5d60d0 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -99,7 +99,7 @@ Requires: httpd >= 2.4.6-6
 Requires: mod_wsgi
 Requires: mod_auth_kerb >= 5.4-16
 Requires: mod_nss >= 1.0.8-26
-Requires: python-ldap
+Requires: python-ldap >= 2.4.15
 Requires: python-krbV
 Requires: acl
 Requires: python-pyasn1
diff --git a/ipaserver/install/schemaupdate.py b/ipaserver/install/schemaupdate.py
index bb2f0f161499c0358452d97f27f5c39b9b64a5ad..eaa45d5e947803bb339b9462df666f58f06e0a2d 100644
--- a/ipaserver/install/schemaupdate.py
+++ b/ipaserver/install/schemaupdate.py
@@ -29,17 +29,60 @@ from ipaserver.install.ldapupdate import connect
 from ipaserver.install import installutils
 
 
-SCHEMA_ELEMENT_CLASSES = {
+SCHEMA_ELEMENT_CLASSES = (
     # All schema model classes this tool can modify
-    'objectclasses': ldap.schema.models.ObjectClass,
-    'attributetypes': ldap.schema.models.AttributeType,
-}
+    # Depends on order, attributes first, then objectclasses
+    ('attributetypes', ldap.schema.models.AttributeType),
+    ('objectclasses', ldap.schema.models.ObjectClass),
+)
 
 ORIGIN = 'IPA v%s' % ipapython.version.VERSION
 
 log = log_mgr.get_logger(__name__)
 
 
+def _get_oid_dependency_order(schema, cls):
+    """
+    Returns a ordered list of OIDs sets, in order which respects inheritance in LDAP
+    OIDs in second set, depend on first set, etc.
+
+    :return [set(1st-tree-level), set(2nd-tree-level), ...]
+    """
+    top_node = '_'
+    ordered_oid_groups = []
+
+    tree = schema.tree(cls)  # tree structure of schema
+
+    # remove top_node from tree, it breaks ordering
+    # we don't need this, tree from file is not consistent
+    del tree[top_node]
+    unordered_oids = tree.keys()
+
+    # split into two groups, parents and child nodes, and iterate until
+    # child nodes are not empty
+    while unordered_oids:
+        parent_nodes = set()
+        child_nodes = set()
+
+        for node in unordered_oids:
+            if node not in child_nodes:
+                # if node was child once, must remain as child
+                parent_nodes.add(node)
+
+            for child_oid in tree[node]:
+                # iterate over all child nodes stored in tree[node] per node
+                # child node must be removed from parents
+                parent_nodes.discard(child_oid)
+                child_nodes.add(child_oid)
+
+        ordered_oid_groups.append(parent_nodes)  # parents nodes are not dependent
+
+        assert len(child_nodes) < len(unordered_oids)  # while iteration must be finite
+        unordered_oids = child_nodes  # extract new parent nodes in next iteration
+
+    return ordered_oid_groups
+
+
 def update_schema(schema_files, ldapi=False, dm_password=None, live_run=True):
     """Update schema to match the given ldif files
 
@@ -62,64 +105,68 @@ def update_schema(schema_files, ldapi=False, dm_password=None, live_run=True):
         True if modifications were made
         (or *would be* made, for live_run=false)
     """
+    SCHEMA_ELEMENT_CLASSES_KEYS = [x[0] for x in SCHEMA_ELEMENT_CLASSES]
+
     conn = connect(ldapi=ldapi, dm_password=dm_password,
                    realm=krbV.default_context().default_realm,
                    fqdn=installutils.get_fqdn())
 
     old_schema = conn.schema
 
+
     schema_entry = conn.get_entry(DN(('cn', 'schema')),
-                                  SCHEMA_ELEMENT_CLASSES.keys())
+                                  SCHEMA_ELEMENT_CLASSES_KEYS)
 
     modified = False
 
     # The exact representation the DS gives us for each OID
     # (for debug logging)
     old_entries_by_oid = {cls(str(attr)).oid: str(attr)
-                          for attrname, cls in SCHEMA_ELEMENT_CLASSES.items()
+                          for (attrname, cls) in SCHEMA_ELEMENT_CLASSES
                           for attr in schema_entry[attrname]}
 
     for filename in schema_files:
         log.info('Processing schema LDIF file %s', filename)
         dn, new_schema = ldap.schema.subentry.urlfetch(filename)
 
-        for attrname, cls in SCHEMA_ELEMENT_CLASSES.items():
+        for attrname, cls in SCHEMA_ELEMENT_CLASSES:
+            for oids_set in _get_oid_dependency_order(new_schema, cls):
+                # Set of all elements of this class, as strings given by the DS
+                new_elements = []
+                for oid in oids_set:
+                    new_obj = new_schema.get_obj(cls, oid)
+                    old_obj = old_schema.get_obj(cls, oid)
+                    # Compare python-ldap's sanitized string representations
+                    # to see if the value is different
+                    # This can give false positives, e.g. with case differences
+                    # in case-insensitive names.
+                    # But, false positives are harmless (and infrequent)
+                    if not old_obj or str(new_obj) != str(old_obj):
+                        # Note: An add will automatically replace any existing
+                        # schema with the same OID. So, we only add.
+                        value = add_x_origin(new_obj)
+                        new_elements.append(value)
 
-            # Set of all elements of this class, as strings given by the DS
-            new_elements = []
+                        if old_obj:
+                            old_attr = old_entries_by_oid.get(oid)
+                            log.info('Replace: %s', old_attr)
+                            log.info('   with: %s', value)
+                        else:
+                            log.info('Add: %s', value)
 
-            for oid in new_schema.listall(cls):
-                new_obj = new_schema.get_obj(cls, oid)
-                old_obj = old_schema.get_obj(cls, oid)
-                # Compare python-ldap's sanitized string representations
-                # to see if the value is different
-                # This can give false positives, e.g. with case differences
-                # in case-insensitive names.
-                # But, false positives are harmless (and infrequent)
-                if not old_obj or str(new_obj) != str(old_obj):
-                    # Note: An add will automatically replace any existing
-                    # schema with the same OID. So, we only add.
-                    value = add_x_origin(new_obj)
-                    new_elements.append(value)
+                modified = modified or new_elements
+                schema_entry[attrname].extend(new_elements)
+                # we need to iterate schema updates, due to dependencies (SUP)
+                # schema_entry doesn't respect order of objectclasses/attributes
+                # so updates must be executed with groups of independent OIDs
+                if new_elements:
+                    modlist = schema_entry.generate_modlist()
+                    log.debug("Schema modlist:\n%s", pprint.pformat(modlist))
 
-                    if old_obj:
-                        old_attr = old_entries_by_oid.get(oid)
-                        log.info('Replace: %s', old_attr)
-                        log.info('   with: %s', value)
-                    else:
-                        log.info('Add: %s', value)
+                    if live_run:
+                        conn.update_entry(schema_entry)
 
-            modified = modified or new_elements
-            schema_entry[attrname].extend(new_elements)
-
-    # FIXME: We should have a better way to display the modlist,
-    # for now display raw output of our internal routine
-    modlist = schema_entry.generate_modlist()
-    log.debug("Complete schema modlist:\n%s", pprint.pformat(modlist))
-
-    if modified and live_run:
-        conn.update_entry(schema_entry)
-    else:
+    if not (modified and live_run):
         log.info('Not updating schema')
 
     return modified
-- 
1.8.3.1

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to