On 18.11.2015 14:26, Martin Basti wrote:


On 18.11.2015 14:24, Martin Kosek wrote:
On 11/18/2015 02:18 PM, Martin Basti wrote:

On 18.11.2015 13:55, Martin Kosek wrote:
On 11/18/2015 01:30 PM, Martin Basti wrote:
+    DEFAULT_TIME_LIMIT = -1.0
+    DEFAULT_SIZE_LIMIT = 0
...
           if time_limit is None or time_limit == 0:
-            time_limit = -1.0
+            if self.time_limit is not None and self.time_limit != 0:
+                time_limit = self.time_limit
+            else:
+                time_limit = self.DEFAULT_TIME_LIMIT
+
I wonder why is the -1 default time limit a float number, I would expect that
some trouble may emerge out of it. Maybe Rob knows?

Python LDAP allows to have smaller granularity than seconds and it internally
convert time limit values to float.
Hm, ok. I was just curious since the value we expose in API is Int and the default does not use the smaller granularity, so I was thinking "Why bother?". If it is fixed in your patch, good. If not, good also, no need to bikeshed here
I suppose.
I just keep the original values there, I do not want to touch it with this patch

Updated patch attached, the original one has wrong default limit for ldap2
From 770e1bec7a95fa63f910213a5611ba9bccf18547 Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Wed, 18 Nov 2015 10:31:05 +0100
Subject: [PATCH] Upgrade: increase time limit for upgrades

Default ldap search limit is now 30s by default during upgrade.

This value is configurable via default.conf file, option
'upgrade_ldapsearch_limit'.

Limits must be changed for the whole ldap2 connection, because this
connection is used inside update plugins and commands called from
upgrade.

Together with increasing the time limit, also size limit should be
unlimited during upgrade. With sizelimit=None we may get the
TimeExceeded exception from getting default value of the sizelimit from LDAP.

https://fedorahosted.org/freeipa/ticket/5267
---
 ipalib/constants.py             |  3 +++
 ipapython/ipaldap.py            | 35 ++++++++++++++++++++++++++++++-----
 ipaserver/install/ldapupdate.py |  7 +++++++
 ipaserver/plugins/ldap2.py      | 34 +++++++++++++++++++++++++---------
 4 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/ipalib/constants.py b/ipalib/constants.py
index fc0560ba4fe746f11e8ff3175508ace2e50c3187..15be3ff865cdb0e6a01570144c9fdcc4c6909dac 100644
--- a/ipalib/constants.py
+++ b/ipalib/constants.py
@@ -189,6 +189,9 @@ DEFAULT_CONFIG = (
     # behavior when newer clients talk to older servers. Use with caution.
     ('skip_version_check', False),
 
+    # Upgrade
+    ('upgrade_ldapsearch_limit', 30),  # limit in seconds
+
     # ********************************************************
     #  The remaining keys are never set from the values here!
     # ********************************************************
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 7e5bc04fea1dc89cc141051025d00a57d74b2b41..7b70894b27b5c32ed05251cf3bc6d1824c88010e 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -689,8 +689,12 @@ class LDAPClient(object):
         'nsslapd-minssf-exclude-rootdse': True,
     })
 
+    DEFAULT_TIME_LIMIT = -1.0
+    DEFAULT_SIZE_LIMIT = 0
+
     def __init__(self, ldap_uri, start_tls=False, force_schema_updates=False,
-                 no_schema=False, decode_attrs=True):
+                 no_schema=False, decode_attrs=True, time_limit=None,
+                 size_limit=None):
         """Create LDAPClient object.
 
         :param ldap_uri: The LDAP URI to connect to
@@ -706,12 +710,18 @@ class LDAPClient(object):
         :param decode_attrs:
             If true, attributes are decoded to Python types according to their
             syntax.
+        :param time_limit: time limit for ldap search that is used for whole
+            connection
+        :param size_limit: size limit for ldap seach that is used for whole
+            connection
         """
         self.ldap_uri = ldap_uri
         self._start_tls = start_tls
         self._force_schema_updates = force_schema_updates
         self._no_schema = no_schema
         self._decode_attrs = decode_attrs
+        self.time_limit = time_limit
+        self.size_limit = size_limit
 
         self.log = log_mgr.get_logger(self)
         self._has_schema = False
@@ -1283,6 +1293,11 @@ class LDAPClient(object):
             (default skips these entries)
         paged_search -- search using paged results control
 
