URL: https://github.com/SSSD/sssd/pull/418 Author: fidencio Title: #418: CACHE_REQ: Copy the cr_domain list for each request Action: opened
PR body: """ Let's copy the cr_domain list for each request as this list may be free'd due to a refresh domains request.
This is a cheap solution to avoid the crash but a proper one should be implemented in the future, which is properly handling new domains being inserted and existing domains disappearing from the list.
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/418/head:pr418 git checkout pr418
URL: https://github.com/SSSD/sssd/pull/418 Author: fidencio Title: #418: CACHE_REQ: Copy the cr_domain list for each request Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/418/head:pr418 git checkout pr418
URL: https://github.com/SSSD/sssd/pull/418 Title: #418: CACHE_REQ: Copy the cr_domain list for each request
fidencio commented: """ I've updated the patch to copy the cr_domain inside cache_req_select_domains(), so we ensure we run the request in a updated list.
@pbrezina, could you take a look and see whether it's a valid approach? """
See the full comment at https://github.com/SSSD/sssd/pull/418#issuecomment-338158130
URL: https://github.com/SSSD/sssd/pull/418 Title: #418: CACHE_REQ: Copy the cr_domain list for each request
pbrezina commented: """ It is a valid approach and I don't have a problem with that. However, number of domain may be rather high so it would be better to use reference counter. """
See the full comment at https://github.com/SSSD/sssd/pull/418#issuecomment-338164976
URL: https://github.com/SSSD/sssd/pull/418 Title: #418: CACHE_REQ: Copy the cr_domain list for each request
lslebodn commented: """ Yes, we already have a reports about "leaks" in nss responder.
I assume that PR tries to fix https://bugzilla.redhat.com/show_bug.cgi?id=1494002 ? So you might reference upstream ticket https://pagure.io/SSSD/sssd/issue/3551 which was created few minutes ago """
See the full comment at https://github.com/SSSD/sssd/pull/418#issuecomment-338171814
URL: https://github.com/SSSD/sssd/pull/418 Title: #418: CACHE_REQ: Copy the cr_domain list for each request
jhrozek commented: """ Since even on IRC we agreed that @fidencio will investigate the reference counting, I'm setting the Changes Requested label for now.
If the refcount is not viable, feel free to remove the label. Nonetheless, we should (also as discussed on IRC) check if we can create a unit test for this bug.
With all this said, this patch might be good enough to give to the customer who was seeing the crashes at least as a way to checking if our suspicion about what is causing the crash is correct -- we haven't been able to gather any logs yet since the crash is quite intermittent. """
See the full comment at https://github.com/SSSD/sssd/pull/418#issuecomment-338199810
URL: https://github.com/SSSD/sssd/pull/418 Title: #418: CACHE_REQ: Copy the cr_domain list for each request
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/418 Author: fidencio Title: #418: CACHE_REQ: Copy the cr_domain list for each request Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/418/head:pr418 git checkout pr418
URL: https://github.com/SSSD/sssd/pull/418 Title: #418: CACHE_REQ: Copy the cr_domain list for each request
fidencio commented: """ We've had another round of discussions on IRC and I've decided to update the patch as it is, just adding the link to the pagure issue.
The reason behind this is that adding the reference counter to this code does not seen to bring a lot of benefits, but does bring some more complexity. I've a [branch](https://github.com/fidencio/sssd/tree/wip/domain_resolution_order_crash_refc...) where refcount is half-baked and half-broken and in case someone wants to take a look and chime in to help, I'd appreciate (maybe it'll make me change my mind).
Apart from this copying approach, the ref_count approach, there was a third option discussed that would be changing the cr_domains list instead of throwing it away and rebuilding it as we currently do. This option also does not bring a lot of benefits as, for safety reasons, we'd have to ensure that this list doesn't change during the cache req request (then we're back to the first 2 options).
So, my proposal here is to have it as it is and I'd like to hear the opinion of the other developers on this one (specially from @pbrezina who knows cache_req quite well; @jhrozek who remebers having some issues with our ref_count module; and @sumit-bose who came up with the suggestions in the first place).
I'm removing the "Changes Requested" label as from now on this PR is no longer blocked on me. """
See the full comment at https://github.com/SSSD/sssd/pull/418#issuecomment-338242063
URL: https://github.com/SSSD/sssd/pull/418 Title: #418: CACHE_REQ: Copy the cr_domain list for each request
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/418 Author: fidencio Title: #418: CACHE_REQ: Copy the cr_domain list for each request Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/418/head:pr418 git checkout pr418
URL: https://github.com/SSSD/sssd/pull/418 Author: fidencio Title: #418: CACHE_REQ: Copy the cr_domain list for each request Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/418/head:pr418 git checkout pr418
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
lslebodn
-
pbrezina