Bug 41764 - Check S4-Connector behaviour for groups with >1500 members
Check S4-Connector behaviour for groups with >1500 members
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: S4 Connector
UCS 4.1
Other Linux
: P2 normal (vote)
: UCS 4.2
Assigned To: Lukas Oyen
Arvid Requate
https://msdn.microsoft.com/en-us/libr...
: interim-3
Depends on: 41744
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-11 13:21 CEST by Arvid Requate
Modified: 2017-04-04 18:28 CEST (History)
5 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 2: Improvement: Would be a product improvement
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 2: A Pain – users won’t like this once they notice it
User Pain: 0.046
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional): Large environments, Troubleshooting
Max CVSS v3 score:
oyen: Patch_Available+


Attachments
Paged member search ported to the S4-connector (20.00 KB, application/x-tar)
2016-12-15 15:02 CET, Lukas Oyen
Details
bug41764_qa.patch (1.48 KB, patch)
2017-03-16 16:07 CET, Arvid Requate
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Requate univentionstaff 2016-07-11 13:21:17 CEST
We need to check if the S4-Connector can deal properly with groups containing more than 1500 members. Bug 41744 reported an issue in the AD-Connector.


+++ This bug was initially created as a clone of Bug #41744 +++

If a group on UCS side contains more than 1500 member and the group will be synced to the AD all members will be removed during back sync.
Comment 1 Arvid Requate univentionstaff 2016-08-16 22:39:42 CEST
Samba 4.3.7 doesn't return ranged results for attributes with more than 1500 values, but that might change in the future. We should merge the fix from
Bug #41744 to be safe from unpleasant surprises.
Comment 2 Lukas Oyen univentionstaff 2016-12-15 15:02:12 CET
Created attachment 8306 [details]
Paged member search ported to the S4-connector
Comment 3 Lukas Oyen univentionstaff 2016-12-15 15:06:05 CET
(In reply to Lukas Oyen from comment #2)
> Created attachment 8306 [details]
> Paged member search ported to the S4-connector

The attached patches port the paged member search from bug #41744 to the s4-connector (with a slight cleanup). All s4-connector tests on a single UCS 4.1-4 errata359 are passing with the following exceptions (but those are failing the jenkins tests aswell):

- 159sync_ad_create_non_domain_user
- 259read_ad_create_non_domain_user
- 269read_ad_move_object_from_ignore_subtree
Comment 4 Lukas Oyen univentionstaff 2017-02-28 17:07:46 CET
Committed in r77169 and r77170. Changelog r77171/r77172.
Comment 5 Florian Best univentionstaff 2017-03-02 17:12:10 CET
svn r77170:
+			s4_members_lower = map(str.lower, s4_members)
+			if object_dn_modstring.lower() not in s4_members_lower:  # add as member

I think this change is dangerous because it can't handle unicode (are you 100% sure only bytes will end there?):
>>> map(str.lower, ['', u''])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: descriptor 'lower' requires a 'str' object but received a 'unicode'

Better might be:
if all(object_dn_modstring.lower() != x.lower() for x in s4_members):
Comment 6 Arvid Requate univentionstaff 2017-03-16 16:07:35 CET
Created attachment 8558 [details]
bug41764_qa.patch

The patch addresses two points:
* syntax of [].extend()
* Fix for Comment 5
  My patch for the AD-Connector also was not optimal, because the
  list comprehension has a different behavior than the for / break.
Comment 7 Lukas Oyen univentionstaff 2017-03-20 14:30:08 CET
(In reply to Arvid Requate from comment #6)
> Created attachment 8558 [details]
> bug41764_qa.patch


1st hunk:
-	group_cache.extend(m.lower() for m in s4_members)
+	group_cache.extend([m.lower() for m in s4_members])

This doesn't make a difference. `list.lower()` expects an iterator. A generator expression is a valid iterator. The old approach in fact saves some memory as it does not create a list just to throw it away.

2nd hunk:

I don't mean to be harsh, but I don't see the difference or improvement upon the approach as suggested in comment #5 or the simple `if x not in y`. In fact I find a `for .. else ..` harder to read.

(In reply to Florian Best from comment #5)
> are you 100% sure only bytes will end there?

Yes, as just above, the following was called (not pretty but it works):

s4_members = map(compatible_modstring, s4_members)
Comment 8 Arvid Requate univentionstaff 2017-03-20 15:21:34 CET
Ok, changelog too.
Comment 9 Stefan Gohmann univentionstaff 2017-04-04 18:28:58 CEST
UCS 4.2 has been released:
 https://docs.software-univention.de/release-notes-4.2-0-en.html
 https://docs.software-univention.de/release-notes-4.2-0-de.html

If this error occurs again, please use "Clone This Bug".