Rob Crittenden wrote:
Petr Viktorin wrote:
On 04/10/2013 08:02 PM, Rob Crittenden wrote:
The original design of the LDAP updater was to use numbered update files
which would be applied in order in blocks of 10. We ended up just
applying everything together, sorted by length of the DN.


Why not just sort the files lexicographically, and _run_updates after
each one?

That might work. I did this mostly for schema which can have
interdependencies. I didn't want to force us to have humongous updates
for schema.

I can kind of see the reasoning behind the blocks of ten, but it looks
pretty arbitrary and unnecessarily complex.
It will allow you to create/update parents and children anywhere in the
block of 10 and they'll be sorted properly, but outside of the blocks
you have to watch the ordering. This is pretty confusing; if it's really
needed it should at least be in the README.

It is absolutely arbitrary.

I'll beef up the README.

In practice it probably isn't a big deal WHERE the updates get put, as
long as schema is first. This is just an attempt to force us to be
somewhat organized with things.

This works ok except in the case of roles/privileges/permissions wehre
it is possible that a role is added to a permission  before the role is
created. So the permission has no memberOf attribute and things don't
work as expected.

So this patch implements the by-10 rule and applies the files 10-19,
20-29, etc. I left the ability to run unstructured updates too by
default.

We also need to revert this commit which breaks a test case now that
roles/permissions are created properly,
f7e27b547547be06f511a3ddfaff8db7d0b7898f

\o/


In the README, 10 - 19 should be Schema & configuration.

OK.

While you're at it you can update the FDS Server reference (FDS was
Fedora Directory Server, right?)


Yeah, shows how old this is. I'll fix it.

I updated the README with a bit more information. I dod not update the 10-19 range because it really should just be schema. It isn't that way in the updates currently, but it is what we should strive for.

These are not hard and fast rules, with the exception of schema really, just a recommendation for organization. I can go ahead and move some files around if you really want, but I think it's just shuffling deck chairs.

rob

>From db89012a78cc92d5a9853098766e1d9ade75b1d3 Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcrit...@redhat.com>
Date: Wed, 10 Apr 2013 12:05:29 -0400
Subject: [PATCH] Apply LDAP update files in blocks of 10, as originally
 designed.

In order to have control over the order that updates are applied
a numbering system was created for the update files. These values
were not actually used.

The updates were sorted by DN length and in most cases this was
adequate for proper function. The exception was with roles where
in some cases a role was added as a member of a permission before
the role itself was added so the memberOf value was never created.

Now updates are computed and applied in blocks of 10.

https://fedorahosted.org/freeipa/ticket/3377
---
 install/updates/README                | 23 +++++++++++++++++++----
 ipaserver/install/dsinstance.py       |  2 +-
 ipaserver/install/ipa_ldap_updater.py |  2 +-
 ipaserver/install/ldapupdate.py       | 28 +++++++++++++++++++++++++++-
 ipaserver/install/upgradeinstance.py  |  2 +-
 5 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/install/updates/README b/install/updates/README
index 064c6159fd883be3116cdaf72ed5bd1f5eec0892..175280454cff127fe50d6b82f666f5288f82b17e 100644
--- a/install/updates/README
+++ b/install/updates/README
@@ -2,7 +2,22 @@ The update files are sorted before being processed because there are
 cases where order matters (such as getting schema added first, creating
 parent entries, etc).
 
-10 - 20: Schema
-20 - 30: FDS Configuration, new indices
-30 - 40: Structual elements of the DIT
-40 - 50: Pre-loaded data
+Updates are applied in blocks of ten so that any entries that are dependant
+on another can be added successfully without having to rely on the length
+of the DN to get the sorting correct.
+
+The file names should use the format #-<description>.update where # conforms
+to this:
+
+10 - 19: Schema
+20 - 29: 389-ds configuration, new indices
+30 - 39: Structual elements of the DIT
+40 - 49: Pre-loaded data
+50 - 59: Cleanup existing data
+60 - 69: AD Trust
+70 - 79: Reserved
+80 - 89: Reserved
+
+These numbers aren't absolute, there may be reasons to put an update
+into one place or another, but by adhereing to the scheme it will be
+easier to find existing updates and know where to put new ones.
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 93a226ca982c6d449f242021d4f76fc736abe36c..be629b19898547cd4728e2eea13cf755332d5f83 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -472,7 +472,7 @@ class DsInstance(service.Service):
     def apply_updates(self):
         ld = ldapupdate.LDAPUpdate(dm_password=self.dm_password, sub_dict=self.sub_dict, plugins=True)
         files = ld.get_all_files(ldapupdate.UPDATES_DIR)
