URL: https://github.com/SSSD/sssd/pull/143 Author: fidencio Title: #143: Explicitly add ordering dependency for the responders' sockets Action: opened
PR body: """ This patch set is intended to solve https://fedorahosted.org/sssd/ticket/3298 and more details can be find in the commit messages.
**SYSTEMD: Add "After=sssd.service" to the responders' sockets units** is the patch that actually solves the problem. **SYSTEMD: Add "WantedBy=sockets.target" to the responders' sockets units** is a desirable patch that slipped off during the development of the series and would be nice to have merged as well. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/143/head:pr143 git checkout pr143
URL: https://github.com/SSSD/sssd/pull/143 Author: fidencio Title: #143: Explicitly add ordering dependency for the responders' sockets Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/143/head:pr143 git checkout pr143
URL: https://github.com/SSSD/sssd/pull/143 Author: fidencio Title: #143: Explicitly add ordering dependency for the responders' sockets Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/143/head:pr143 git checkout pr143
URL: https://github.com/SSSD/sssd/pull/143 Author: fidencio Title: #143: Explicitly add ordering dependency for the responders' sockets Action: edited
Changed field: body Original value: """ This patch set is intended to solve https://fedorahosted.org/sssd/ticket/3298 and more details can be find in the commit messages.
**SYSTEMD: Add "After=sssd.service" to the responders' sockets units** is the patch that actually solves the problem. **SYSTEMD: Add "WantedBy=sockets.target" to the responders' sockets units** is a desirable patch that slipped off during the development of the series and would be nice to have merged as well. """
URL: https://github.com/SSSD/sssd/pull/143 Author: fidencio Title: #143: Explicitly add ordering dependency for the responders' sockets Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/143/head:pr143 git checkout pr143
URL: https://github.com/SSSD/sssd/pull/143 Author: fidencio Title: #143: Explicitly add ordering dependency for the responders' sockets Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/143/head:pr143 git checkout pr143
URL: https://github.com/SSSD/sssd/pull/143 Author: fidencio Title: #143: Explicitly add ordering dependency for the responders' sockets Action: edited
Changed field: body Original value: """ While debugging the whole breakage reported by Stric I've noticed that the NSS socket has been starting up the NSS responder _before_ SSSD being up. Leading us to a chaotic situation.
By adding this ordering explicitly we can avoid the reported situation.
Interesting that I haven't seen the same behaviour when starting/stopping the socket after the system is up.
I also haven't noticed any kind of problem caused by explicitly adding "After=sssd.service" to the unit files.
**EDITED: - This patch set previously consisted in two patches before but I ended up dropping the second one as I've noticed some issues about breaking the dependency cycle. """
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
jhrozek commented: """ I tested the patches both during the VM operation bringing different responders up and during system boot. I haven't see any issues.
ACK """
See the full comment at https://github.com/SSSD/sssd/pull/143#issuecomment-283026260
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
lslebodn commented: """ On (28/02/17 04:27), Jakub Hrozek wrote:
I tested the patches both during the VM operation bringing different responders up and during system boot. I haven't see any issues.
A tiny question. Do we need to add After to all sockets? The ideal would be that socket will trigger starting of responder and monitor will be started as a dependency of responder.
I thought it is required just for nss due to some circular dependency.
I would appreciate explanation. So if someone decide to remove it in future he will consider problematic use-case.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/143#issuecomment-283069714
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
fidencio commented: """ On Tue, Feb 28, 2017 at 4:25 PM, lslebodn notifications@github.com wrote:
On (28/02/17 04:27), Jakub Hrozek wrote:
I tested the patches both during the VM operation bringing different
responders up and during system boot. I haven't see any issues.
A tiny question. Do we need to add After to all sockets? The ideal would be that socket will trigger starting of responder and monitor will be started as a dependency of responder.
I thought it is required just for nss due to some circular dependency.
I would appreciate explanation. So if someone decide to remove it in future he will consider problematic use-case.
Lukáš,
That was a recommendation of Lukáš Nykrýn that BindsTo and After must go together. It's mentioned in the commit message. Lukáš Nykrýn also mentioned they will fix their docs about this.
Best Regards, -- Fabiano Fidêncio
"""
See the full comment at https://github.com/SSSD/sssd/pull/143#issuecomment-283077512
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
jhrozek commented: """ On Tue, Feb 28, 2017 at 07:51:26AM -0800, fidencio wrote:
On Tue, Feb 28, 2017 at 4:25 PM, lslebodn notifications@github.com wrote:
On (28/02/17 04:27), Jakub Hrozek wrote:
I tested the patches both during the VM operation bringing different
responders up and during system boot. I haven't see any issues.
A tiny question. Do we need to add After to all sockets? The ideal would be that socket will trigger starting of responder and monitor will be started as a dependency of responder.
I thought it is required just for nss due to some circular dependency.
I would appreciate explanation. So if someone decide to remove it in future he will consider problematic use-case.
Lukáš,
That was a recommendation of Lukáš Nykrýn that BindsTo and After must go together. It's mentioned in the commit message. Lukáš Nykrýn also mentioned they will fix their docs about this.
I think Lukas was asking about something else -- would it be enough to only add those changes to the nss socket file?
"""
See the full comment at https://github.com/SSSD/sssd/pull/143#issuecomment-283155833
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
fidencio commented: """ On Tue, Feb 28, 2017 at 9:47 PM, Jakub Hrozek notifications@github.com wrote:
On Tue, Feb 28, 2017 at 07:51:26AM -0800, fidencio wrote:
On Tue, Feb 28, 2017 at 4:25 PM, lslebodn notifications@github.com
wrote:
On (28/02/17 04:27), Jakub Hrozek wrote:
I tested the patches both during the VM operation bringing different
responders up and during system boot. I haven't see any issues.
A tiny question. Do we need to add After to all sockets? The ideal would be that socket will trigger starting of responder and monitor will be started as a dependency of responder.
I thought it is required just for nss due to some circular dependency.
I would appreciate explanation. So if someone decide to remove it in
future
he will consider problematic use-case.
Lukáš,
That was a recommendation of Lukáš Nykrýn that BindsTo and After must go together. It's mentioned in the commit message. Lukáš Nykrýn also mentioned they will fix their docs about this.
I think Lukas was asking about something else -- would it be enough to only add those changes to the nss socket file?
I've talked to Lukáš on the office before replying the email :-) Anyways, no, it won't be enough. BindsTo and After *must* *go* *together*. We use BindsTo on all services' sockets and not using After as well may lead to unexpected behaviour.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/143#issuecomment-283155833, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4ekmc6BgOlXlv7YWf66QmyxbNv09kks5rhIfvgaJpZM4L3N4p .
"""
See the full comment at https://github.com/SSSD/sssd/pull/143#issuecomment-283158870
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
lslebodn commented: """ On (28/02/17 13:01), fidencio wrote:
I've talked to Lukáš on the office before replying the email :-) Anyways, no, it won't be enough. BindsTo and After *must* *go* *together*. We use BindsTo on all services' sockets and not using After as well may lead to unexpected behaviour.
The main problem is that commit message is not clear enough and contain unclear statemets e.g.
While debugging the whole breakage reported by Stric I've noticed that the NSS socket has been starting up the NSS responder _before_ SSSD being up, leading us to a chaotic situation.
What does chaotic situation mean here?
There is also a missing context about "BindsTo".
By adding this ordering explicitly we can avoid the reported situation. Also, it has been recommend by Lukáš Nykrýn that BindsTo and After must be used together (although it's still not mentioned yet in the systemd documentation).
IIUC "BindsTo" is a stricter relation between units then "Requires" and therefore "After" need to be used to avoid unuexpected/undefined behaviour.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/143#issuecomment-283279883
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
fidencio commented: """ On Wed, Mar 1, 2017 at 9:46 AM, lslebodn notifications@github.com wrote:
On (28/02/17 13:01), fidencio wrote:
I've talked to Lukáš on the office before replying the email :-) Anyways, no, it won't be enough. BindsTo and After *must* *go* *together*. We use BindsTo on all services' sockets and not using After as well may lead to unexpected behaviour.
The main problem is that commit message is not clear enough and contain unclear statemets e.g.
While debugging the whole breakage reported by Stric I've noticed that the NSS socket has been starting up the NSS responder _before_ SSSD being up, leading us to a chaotic situation.
What does chaotic situation mean here?
As far as I remember, that some services like systemd-logind will try to start and will time out. It will cause other services to timeout. So the boot will take a really long time and so services won't be working after the boot is completed.
Would be enough adding this info? If not, may I ask you for some suggestion as well?
There is also a missing context about "BindsTo".
By adding this ordering explicitly we can avoid the reported situation. Also, it has been recommend by Lukáš Nykrýn that BindsTo and After must be used together (although it's still not mentioned yet in the systemd documentation).
IIUC "BindsTo" is a stricter relation between units then "Requires" and therefore "After" need to be used to avoid unuexpected/undefined behaviour.
I added something similar to your suggestion to the commit message. Thanks.
Best Regards, -- Fabiano Fidêncio
"""
See the full comment at https://github.com/SSSD/sssd/pull/143#issuecomment-283289617
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
jhrozek commented: """ On Wed, Mar 01, 2017 at 01:30:15AM -0800, fidencio wrote:
On Wed, Mar 1, 2017 at 9:46 AM, lslebodn notifications@github.com wrote:
On (28/02/17 13:01), fidencio wrote:
I've talked to Lukáš on the office before replying the email :-) Anyways, no, it won't be enough. BindsTo and After *must* *go* *together*. We use BindsTo on all services' sockets and not using After as well may lead to unexpected behaviour.
The main problem is that commit message is not clear enough and contain unclear statemets e.g.
While debugging the whole breakage reported by Stric I've noticed that the NSS socket has been starting up the NSS responder _before_ SSSD being up, leading us to a chaotic situation.
What does chaotic situation mean here?
As far as I remember, that some services like systemd-logind will try to start and will time out. It will cause other services to timeout. So the boot will take a really long time and so services won't be working after the boot is completed.
This is probably caused by libc doing initgroups on pretty much any account. Since initgroups must check all NSS modules in order to be precise and talking to nss_sss would trigger talking to the NSS responder. Then, if the account wasn't from SSSD, the NSS responder would try talking to the Data Provider which is not up yet, because SSSD is not up, so the whole process hangs until libc gives up which takes 2 minutes by default.
Would be enough adding this info? If not, may I ask you for some suggestion as well?
There is also a missing context about "BindsTo".
By adding this ordering explicitly we can avoid the reported situation. Also, it has been recommend by Lukáš Nykrýn that BindsTo and After must be used together (although it's still not mentioned yet in the systemd documentation).
IIUC "BindsTo" is a stricter relation between units then "Requires" and therefore "After" need to be used to avoid unuexpected/undefined behaviour.
I added something similar to your suggestion to the commit message. Thanks.
Best Regards,
Fabiano Fidêncio
-- You are receiving this because you were assigned. Reply to this email directly or view it on GitHub: https://github.com/SSSD/sssd/pull/143#issuecomment-283289617
"""
See the full comment at https://github.com/SSSD/sssd/pull/143#issuecomment-283292747
URL: https://github.com/SSSD/sssd/pull/143 Author: fidencio Title: #143: Explicitly add ordering dependency for the responders' sockets Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/143/head:pr143 git checkout pr143
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
fidencio commented: """ New patch set has been pushed. """
See the full comment at https://github.com/SSSD/sssd/pull/143#issuecomment-283298505
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
lslebodn commented: """ master:
* bd5e09bad2b0ac8a7ca78f45d90c8ebb903efaa3 * 0adcf95a423155f9c9d8062af42561402ceab4cc """
See the full comment at https://github.com/SSSD/sssd/pull/143#issuecomment-283311920
URL: https://github.com/SSSD/sssd/pull/143 Title: #143: Explicitly add ordering dependency for the responders' sockets
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/143 Author: fidencio Title: #143: Explicitly add ordering dependency for the responders' sockets Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/143/head:pr143 git checkout pr143
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
lslebodn