Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Jan Cholasta]

On 30.11.2015 14:15, Martin Basti wrote:



On 30.11.2015 08:19, Jan Cholasta wrote:

On 24.11.2015 10:15, Martin Basti wrote:



On 20.11.2015 09:00, Jan Cholasta wrote:

On 19.11.2015 14:13, Jan Cholasta wrote:

On 19.11.2015 14:09, Martin Babinsky wrote:

On 11/19/2015 01:08 PM, Martin Basti wrote:



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



ACK


Hold your horses.


1) The upgrade_ldapsearch_limit option is not very useful, either add
generic search_time_limit and search_records_limit options, or don't
add any options at all (which, incidentally, is exactly what the
ticket asks for).

2) The limits are not passed to the connection using __init__()
anywhere, so there is no need to have arguments for them.

3) Rather than adding layers of if statements everywhere, implement
the time_limit and size_limit attributes as properties.

4) Use config_entry.single_value when setting defaults in
ldap2.get_ipa_config().

BTW, add comments only when it's not obvious what the code does - a
comment saying "set defaults" above two .setdefault() calls is pretty
redundant.


Updated patch attached


In ldap2.create_connection(), you should set self.time_limit only if
time_limit is not None, ditto for size limit, instead of handling None
in the properties' setters.

You don't need to handle the limits in ldap2.find_entries() anymore,
since it is handled by LDAPClient.find_entries(), so you can remove
the related code completely from ldap2.find_entries().


Updated patch attached.


The defaults should be set in LDAPClient class attributes rather than in 
__init__().


Besides that ACK.

To speed things up I have amended your patch myself, see attachment.

Pushed to master: 2a1a3c498a71e85193af76a25333ebe9011e6b2a

--
Jan Cholasta
From 097b59f49776c4842c4ff774541f9f101bbda753 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 18 Nov 2015 10:31:05 +0100
Subject: [PATCH] Upgrade: increase time limit for upgrades

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

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/plugins/permission.py|  3 +-
 ipapython/ipaldap.py| 11 +--
 ipaserver/install/ldapupdate.py |  5 ++-
 ipaserver/plugins/ldap2.py  | 72 -
 4 files changed, 57 insertions(+), 34 deletions(-)

diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index b17b61e..cbc48f5 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -1292,8 +1292,7 @@ class permission_find(baseldap.LDAPSearch):
 if 'sizelimit' in options:
 max_entries = options['sizelimit']
 else:
-config = ldap.get_ipa_config()
-max_entries = int(config.single_value['ipasearchrecordslimit'])
+max_entries = self.api.Backend.ldap2.size_limit
 
 filters = ['(objectclass=ipaPermission)',
'(!(ipaPermissionType=V2))']
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 7e5bc04..bbd27ac 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -689,6 +689,9 @@ class LDAPClient(object):
 'nsslapd-minssf-exclude-rootdse': True,
 })
 
