On 02/23/2016 07:43 AM, Fraser Tweedale wrote:
On Tue, Feb 23, 2016 at 07:32:31AM +0100, Jan Cholasta wrote:
On 23.2.2016 06:40, Fraser Tweedale wrote:
On Mon, Feb 22, 2016 at 02:03:49PM +0100, Martin Babinsky wrote:
https://fedorahosted.org/freeipa/ticket/5682

--
Martin^3 Babinsky

Thanks for the patch.  Conditional ACK.

Patch is tested and works, but I am wary about checking for
substring match against RemoteRetrieveError reason string (see hunk
below).  It would be better to carry the status code as an attribute
of RemoteRetrieveError and check whether it is 409.

+        except errors.RemoteRetrieveError as e:
+            if "Profile already exists" in e.reason:
+                root_logger.debug("Profile '{}' already present".format(
+                    profile_id))
+            else:
+                root_logger.error("Error migrating profile '{}': {}".format(
+                    profile_id, e))
+                return

If you agree, we can file a ticket and I am happy for these patches
to be merged as-is.  The scope of changing RemoteRetrieveError is
larger than #5682 so it makes sense to do it separately, and just
for master branch.

I don't think this is the right approach. create_profile() should raise
DuplicateEntry rather than RemoteRetrieveError if the profile already
exists, which can then be properly handled in _create_dogtag_profile().

I am happy with that.


Attaching updated patch. Unfortunately, REST API does report only a super-generic error code 400: Bad request when adding profile which already exists, which is not very useful for us (I will open a ticket for Dogtag for this).

After consultation with Jan, we decided to log the error at debug level and be done with it (for the moment).

--
Martin^3 Babinsky
From e75576971f9639825e92b234b688cb56e09cb707 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Mon, 22 Feb 2016 13:35:41 +0100
Subject: [PATCH] upgrade: unconditional import of certificate profiles into
 LDAP

During IPA server upgrade, the migration of Dogtag profiles into LDAP
backend was bound to the update of CS.cfg which enabled the LDAP profile
subsystem. If the subsequent profile migration failed, the subsequent
upgrades were not executing the migration code leaving CA subsystem in
broken state. Therefore the migration code path should be executed
regardless of the status of the main Dogtag config file.

https://fedorahosted.org/freeipa/ticket/5682
---
 ipaserver/install/cainstance.py     | 8 ++++++--
 ipaserver/install/server/upgrade.py | 4 +++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 369902ad04b197c9e9516503c1f81c4de1ef153b..1a98c438786ae7dad208212fff23e3a760c95b3c 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -1807,7 +1807,6 @@ def migrate_profiles_to_ldap(dogtag_constants):
             continue
         class_id = match.group(1)
 
-        root_logger.info("Migrating profile '%s' to LDAP", profile_id)
         with open(filename) as f:
             profile_data = f.read()
             if profile_data[-1] != '\n':
@@ -1824,7 +1823,12 @@ def _create_dogtag_profile(profile_id, profile_data):
         # import the profile
         try:
             profile_api.create_profile(profile_data)
-        except errors.RemoteRetrieveError:
+            root_logger.info("Profile '%s' successfully migrated to LDAP",
+                             profile_id)
+        except errors.RemoteRetrieveError as e:
+            root_logger.debug("Error migrating '{}': {}".format(
+                profile_id, e))
+
             # conflicting profile; replace it if we are
             # installing IPA, but keep it for upgrades
             if api.env.context == 'installer':
diff --git a/ipaserver/install/server/upgrade.py b/ipaserver/install/server/upgrade.py
index 0a46635979497f8028465c2295b22485fd9c0279..258d976c83844f89c1a939303b685fd6565b79e5 100644
--- a/ipaserver/install/server/upgrade.py
+++ b/ipaserver/install/server/upgrade.py
@@ -336,7 +336,9 @@ def ca_enable_ldap_profile_subsystem(ca):
             separator='=')
 
         ca.restart(dogtag.configured_constants().PKI_INSTANCE_NAME)
-        cainstance.migrate_profiles_to_ldap(caconfig)
+
+    root_logger.info('[Migrating certificate profiles to LDAP]')
+    cainstance.migrate_profiles_to_ldap(caconfig)
 
     return needs_update
 
-- 
2.5.0

From 148d44fc744c92fc13ce0ac3544e924c7099ff05 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Mon, 22 Feb 2016 13:35:41 +0100
Subject: [PATCH] upgrade: unconditional import of certificate profiles into
 LDAP

During IPA server upgrade, the migration of Dogtag profiles into LDAP
backend was bound to the update of CS.cfg which enabled the LDAP profile
subsystem. If the subsequent profile migration failed, the subsequent
upgrades were not executing the migration code leaving CA subsystem in
broken state. Therefore the migration code path should be executed
regardless of the status of the main Dogtag config file.

https://fedorahosted.org/freeipa/ticket/5682
---
 ipaserver/install/cainstance.py     | 8 ++++++--
 ipaserver/install/server/upgrade.py | 4 +++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index f3c1bfa361f2627d8e95ad6cb2fa93b4dc41ee38..b72255f1ad2f95c5265a40eddfb1fe413826dba4 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -1707,7 +1707,6 @@ def migrate_profiles_to_ldap():
             continue
         class_id = match.group(1)
 
-        root_logger.info("Migrating profile '%s' to LDAP", profile_id)
         with open(filename) as f:
             profile_data = f.read()
             if profile_data[-1] != '\n':
@@ -1724,7 +1723,12 @@ def _create_dogtag_profile(profile_id, profile_data):
         # import the profile
         try:
             profile_api.create_profile(profile_data)
-        except errors.RemoteRetrieveError:
+            root_logger.info("Profile '%s' successfully migrated to LDAP",
+                             profile_id)
+        except errors.RemoteRetrieveError as e:
+            root_logger.debug("Error migrating '{}': {}".format(
+                profile_id, e))
+
             # conflicting profile; replace it if we are
             # installing IPA, but keep it for upgrades
             if api.env.context == 'installer':
diff --git a/ipaserver/install/server/upgrade.py b/ipaserver/install/server/upgrade.py
index d1bb346360c98844cac9b21a1a19c5883d68757b..2ec089d77c3fad4c934c4571707afe7681ddee5c 100644
--- a/ipaserver/install/server/upgrade.py
+++ b/ipaserver/install/server/upgrade.py
@@ -333,7 +333,9 @@ def ca_enable_ldap_profile_subsystem(ca):
             separator='=')
 
         ca.restart('pki-tomcat')
-        cainstance.migrate_profiles_to_ldap()
+
+    root_logger.info('[Migrating certificate profiles to LDAP]')
+    cainstance.migrate_profiles_to_ldap()
 
     return needs_update
 
-- 
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