URL: https://github.com/SSSD/sssd/pull/5300 Author: pbrezina Title: #5300: ad: use parallel cldap ping for site discovery Action: opened
PR body: """ This PR adds support for CLDAP (connectionless LDAP over UDP) and makes use of it during site discovery.
The discovery process is improved, it now sends the CLDAP ping to all discovered domain controllers at once to avoid potential timeouts when some of them are unreachable (which is quite common in real environment). The first received reply is used.
The netlogon information is now requested only when SSSD starts and when we are recovering from offline state To avoid swamping network with many ping. This is quite fine given the site and forest information is not dynamic but usually stable.
Additional improvement is that if we already know client's site we ping only in-site controllers first. We were already doing that but now we also fallback to off-site controllers in case none in-site controller is unreachable. Previously we would just fail.
There is a bug in openldap [1] that makes SSSD wait for timeout in-case none of the controller is reachable, but as long as at least one is reachble the resolution will be fast.
Resolves: https://github.com/SSSD/sssd/issues/3743 https://github.com/SSSD/sssd/issues/5215
[1] https://bugs.openldap.org/show_bug.cgi?id=9328 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5300/head:pr5300 git checkout pr5300
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
abbra commented: """ @pbrezina I see linking failures:
``` /usr/bin/ld: /tmp/nss-srv-tests.hGpNig.ltrans0.ltrans.o: in function `__wrap_sss_packet_get_body': /builddir/build/BUILD/sssd-2.3.2/src/tests/cmocka/test_nss_srv.c:103: undefined reference to `sss_packet_get_body' /usr/bin/ld: /tmp/nss-srv-tests.hGpNig.ltrans1.ltrans.o: in function `__wrap_sss_cmd_done': /builddir/build/BUILD/sssd-2.3.2/src/tests/cmocka/test_nss_srv.c:139: undefined reference to `sss_packet_get_body' /usr/bin/ld: /tmp/nss-srv-tests.hGpNig.ltrans1.ltrans.o: in function `__wrap_sss_ncache_check_user': /builddir/build/BUILD/sssd-2.3.2/src/tests/cmocka/test_nss_srv.c:170: undefined reference to `sss_ncache_check_user' /usr/bin/ld: /tmp/nss-srv-tests.hGpNig.ltrans1.ltrans.o: in function `__wrap_sss_ncache_check_upn': /builddir/build/BUILD/sssd-2.3.2/src/tests/cmocka/test_nss_srv.c:185: undefined reference to `sss_ncache_check_upn' /usr/bin/ld: /tmp/nss-srv-tests.hGpNig.ltrans1.ltrans.o: in function `__wrap_sss_ncache_check_uid': /builddir/build/BUILD/sssd-2.3.2/src/tests/cmocka/test_nss_srv.c:200: undefined reference to `sss_ncache_check_uid' /usr/bin/ld: /tmp/nss-srv-tests.hGpNig.ltrans1.ltrans.o: in function `__wrap_sss_ncache_check_sid': /builddir/build/BUILD/sssd-2.3.2/src/tests/cmocka/test_nss_srv.c:213: undefined reference to `sss_ncache_check_sid' /usr/bin/ld: /tmp/nss-srv-tests.hGpNig.ltrans1.ltrans.o: in function `__wrap_sss_ncache_check_cert': /builddir/build/BUILD/sssd-2.3.2/src/tests/cmocka/test_nss_srv.c:226: undefined reference to `sss_ncache_check_cert' collect2: error: ld returned 1 exit status ``` """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-694177356
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
alexey-tikhonov commented: """
@pbrezina I see linking failures:
/usr/bin/ld: /tmp/nss-srv-tests.hGpNig.ltrans0.ltrans.o: in function `__wrap_sss_packet_get_body':
...
cmocka based tests aren't compatible with LTO linker flag.
This only affects few tests.
In Fedora spec-file it is worked around quite bluntly: https://src.fedoraproject.org/rpms/sssd/c/ca22aded04cabb6a71acb2b9271497b104...
"""
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-694296874
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
sumit-bose commented: """ Hi,
thanks for the patches. My tests with various configurations with multiple existing, not-existing or not responding DCs went well. Coverity didn't had anything to complain either.
While reading the patches I was wondering if it would make sense to create a macro for `"cldap"` since the string is used in a comparison? What do you think? This can be extended of course to the `"ldap"`, `"ldaps"` and `"ldapi"` strings when used to indicate an LDAP schema. Since this won't improve the readability I'm fine with keeping it as it is.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-698259812
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
sumit-bose commented: """ Hi,
maybe there should be some limit in the for-loop so that SSSD does not accidentally tries to ping hundreds of DC in a larger AD environment.
adcli implements a scheme described in https://winprotocoldoc.blob.core.windows.net/productionwindowsarchives/WinAr... section 5.4.5.3 where first 5 DC are pinged with a 0.4s timeout, then the next 5 are pinged with a 0.2s timeout and finally all other with a timeout of 0.1s. Unfortunately I haven't found any newer document how Windows clients are doing CLDAP pings.
bye, Sumit
"""
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-700577606
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
pbrezina commented: """ Do you mean there is 100ms timeout for the DC's to deliver a reply? That's quite low, isn't it? """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-700637011
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
sumit-bose commented: """
Do you mean there is 100ms timeout for the DC's to deliver a reply? That's quite low, isn't it?
If I understand the MFST document correctly the client still sends the pings sequentially and the timeout is the time beofre sending the next ping. I guess the client would still accept replies from other DC pinged earlier.
So it idea is more the other way round. The DCs are split in batches to not ping all DCs at once. Then give the first batch of DC a longer time to reply to increase the chance that the reply is received in that time, for the second batch wait already a bit shorter and for the rest even shorter to make sure that the waiting time does not depend too much on the number of DC.
bye, Sumit
"""
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-700770559
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
pbrezina commented: """ So do you suggest to implement 3 batches that are sent at time T (5 servers), T+400ms (5 servers), T+600ms (remainder) or to iterate over it sequentially with slow delay before going to next dc (to ping first five servers will take 400*5, the next five 200*5 and the remaining 100*X)? """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-700786640
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
sumit-bose commented: """
So do you suggest to implement 3 batches that are sent at time T (5 servers), T+400ms (5 servers), T+600ms (remainder) or to iterate over it sequentially with slow delay before going to next dc (to ping first five servers will take 400_5, the next five 200_5 and the remaining 100*X)?
Hi,
I was thinking of keeping the parallel pings but doing it in batches, so that in the typical/positive case we do not have to send pings to all DCs.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-700804289
URL: https://github.com/SSSD/sssd/pull/5300 Author: pbrezina Title: #5300: ad: use parallel cldap ping for site discovery Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5300/head:pr5300 git checkout pr5300
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
pbrezina commented: """ Done. I added `ad_cldap_ping_parallel_batch` and logic around it. """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-701338948
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
sumit-bose commented: """ Hi,
thanks for the changes, the patches are working well and with `debug_microseconds = True` one can actually see the time SSSD is waiting for replies for the batches.
To make sure people will understand in future why the batch sizes and waiting times were chosen this way I wonder if you can add a reference to [MS-DISO] section 5.4.5.3 to the comment?
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-702125724
URL: https://github.com/SSSD/sssd/pull/5300 Author: pbrezina Title: #5300: ad: use parallel cldap ping for site discovery Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5300/head:pr5300 git checkout pr5300
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
pbrezina commented: """ Done. I only changed comment in `ad_cldap_ping_parallel_batch`. """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-702138924
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
sumit-bose commented: """ Hi,
thanks, ACK.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-702188350
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
Label: +Ready to push
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
pbrezina commented: """ Pushed PR: https://github.com/SSSD/sssd/pull/5300
* `master` * f0d650799d4390f90890d17c56a4e395e931d8cb - tevent: correctly handle req timeout error * 9fdf5cfacd1a425691d44db53897096887bb3e6f - ad: renew site information only when SSSD was previously offline * a62a13ae61d4e08b21e706df6ca266c38891f430 - man: fix typo in failover description * fcfd834c9d80d7690f938582335d81231a5f6e60 - ad: if all in-site dc are unreachable try off-site controllers * 1889ca60a9c642f0cca60b20a5b94de7a66924f6 - ad: connect to the first available server for cldap ping * 8265674a055e5cdb57acebad72d935356408540a - ad: use cldap for site and forrest discover (perform CLDAP ping) * 414593cca65ed09fe4659e2786370a4553664cd0 - ldap: add support for cldap and udp connections
"""
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-702649279
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
Label: -Ready to push
URL: https://github.com/SSSD/sssd/pull/5300 Author: pbrezina Title: #5300: ad: use parallel cldap ping for site discovery Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5300/head:pr5300 git checkout pr5300
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
sumit-bose commented: """ Hi @pbrezina,
I came across an issue with `current_forest` and `current_site`. After some calls to `ad_cldap_ping_send()` with `renew_site == False` the value was just garbage. I guess something like
``` diff --git a/src/providers/ad/ad_cldap_ping.c b/src/providers/ad/ad_cldap_ping.c index cd23065..ac6f72d 100644 --- a/src/providers/ad/ad_cldap_ping.c +++ b/src/providers/ad/ad_cldap_ping.c @@ -597,8 +597,8 @@ struct tevent_req *ad_cldap_ping_send(TALLOC_CTX *mem_ctx, }
if (!srv_ctx->renew_site) { - state->site = srv_ctx->current_site; - state->forest = srv_ctx->current_forest; + state->site = talloc_strdup(state, srv_ctx->current_site); + state->forest = talloc_strdup(state, srv_ctx->current_forest); DEBUG(SSSDBG_TRACE_FUNC, "CLDAP ping is not necessary, using site '%s' and forest '%s'\n", state->site != NULL ? state->site : "unknown", ```
is needed to make sure that due to the `talloc_steal()` in the *_recv() function the values from `src_ctx` do not loose their parent.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-702837048
URL: https://github.com/SSSD/sssd/pull/5300 Title: #5300: ad: use parallel cldap ping for site discovery
alexey-tikhonov commented: """ JFTR, the above comment was addressed via 37ba37a425453d8222584176ae5975a795422091 """
See the full comment at https://github.com/SSSD/sssd/pull/5300#issuecomment-715261406
sssd-devel@lists.fedorahosted.org
-
abbra -
alexey-tikhonov
-
pbrezina
-
sumit-bose