+time_limit = -1.0   # unlimited
+size_limit = 0  # unlimited
+
 def __init__(self, ldap_uri, start_tls=False, force_schema_updates=False,
  no_schema=False, decode_attrs=True):
 """Create LDAPClient 

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Martin Basti]

On 30.11.2015 08:19, Jan Cholasta wrote:

On 24.11.2015 10:15, Martin Basti wrote:



On 20.11.2015 09:00, Jan Cholasta wrote:

On 19.11.2015 14:13, Jan Cholasta wrote:

On 19.11.2015 14:09, Martin Babinsky wrote:

On 11/19/2015 01:08 PM, Martin Basti wrote:



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



ACK


Hold your horses.


1) The upgrade_ldapsearch_limit option is not very useful, either add
generic search_time_limit and search_records_limit options, or don't
add any options at all (which, incidentally, is exactly what the
ticket asks for).

2) The limits are not passed to the connection using __init__()
anywhere, so there is no need to have arguments for them.

3) Rather than adding layers of if statements everywhere, implement
the time_limit and size_limit attributes as properties.

4) Use config_entry.single_value when setting defaults in
ldap2.get_ipa_config().

BTW, add comments only when it's not obvious what the code does - a
comment saying "set defaults" above two .setdefault() calls is pretty
redundant.


Updated patch attached


In ldap2.create_connection(), you should set self.time_limit only if 
time_limit is not None, ditto for size limit, instead of handling None 
in the properties' setters.


You don't need to handle the limits in ldap2.find_entries() anymore, 
since it is handled by LDAPClient.find_entries(), so you can remove 
the related code completely from ldap2.find_entries().



Updated patch attached.
From de56acb8b745dab14560790b4166c852ad014651 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 18 Nov 2015 10:31:05 +0100
Subject: [PATCH] Upgrade: increase time limit for upgrades

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

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/plugins/permission.py|  3 +-
 ipapython/ipaldap.py| 10 --
 ipaserver/install/ldapupdate.py |  5 ++-
 ipaserver/plugins/ldap2.py  | 70 -
 4 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index b17b61e696a8f333a25150f60f27cffd03085196..cbc48f5e8a6e5765d5eca59797ca75b7a9d98244 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -1292,8 +1292,7 @@ class permission_find(baseldap.LDAPSearch):
 if 'sizelimit' in options:
 max_entries = options['sizelimit']
 else:
-config = ldap.get_ipa_config()
-max_entries = int(config.single_value['ipasearchrecordslimit'])
+max_entries = self.api.Backend.ldap2.size_limit
 
 filters = ['(objectclass=ipaPermission)',
'(!(ipaPermissionType=V2))']
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 7e5bc04fea1dc89cc141051025d00a57d74b2b41..8274039e6d6b6cad431ea83e9bc768afadca77ab 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -712,6 +712,8 @@ class LDAPClient(object):
 self._force_schema_updates = force_schema_updates
 self._no_schema = no_schema
 self._decode_attrs = decode_attrs
+self.time_limit = -1.0  # unlimited
+self.size_limit = 0  # unlimited
 
 self.log = log_mgr.get_logger(self)
 self._has_schema = False
@@ -1294,10 +1296,14 @@ class LDAPClient(object):
 res = []
 truncated = False
 
-if time_limit is None or time_limit == 0:

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Jan Cholasta]

On 24.11.2015 10:15, Martin Basti wrote:



On 20.11.2015 09:00, Jan Cholasta wrote:

On 19.11.2015 14:13, Jan Cholasta wrote:

On 19.11.2015 14:09, Martin Babinsky wrote:

On 11/19/2015 01:08 PM, Martin Basti wrote:



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



ACK


Hold your horses.


1) The upgrade_ldapsearch_limit option is not very useful, either add
generic search_time_limit and search_records_limit options, or don't
add any options at all (which, incidentally, is exactly what the
ticket asks for).

2) The limits are not passed to the connection using __init__()
anywhere, so there is no need to have arguments for them.

3) Rather than adding layers of if statements everywhere, implement
the time_limit and size_limit attributes as properties.

4) Use config_entry.single_value when setting defaults in
ldap2.get_ipa_config().

BTW, add comments only when it's not obvious what the code does - a
comment saying "set defaults" above two .setdefault() calls is pretty
redundant.


Updated patch attached


In ldap2.create_connection(), you should set self.time_limit only if 
time_limit is not None, ditto for size limit, instead of handling None 
in the properties' setters.


You don't need to handle the limits in ldap2.find_entries() anymore, 
since it is handled by LDAPClient.find_entries(), so you can remove the 
related code completely from ldap2.find_entries().


--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Martin Basti]

On 20.11.2015 09:00, Jan Cholasta wrote:

On 19.11.2015 14:13, Jan Cholasta wrote:

On 19.11.2015 14:09, Martin Babinsky wrote:

On 11/19/2015 01:08 PM, Martin Basti wrote:



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



ACK


Hold your horses.


1) The upgrade_ldapsearch_limit option is not very useful, either add 
generic search_time_limit and search_records_limit options, or don't 
add any options at all (which, incidentally, is exactly what the 
ticket asks for).


2) The limits are not passed to the connection using __init__() 
anywhere, so there is no need to have arguments for them.


3) Rather than adding layers of if statements everywhere, implement 
the time_limit and size_limit attributes as properties.


4) Use config_entry.single_value when setting defaults in 
ldap2.get_ipa_config().


BTW, add comments only when it's not obvious what the code does - a 
comment saying "set defaults" above two .setdefault() calls is pretty 
redundant.



Updated patch attached
From 754d3506a26e239121917ff8f08d30b291657397 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 18 Nov 2015 10:31:05 +0100
Subject: [PATCH] Upgrade: increase time limit for upgrades

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

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/plugins/permission.py|  3 +-
 ipapython/ipaldap.py| 10 +--
 ipaserver/install/ldapupdate.py |  5 +++-
 ipaserver/plugins/ldap2.py  | 66 ++---
 4 files changed, 62 insertions(+), 22 deletions(-)

diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index b17b61e696a8f333a25150f60f27cffd03085196..cbc48f5e8a6e5765d5eca59797ca75b7a9d98244 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -1292,8 +1292,7 @@ class permission_find(baseldap.LDAPSearch):
 if 'sizelimit' in options:
 max_entries = options['sizelimit']
 else:
-config = ldap.get_ipa_config()
-max_entries = int(config.single_value['ipasearchrecordslimit'])
+max_entries = self.api.Backend.ldap2.size_limit
 
 filters = ['(objectclass=ipaPermission)',
'(!(ipaPermissionType=V2))']
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 7e5bc04fea1dc89cc141051025d00a57d74b2b41..8274039e6d6b6cad431ea83e9bc768afadca77ab 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -712,6 +712,8 @@ class LDAPClient(object):
 self._force_schema_updates = force_schema_updates
 self._no_schema = no_schema
 self._decode_attrs = decode_attrs
+self.time_limit = -1.0  # unlimited
+self.size_limit = 0  # unlimited
 
 self.log = log_mgr.get_logger(self)
 self._has_schema = False
@@ -1294,10 +1296,14 @@ class LDAPClient(object):
 res = []
 truncated = False
 
-if time_limit is None or time_limit == 0:
+if time_limit is None:
+time_limit = self.time_limit
+if time_limit == 0:
 time_limit = -1.0
+
 if size_limit is None:
-size_limit = 0
+size_limit = self.size_limit
+
 if not isinstance(size_limit, int):
 size_limit = int(size_limit)
 if not isinstance(time_limit, float):
diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index 

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Jan Cholasta]

On 19.11.2015 14:13, Jan Cholasta wrote:

On 19.11.2015 14:09, Martin Babinsky wrote:

On 11/19/2015 01:08 PM, Martin Basti wrote:



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



ACK


Hold your horses.


1) The upgrade_ldapsearch_limit option is not very useful, either add 
generic search_time_limit and search_records_limit options, or don't add 
any options at all (which, incidentally, is exactly what the ticket asks 
for).


2) The limits are not passed to the connection using __init__() 
anywhere, so there is no need to have arguments for them.


3) Rather than adding layers of if statements everywhere, implement the 
time_limit and size_limit attributes as properties.


4) Use config_entry.single_value when setting defaults in 
ldap2.get_ipa_config().


BTW, add comments only when it's not obvious what the code does - a 
comment saying "set defaults" above two .setdefault() calls is pretty 
redundant.


--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Martin Basti]

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

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Jan Cholasta]

On 19.11.2015 14:09, Martin Babinsky wrote:

On 11/19/2015 01:08 PM, Martin Basti wrote:



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



ACK


Hold your horses.

--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Martin Kosek]

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?

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

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Martin Basti]

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.


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

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Martin Basti]

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


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

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Martin Kosek]

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.

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