+        size_limit and time_limit priority:
+            1. use locally specified limits (if specified)
+            2. use limits specified in __init__ (if specified)
+            3. use default limits
+
         :raises: errors.NotFound if result set is empty
                                  or base_dn doesn't exist
         """
@@ -1295,9 +1310,17 @@ class LDAPClient(object):
         truncated = False
 
         if time_limit is None or time_limit == 0:
-            time_limit = -1.0
+            if self.time_limit is not None and self.time_limit != 0:
+                time_limit = self.time_limit
+            else:
+                time_limit = self.DEFAULT_TIME_LIMIT
+
         if size_limit is None:
-            size_limit = 0
+            if self.size_limit is not None:
+                size_limit = self.size_limit
+            else:
+                size_limit = self.DEFAULT_SIZE_LIMIT
+
         if not isinstance(size_limit, int):
             size_limit = int(size_limit)
         if not isinstance(time_limit, float):
@@ -1544,7 +1567,8 @@ class IPAdmin(LDAPClient):
     def __init__(self, host='', port=389, cacert=None, debug=None, ldapi=False,
                  realm=None, protocol=None, force_schema_updates=True,
                  start_tls=False, ldap_uri=None, no_schema=False,
-                 decode_attrs=True, sasl_nocanon=False, demand_cert=False):
+                 decode_attrs=True, sasl_nocanon=False, demand_cert=False,
+                 time_limit=None, size_limit=None):
         self._conn = None
         log_mgr.get_logger(self, True)
         if debug and debug.lower() == "on":
@@ -1564,7 +1588,8 @@ class IPAdmin(LDAPClient):
 
         super(IPAdmin, self).__init__(
             ldap_uri, force_schema_updates=force_schema_updates,
-            no_schema=no_schema, decode_attrs=decode_attrs)
+            no_schema=no_schema, decode_attrs=decode_attrs,
+            time_limit=time_limit, size_limit=size_limit)
 
         with self.error_handler():
             if demand_cert:
diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index 86c01104710ff166a07057f9873ed8661621e704..21d53292a71ec517c70924a68d672fa59b7aa525 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -315,6 +315,7 @@ class LDAPUpdate:
         self.api = create_api(mode=None)
         self.api.bootstrap(in_server=True, context='updates')
         self.api.finalize()
+        self.time_limit = self.api.env.upgrade_ldapsearch_limit
         if online:
             # Try out the connection/password
             # (This will raise if the server is not available)
@@ -869,6 +870,12 @@ class LDAPUpdate:
                 bind_pw=self.dm_password,
                 autobind=self.ldapi)
             self.conn = self.api.Backend.ldap2
+
+            # set time limit and size limit parameters for upgrade
+            # size limit must be 0, we need update all entries
+            self.conn.time_limit = self.time_limit
+            self.conn.size_limit = 0
+
         else:
             raise RuntimeError("Offline updates are not supported.")
 
diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py
index ca11efd5e3b4608890edb266573b2586e79cbc46..7614a3a5585c8469ab95098446331f6185226e82 100644
--- a/ipaserver/plugins/ldap2.py
+++ b/ipaserver/plugins/ldap2.py
@@ -62,7 +62,9 @@ class ldap2(CrudBackend, LDAPClient):
     LDAP Backend Take 2.
     """
 
-    def __init__(self, api, ldap_uri=None):
+    DEFAULT_TIME_LIMIT = 2.0
+
+    def __init__(self, api, ldap_uri=None, time_limit=None, size_limit=None):
         if ldap_uri is None:
             ldap_uri = api.env.ldap_uri
 
@@ -70,7 +72,8 @@ class ldap2(CrudBackend, LDAPClient):
 
         CrudBackend.__init__(self, api)
         LDAPClient.__init__(self, ldap_uri,
-                            force_schema_updates=force_schema_updates)
+                            force_schema_updates=force_schema_updates,
+                            time_limit=time_limit, size_limit=size_limit)
 
     def _connect(self):
         # Connectible.conn is a proxy to thread-local storage;
@@ -178,7 +181,13 @@ class ldap2(CrudBackend, LDAPClient):
     def find_entries(self, filter=None, attrs_list=None, base_dn=None,
                      scope=_ldap.SCOPE_SUBTREE, time_limit=None,
                      size_limit=None, search_refs=False, paged_search=False):
-
+        """
+        Priority of time limit and size limit:
+            1. use locally specified limits (if specified)
+            2. use limits specified in __init__ (if specified)
+            3. use limits specified in ipa config (if specified)
+            4. use default limits
+        """
         def _get_limits():
             """Get configured global limits, caching them for more calls"""
             if not _lims:
@@ -189,9 +198,15 @@ class ldap2(CrudBackend, LDAPClient):
         _lims = {}
 
         if time_limit is None:
-            time_limit = _get_limits()['time']
+            if self.time_limit is not None:
+                time_limit = self.time_limit
+            else:
+                time_limit = _get_limits()['time']
         if size_limit is None:
-            size_limit = _get_limits()['size']
+            if self.size_limit is not None:
+                size_limit = self.size_limit
+            else:
+                size_limit = _get_limits()['size']
 
         res, truncated = super(ldap2, self).find_entries(
             filter=filter, attrs_list=attrs_list, base_dn=base_dn, scope=scope,
@@ -199,7 +214,6 @@ class ldap2(CrudBackend, LDAPClient):
             search_refs=search_refs, paged_search=paged_search)
         return (res, truncated)
 
-    config_defaults = {'ipasearchtimelimit': [2], 'ipasearchrecordslimit': [0]}
     def get_ipa_config(self, attrs_list=None):
         """Returns the IPA configuration entry (dn, entry_attrs)."""
 
@@ -223,9 +237,11 @@ class ldap2(CrudBackend, LDAPClient):
             config_entry = entries[0]
         except errors.NotFound:
             config_entry = self.make_entry(dn)
-        for a in self.config_defaults:
-            if a not in config_entry:
-                config_entry[a] = self.config_defaults[a]
+
+        # set defaults
+        config_entry.setdefault('ipasearchtimelimit', [self.DEFAULT_TIME_LIMIT])
+        config_entry.setdefault('ipasearchrecordslimit', [self.DEFAULT_SIZE_LIMIT])
+
         context.config_entry = config_entry
         return config_entry
 
-- 
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