-        ld.update(files)
+        ld.update(files, ordered=True)
 
     def __add_referint_module(self):
         self._ldap_mod("referint-conf.ldif")
diff --git a/ipaserver/install/ipa_ldap_updater.py b/ipaserver/install/ipa_ldap_updater.py
index df409ebb6cc796569333406331bd66dc688ed64f..09a1962eca3023f6ebcbff6bda3b778a992e88b4 100644
--- a/ipaserver/install/ipa_ldap_updater.py
+++ b/ipaserver/install/ipa_ldap_updater.py
@@ -185,7 +185,7 @@ class LDAPUpdater_NonUpgrade(LDAPUpdater):
         if not self.files:
             self.files = ld.get_all_files(UPDATES_DIR)
 
-        modified = ld.update(self.files)
+        modified = ld.update(self.files, ordered=True)
 
         if modified and options.test:
             self.log.info('Update complete, changes to be made, test mode')
diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index 79aea1787cbc96d1c5eaa0e5fdfcef6320720b9f..8f3e89249084aadfef27f63b65d6e74ffa0fc0ad 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -33,6 +33,7 @@ import pwd
 import fnmatch
 import csv
 import inspect
+import re
 
 import krbV
 import ldap
@@ -900,21 +901,46 @@ class LDAPUpdate:
         for dn, update in sorted_updates:
             self._delete_record(update)
 
-    def update(self, files):
+    def update(self, files, ordered=False):
         """Execute the update. files is a list of the update files to use.
 
+           If ordered is True then the updates the file must be of the form
+           ##-name.update where ## is an integer between 10 and 89. The
+           changes are applied to LDAP at the end of each value divisible
+           by 10, so after 20, 30, etc.
+
            returns True if anything was changed, otherwise False
         """
 
+        pat = re.compile(r'(\d+)-.*\.update')
         all_updates = {}
+        r = 20
         if self.plugins:
             self.info('PRE_UPDATE')
             updates = api.Backend.updateclient.update(PRE_UPDATE, self.dm_password, self.ldapi, self.live_run)
             self.merge_updates(all_updates, updates)
         try:
             self.create_connection()
+            if ordered and all_updates:
+                # flush out PRE_UPDATE plugin updates before we begin
+                self._run_updates(all_updates)
+                all_updates = {}
 
             for f in files:
+                name = os.path.basename(f)
+                if ordered:
+                    m = pat.match(name)
+                    if not m:
+                        raise RuntimeError("Filename does not match format #-name.update: %s" % f)
+                    index = int(m.group(1))
+                    if index < 10 or index > 90:
+                        raise RuntimeError("Index not legal range: %d" % index)
+
+                    if index >= r:
+                        self._run_updates(all_updates)
+                        all_updates = {}
+                        r += 10
+
                 try:
                     self.info("Parsing update file '%s'" % f)
                     data = self.read_file(f)
diff --git a/ipaserver/install/upgradeinstance.py b/ipaserver/install/upgradeinstance.py
index aa4440c71ea04ce1044cb0cd1379039836b1c2ad..895f29b3dede5542c7316f64978e8b1434b5236c 100644
--- a/ipaserver/install/upgradeinstance.py
+++ b/ipaserver/install/upgradeinstance.py
@@ -115,7 +115,7 @@ class IPAUpgrade(service.Service):
             ld = ldapupdate.LDAPUpdate(dm_password='', ldapi=True, live_run=self.live_run, plugins=True)
             if len(self.files) == 0:
                 self.files = ld.get_all_files(ldapupdate.UPDATES_DIR)
-            self.modified = ld.update(self.files)
+            self.modified = ld.update(self.files, ordered=True)
         except ldapupdate.BadSyntax, e:
             root_logger.error('Bad syntax in upgrade %s' % str(e))
             self.modified = False
-- 
1.8.1

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

Reply via email to