Moodle
  1. Moodle
  2. MDL-36119

ldap auth sync - add paged support

    Details

    • Testing Instructions:
      Hide

      Make sure you can run PHP 5.4 and PHP 5.3 - tests need to be done on both PHP versions. probably best to run on PHP 5.3 first.

      Make sure you have an LDAP store that has a high number of users (over 1000 in a single container)

      set Moodle ldap auth to connect to your LDAP as per usual and check to make sure login works correctly with a few accounts.
      with PHP 5.3
      run the CLI script /auth/ldap/cli/sync_users.php
      Not all ldap accounts will be transferred into Moodle (unless you've configured your ldap to return more than the usual number of records)
      Move to PHP 5.4
      run the CLI script again - it should create all the missing accounts that it previously missed.

      Test ldap enrolment.

      Show
      Make sure you can run PHP 5.4 and PHP 5.3 - tests need to be done on both PHP versions. probably best to run on PHP 5.3 first. Make sure you have an LDAP store that has a high number of users (over 1000 in a single container) set Moodle ldap auth to connect to your LDAP as per usual and check to make sure login works correctly with a few accounts. with PHP 5.3 run the CLI script /auth/ldap/cli/sync_users.php Not all ldap accounts will be transferred into Moodle (unless you've configured your ldap to return more than the usual number of records) Move to PHP 5.4 run the CLI script again - it should create all the missing accounts that it previously missed. Test ldap enrolment.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip_master_mdl-36119_ldap_paged_results
    • Rank:
      44885

      Description

      I couldn't find an existing open bug for this so if I've missed something let me know.

      LDAP in PHP has had a problem for ages that has meant that the sync process hasn't worked well. That bug has finally been fixed in PHP 5.4 so we should be able to implement a conditional fix for Moodle 2.4 at least.

      most of the work on this has been done already - we just need to port this for Moodle 2.4
      more info here:
      https://github.com/jcharaoui/moodle-cegep/wiki/PHP-LDAP-Paging

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          adding Jerome, Iñaki here - would be awesome to finally get this patch in.

          Show
          Dan Marsden added a comment - adding Jerome, Iñaki here - would be awesome to finally get this patch in.
          Hide
          Dan Marsden added a comment -

          here's a first untested attempt at applying Jeromes patch on the master branch - am hoping to test it this week - interested to hear from anyone else that gets a chance too.

          Show
          Dan Marsden added a comment - here's a first untested attempt at applying Jeromes patch on the master branch - am hoping to test it this week - interested to hear from anyone else that gets a chance too.
          Hide
          Dan Marsden added a comment -

          wicked - just tested this and it worked really well! - thanks Jerome!

          Show
          Dan Marsden added a comment - wicked - just tested this and it worked really well! - thanks Jerome!
          Hide
          Dan Marsden added a comment -

          bumping this up for peer review (I'm not sure if it qualifies for integration post freeze so this might sit here for a while.)

          I haven't looked much at the actual code that Jerome wrote - I basically copy/pasted/merged it so it's possible there's stuff there that doesn't meet coding guidelines etc.

          Show
          Dan Marsden added a comment - bumping this up for peer review (I'm not sure if it qualifies for integration post freeze so this might sit here for a while.) I haven't looked much at the actual code that Jerome wrote - I basically copy/pasted/merged it so it's possible there's stuff there that doesn't meet coding guidelines etc.
          Hide
          Jerome Charaoui added a comment -

          I agree, it's now more relevant than ever since PHP officially supports paging in 5.4

          Show
          Jerome Charaoui added a comment - I agree, it's now more relevant than ever since PHP officially supports paging in 5.4
          Hide
          Dan Poltawski added a comment -

          Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

          Show
          Dan Poltawski added a comment - Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..
          Hide
          Dan Poltawski added a comment -

          I'm taking this out of integration, as I think we need input from Iñaki on this one first.

          IIRC he was pushing php devs for this support and might have a plan for this.

          Show
          Dan Poltawski added a comment - I'm taking this out of integration, as I think we need input from Iñaki on this one first. IIRC he was pushing php devs for this support and might have a plan for this.
          Hide
          Dan Poltawski added a comment -

          Iñaki, added you as a peer reviewer on this one. Feel free to unassign if you are not able to.

          cheers!

          Dan

          Show
          Dan Poltawski added a comment - Iñaki, added you as a peer reviewer on this one. Feel free to unassign if you are not able to. cheers! Dan
          Hide
          Dan Marsden added a comment -

          yeah - I talked with Dan about this last week - I did the work on Fri before I left for the hackfest and pushed it for peer review - I wasn't ready for it to be grabbed for integration yet - keen for Iñaki's feedback first.

          Show
          Dan Marsden added a comment - yeah - I talked with Dan about this last week - I did the work on Fri before I left for the hackfest and pushed it for peer review - I wasn't ready for it to be grabbed for integration yet - keen for Iñaki's feedback first.
          Hide
          Iñaki Arenaza added a comment -

          Hi,

          I've had a look at this and while it looks good, I've refactored it a bit and added a configurable option for the page size. Given that I'm not a native English speaker, it would be a good idea that someone checks the new language strings I added.

          Also while testing it, I found out that we were 'leaking' some LDAP connections, so I've fixed those too.

          My refactored version is at https://github.com/iarenaza/moodle/compare/master...wip_master_mdl-36119_ldap_paged_results

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi, I've had a look at this and while it looks good, I've refactored it a bit and added a configurable option for the page size. Given that I'm not a native English speaker, it would be a good idea that someone checks the new language strings I added. Also while testing it, I found out that we were 'leaking' some LDAP connections, so I've fixed those too. My refactored version is at https://github.com/iarenaza/moodle/compare/master...wip_master_mdl-36119_ldap_paged_results Saludos. Iñaki.
          Hide
          Dan Marsden added a comment -

          nice improvements! - would be great if we could get this into 2.4!

          Show
          Dan Marsden added a comment - nice improvements! - would be great if we could get this into 2.4!
          Hide
          Dan Marsden added a comment -

          not sure about using error_log for expected behaviour:
          error_log(get_string('pagedresultspagesize', 'auth_ldap', $this->config->pagesize));

          useful for debugging but not sure about adding to error_log ? - integrators might have further thoughts on this?

          Show
          Dan Marsden added a comment - not sure about using error_log for expected behaviour: error_log(get_string('pagedresultspagesize', 'auth_ldap', $this->config->pagesize)); useful for debugging but not sure about adding to error_log ? - integrators might have further thoughts on this?
          Hide
          Dan Marsden added a comment -

          I'd be tempted to change the pagesize string to something simpler and shorter like this:
          "Make sure this value is smaller than your LDAP server size limit
          (the maximum number of entries that can be returned in a single query)."

          and then disable the value in the form if ldapversion is set to 2 (not sure if auth plugins liet us do this easily - they don't seem to have been converted to mforms

          also tempted to hide the setting completely if the installed version of PHP doesn't support it. - or maybe display a message on the ldap settings page to advise upgrading PHP?

          Show
          Dan Marsden added a comment - I'd be tempted to change the pagesize string to something simpler and shorter like this: "Make sure this value is smaller than your LDAP server size limit (the maximum number of entries that can be returned in a single query)." and then disable the value in the form if ldapversion is set to 2 (not sure if auth plugins liet us do this easily - they don't seem to have been converted to mforms also tempted to hide the setting completely if the installed version of PHP doesn't support it. - or maybe display a message on the ldap settings page to advise upgrading PHP?
          Hide
          Jerome Charaoui added a comment -

          "LDAP server size limit" isn't very descriptive. If it needs to be shortened, it has to specifcy "result set" or "search result", like "LDAP server result set size limit".

          Good idea to disable the setting but don't hide it : people might consider upgrading the PHP version if they realize they need this setting. Also, make sure that you test the support using something like function_exists, and not against a particular PHP version. That way, those of us who use a patched version of PHP 5.3 will stay happy

          Show
          Jerome Charaoui added a comment - "LDAP server size limit" isn't very descriptive. If it needs to be shortened, it has to specifcy "result set" or "search result", like "LDAP server result set size limit". Good idea to disable the setting but don't hide it : people might consider upgrading the PHP version if they realize they need this setting. Also, make sure that you test the support using something like function_exists, and not against a particular PHP version. That way, those of us who use a patched version of PHP 5.3 will stay happy
          Hide
          Iñaki Arenaza added a comment -

          I've pushed a new version of the patch (same url), taking into account your suggestions.

          Unfortunately, as Dan says, auth plugins haven't been converted to mforms, so I've tried to be as lightweight as possible (no javascript, etc).

          Jêrome, the tests are from your original patch, so we use function_exists as you suggest

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - I've pushed a new version of the patch (same url), taking into account your suggestions. Unfortunately, as Dan says, auth plugins haven't been converted to mforms, so I've tried to be as lightweight as possible (no javascript, etc). Jêrome, the tests are from your original patch, so we use function_exists as you suggest Saludos. Iñaki.
          Hide
          Iñaki Arenaza added a comment -

          Dan, Jêrome,

          do you think this is ready for submitting it for integration?

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Dan, Jêrome, do you think this is ready for submitting it for integration? Saludos. Iñaki.
          Hide
          Jerome Charaoui added a comment -

          Yes, I think its ready. Thanks Iñaki and Dan for making this happen, it's a great patch!

          Show
          Jerome Charaoui added a comment - Yes, I think its ready. Thanks Iñaki and Dan for making this happen, it's a great patch!
          Hide
          Dan Marsden added a comment -

          definitely - thanks to you guys for getting this done!

          Show
          Dan Marsden added a comment - definitely - thanks to you guys for getting this done!
          Hide
          Iñaki Arenaza added a comment -

          Yup, I completely forgot about enrol/ldap. I have just pushed a new version with enrol/ldap support included.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Yup, I completely forgot about enrol/ldap. I have just pushed a new version with enrol/ldap support included. Saludos. Iñaki.
          Hide
          Dan Poltawski added a comment -

          I've added the QA test required flag, because we need QA tests for:

          • LDAP enrolment
          • LDAP authentication

          Which will make me confident about doing changes with these plugins. These are plugins used by a lot of large institutions, so its important.

          Helen/Mary/QA test triager: I think you'll need some help creating these tests as its rather technical (and also needs an LDAP directory setup to test against).

          Show
          Dan Poltawski added a comment - I've added the QA test required flag, because we need QA tests for: LDAP enrolment LDAP authentication Which will make me confident about doing changes with these plugins. These are plugins used by a lot of large institutions, so its important. Helen/Mary/QA test triager: I think you'll need some help creating these tests as its rather technical (and also needs an LDAP directory setup to test against).
          Hide
          Dan Poltawski added a comment -

          Thanks a lot for your work on this guys, i've integrated this now.

          I did a few things during integration

          • I removed the disabling of the setting based on ldap paging support, it was breaking upgrade/install if php version didn't support it (the setting was never saved). We should investigate this in a new issue.
          • Removed <em> from complete lang string, we should put emphasis on the surrounding call rather than getting all the translators to do it
          • Fixed trailing whitespace.

          (Also noting to casual observers that the code in the ldap plugins is quite far away from documented moodle coding style and this patch continues a bit like that ).

          The big thing we need with this is decent testing, so as commneted i've added the QA tests required flag, and also hope that we can regression test each too.

          Show
          Dan Poltawski added a comment - Thanks a lot for your work on this guys, i've integrated this now. I did a few things during integration I removed the disabling of the setting based on ldap paging support, it was breaking upgrade/install if php version didn't support it (the setting was never saved). We should investigate this in a new issue. Removed <em> from complete lang string, we should put emphasis on the surrounding call rather than getting all the translators to do it Fixed trailing whitespace. (Also noting to casual observers that the code in the ldap plugins is quite far away from documented moodle coding style and this patch continues a bit like that ). The big thing we need with this is decent testing, so as commneted i've added the QA tests required flag, and also hope that we can regression test each too.
          Hide
          Ankit Agarwal added a comment - - edited

          As per discussion with Dan, we changed this to regression testing as atm we donot have a setup of ldap with 1000 of users.
          I installed a ldap setup, with php 5.4 I was able to login as a ldap user without any issue.
          Ldap server has 4 users.
          However when I ran the cli script it gave me the following error:-

          Connecting to LDAP server...
          Creating temporary table tmp_extuser
          Did not get any users from LDAP -- error? -- exiting
          Potential coding error - existing temptables found when disposing database. Must be dropped!
          

          Failing this sorry.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited As per discussion with Dan, we changed this to regression testing as atm we donot have a setup of ldap with 1000 of users. I installed a ldap setup, with php 5.4 I was able to login as a ldap user without any issue. Ldap server has 4 users. However when I ran the cli script it gave me the following error:- Connecting to LDAP server... Creating temporary table tmp_extuser Did not get any users from LDAP -- error? -- exiting Potential coding error - existing temptables found when disposing database. Must be dropped! Failing this sorry. Thanks
          Hide
          Ankit Agarwal added a comment -

          Just confirmed everything is working before the patch.
          Thanks

          Show
          Ankit Agarwal added a comment - Just confirmed everything is working before the patch. Thanks
          Hide
          Dan Poltawski added a comment -

          We need to get the BETA out tomorrow, so if we can't come up with a solution for this in the next few hours i'm afraid the only option will be to revert this change.

          Show
          Dan Poltawski added a comment - We need to get the BETA out tomorrow, so if we can't come up with a solution for this in the next few hours i'm afraid the only option will be to revert this change.
          Hide
          Iñaki Arenaza added a comment -

          I have just run the tests with the following setup (everything running locally in my laptop: Core2 Duo P8800 @2.66GHz, 4GB RAM, HD Intel SSD 520):

          • Moodle from git.moodle.org/integration.git (cloned today at 10:30 CET)
            • Configured to use 250 for the page size
          • MySQL 5.5.24
          • PHP 5.4.4
          • Apache 2.2.22
          • OpenLDAP 2.4.31
            • soft size limit of 5, hard size limit of 10, unlimited size limit for paged requests (see http://www.openldap.org/doc/admin24/limits.html).
            • 20000 users in a single organizational unit
            • 4000 courses in a single organizational unit (each course having 25 users enroled as students)

          Normal logins work as expected.

          It synchronises the 20000 users without problem (takes ~3 minutes when updating user details, ~20 seconds without user updates, with everything already in RAM from a previous ran). It also enrols the users correctly[1] (takes ~45 minutes when creating the courses, ~18 minutes with courses already created).

          If I manually force the config not to use paged results (I don't have PHP 5.3 here), I hit the soft size limit and the process stops after processing just 5 users or 5 courses.

          [1] There's a bug in the enrolment code, I'm attaching the patch here in a minute.

          I'm going to upload all the files needed to reproduce my setup (OpenLDAP config files, dumps of the OpenLDAP directory, Moodle LDAP settings, etc.), in case anyone else wants to try the test.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - I have just run the tests with the following setup (everything running locally in my laptop: Core2 Duo P8800 @2.66GHz, 4GB RAM, HD Intel SSD 520): Moodle from git.moodle.org/integration.git (cloned today at 10:30 CET) Configured to use 250 for the page size MySQL 5.5.24 PHP 5.4.4 Apache 2.2.22 OpenLDAP 2.4.31 soft size limit of 5, hard size limit of 10, unlimited size limit for paged requests (see http://www.openldap.org/doc/admin24/limits.html ). 20000 users in a single organizational unit 4000 courses in a single organizational unit (each course having 25 users enroled as students) Normal logins work as expected. It synchronises the 20000 users without problem (takes ~3 minutes when updating user details, ~20 seconds without user updates, with everything already in RAM from a previous ran). It also enrols the users correctly [1] (takes ~45 minutes when creating the courses, ~18 minutes with courses already created). If I manually force the config not to use paged results (I don't have PHP 5.3 here), I hit the soft size limit and the process stops after processing just 5 users or 5 courses. [1] There's a bug in the enrolment code, I'm attaching the patch here in a minute. I'm going to upload all the files needed to reproduce my setup (OpenLDAP config files, dumps of the OpenLDAP directory, Moodle LDAP settings, etc.), in case anyone else wants to try the test. Saludos. Iñaki.
          Hide
          Iñaki Arenaza added a comment -

          This is the fix for the enrolment bug I just mentionned.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - This is the fix for the enrolment bug I just mentionned. Saludos. Iñaki.
          Hide
          Iñaki Arenaza added a comment -

          Here are the files needed to reproduce my setup. See the README.txt file for instructions and details.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Here are the files needed to reproduce my setup. See the README.txt file for instructions and details. Saludos. Iñaki.
          Hide
          Dan Poltawski added a comment -

          Thanks a lot Iñaki

          Show
          Dan Poltawski added a comment - Thanks a lot Iñaki
          Hide
          Dan Poltawski added a comment -

          BTW, how do you create all the users in your openldap directory?

          Show
          Dan Poltawski added a comment - BTW, how do you create all the users in your openldap directory?
          Hide
          Iñaki Arenaza added a comment -

          Just follow the instructions in the README file

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Just follow the instructions in the README file Saludos. Iñaki.
          Hide
          Dan Poltawski added a comment -

          Nono, i've done that - thanks! I mean, how did YOU do it

          Show
          Dan Poltawski added a comment - Nono, i've done that - thanks! I mean, how did YOU do it
          Hide
          Dan Poltawski added a comment -

          This does look to be working with Inaki's ldap data

          Show
          Dan Poltawski added a comment - This does look to be working with Inaki's ldap data
          Hide
          Dan Poltawski added a comment -

          So passing this.

          We really need this QA'd properly with tests in the cucle.

          Show
          Dan Poltawski added a comment - So passing this. We really need this QA'd properly with tests in the cucle.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

          (not really)

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!
          Hide
          Petr Škoda added a comment - - edited

          Hello, the connection closing in user enrolments sync is done incorrectly, it breaks pretty badly in login enrol sync MDL-37315, because the code calling find_ext_enrolments() ends up using closed ldap connection.

          Show
          Petr Škoda added a comment - - edited Hello, the connection closing in user enrolments sync is done incorrectly, it breaks pretty badly in login enrol sync MDL-37315 , because the code calling find_ext_enrolments() ends up using closed ldap connection.
          Hide
          Mary Cooch added a comment - - edited

          (Housekeeping)If anyone thinks this needs to be added to the user docs, please feel free to do so as I don't really understand LDAP enough to do it; otherwise I will remove the docs_required label.

          Show
          Mary Cooch added a comment - - edited (Housekeeping)If anyone thinks this needs to be added to the user docs, please feel free to do so as I don't really understand LDAP enough to do it; otherwise I will remove the docs_required label.
          Show
          Dan Marsden added a comment - updated docs for 2.4 and 2.5 here: http://docs.moodle.org/25/en/LDAP_authentication#Setting_up_regular_automatic_synchronisation_using_cron thanks!
          Hide
          Mary Cooch added a comment -

          oh great thanks

          Show
          Mary Cooch added a comment - oh great thanks

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: