[sssd PR#327][opened] RESPONDERS: Fix terminating idle connections

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/327
Author: jhrozek
 Title: #327: RESPONDERS: Fix terminating idle connections
Action: opened

PR body:
"""
The client_idle_handler() function tried to schedule another tevent timer
to check for idle client connections in case the current connection was
still valid, but in doing so, it also stored the current time into the
last_request_time field of the client context.

This kept the connection always alive, because the last_request_time could
then never be older than the timeout.

This patch changes the setup_client_idle_timer() function to only do what
the synopsis says and set the idle timer. The caller (usually the function
that accepts the connection) is supposed to store the request time itself.

To test, revert the patch with the code fix and run the integration test
in the other patch.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/327/head:pr327
git checkout pr327

[sssd PR#327][synchronized] RESPONDERS: Fix terminating idle connections

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/327
Author: jhrozek
 Title: #327: RESPONDERS: Fix terminating idle connections
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/327/head:pr327
git checkout pr327

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

fidencio commented:
"""
Okay, I'm acking the patchset, feel free to follow or not my comments (as I said, no
strong feelings about those) ...
"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316434169

[sssd PR#327][+Accepted] RESPONDERS: Fix terminating idle connections

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

Label: +Accepted

[sssd PR#327][-Accepted] RESPONDERS: Fix terminating idle connections

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

Label: -Accepted

[sssd PR#327][+Changes requested] RESPONDERS: Fix terminating idle connections

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

Label: +Changes requested

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

lslebodn commented:
"""
On (19/07/17 09:28), fidencio wrote:
> fidencio commented on this pull request.
> > @@ -100,11 +100,11 @@ def setup_for_secrets(request):
>          [secrets]
>          max_secrets = 10
>          max_payload_size = 2
> +        client_idle_timeout = 10
>
> @lslebodn: I'm just quoting the commit message as I do believe it answers your
> question about the 10 seconds: 
> "The client timeout in the test has to be at least 10 seconds because
> internally, the responders don't allow a shorter timeout."
>  
I checked only man page and that's not mention there.
And it still does not explain why do we need
to wait 50% longer then `client_idle_timeout`

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316449831

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[Fabiano Fidêncio]

On Wed, Jul 19, 2017 at 6:56 PM, lslebodn
<sssd-github-notification(a)fedorahosted.org&gt; wrote:
>    URL: https://github.com/SSSD/sssd/pull/327
>  Title: #327: RESPONDERS: Fix terminating idle connections
>
>  lslebodn commented:
>  """
>  On (19/07/17 09:28), fidencio wrote:
> >fidencio commented on this pull request.
> >> @@ -100,11 +100,11 @@ def setup_for_secrets(request):
> >         [secrets]
> >         max_secrets = 10
> >         max_payload_size = 2
> >+        client_idle_timeout = 10
> >
> >@lslebodn: I'm just quoting the commit message as I do believe it answers your
> question about the 10 seconds:
> >"The client timeout in the test has to be at least 10 seconds because
> >internally, the responders don't allow a shorter timeout."
> >
>  I checked only man page and that's not mention there.
>  And it still does not explain why do we need
>  to wait 50% longer then `client_idle_timeout` 
The reason we have to wait 50% longer than the `client_idle_timeout`
can be find here (please, see the commit message):
https://github.com/SSSD/sssd/commit/560daa14ef013aa14e2aedeea10b07f623d84ec8

And the commit that introduced that the client_idle_timeout has the
minimum value of 10 seconds is this one:
https://github.com/SSSD/sssd/commit/bb79e7559dae451a14150377099e32d6b5159a6c

Hopefully everything is clear now.

>
> LS
>
> """
>
> See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316449831
>
> _______________________________________________
> sssd-devel mailing list -- sssd-devel(a)lists.fedorahosted.org
> To unsubscribe send an email to sssd-devel-leave(a)lists.fedorahosted.org
>

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

fidencio commented:
"""
On Wed, Jul 19, 2017 at 6:56 PM, lslebodn <notifications(a)github.com&gt; wrote:

>  On (19/07/17 09:28), fidencio wrote:
>  >fidencio commented on this pull request.
>  >> @@ -100,11 +100,11 @@ def setup_for_secrets(request):
>  > [secrets]
>  > max_secrets = 10
>  > max_payload_size = 2
>  >+ client_idle_timeout = 10
>  >
>  >@lslebodn: I'm just quoting the commit message as I do believe it answers
>  your question about the 10 seconds:
>  >"The client timeout in the test has to be at least 10 seconds because
>  >internally, the responders don't allow a shorter timeout."
>  >
>  I checked only man page and that's not mention there.
>  And it still does not explain why do we need
>  to wait 50% longer then `client_idle_timeout`
>  
The reason we have to wait 50% longer than the `client_idle_timeout`
can be find here (please, see the commit message):
https://github.com/SSSD/sssd/commit/560daa14ef013aa14e2aedeea10b07f623d84ec8

And the commit that introduced that the client_idle_timeout has the
minimum value of 10 seconds is this one:
https://github.com/SSSD/sssd/commit/bb79e7559dae451a14150377099e32d6b5159a6c

Hopefully everything is clear now.

(Sorry for the duplicated answer, but in the first time I did reply only
for sssd-devel).


>  LS
>
>  —
>  You are receiving this because you were assigned.
>  Reply to this email directly, view it on GitHub
>  <https://github.com/SSSD/sssd/pull/327#issuecomment-316449831>;, or mute
>  the thread
>
> <https://github.com/notifications/unsubscribe-auth/AAG4ercHvLn88DRfoppCfQ3...
>  .
>  
"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316602028

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

lslebodn commented:
"""
On (19/07/17 22:38), fidencio wrote:
> On Wed, Jul 19, 2017 at 6:56 PM, lslebodn
> <notifications(a)github.com&gt; wrote:
>
> > On (19/07/17 09:28), fidencio wrote:
> > >fidencio commented on this pull request.
> > >> @@ -100,11 +100,11 @@ def setup_for_secrets(request):
> > > [secrets]
> > > max_secrets = 10
> > > max_payload_size = 2
> > >+ client_idle_timeout = 10
> > >
> > >@lslebodn: I'm just quoting the commit message as I do believe it answers
> > your question about the 10 seconds:
> > >"The client timeout in the test has to be at least 10 seconds because
> > >internally, the responders don't allow a shorter timeout."
> > >
> > I checked only man page and that's not mention there.
> > And it still does not explain why do we need
> > to wait 50% longer then `client_idle_timeout`
> >
>
> The reason we have to wait 50% longer than the `client_idle_timeout`
> can be find here (please, see the commit message):
> https://github.com/SSSD/sssd/commit/560daa14ef013aa14e2aedeea10b07f623d84ec8
>
> And the commit that introduced that the client_idle_timeout has the
> minimum value of 10 seconds is this one:
> https://github.com/SSSD/sssd/commit/bb79e7559dae451a14150377099e32d6b5159a6c
>
> Hopefully everything is clear now.
>  
A) It is not clear because it is not documented.
B) I do not see a reason why minimal value have to be 10 seconds.
   why not 11, 13, 17, 19 or any other value.

I do not see a reason why we should wait 15 seconds in test.
It is wasting of time without reasonable explanation anywhere.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316624405

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

jhrozek commented:
"""
On Thu, Jul 20, 2017 at 07:45:26AM +0000, lslebodn wrote:
>  On (19/07/17 22:38), fidencio wrote:
>  >On Wed, Jul 19, 2017 at 6:56 PM, lslebodn <notifications(a)github.com&gt; wrote:
>  >
>  >> On (19/07/17 09:28), fidencio wrote:
>  >> >fidencio commented on this pull request.
>  >> >> @@ -100,11 +100,11 @@ def setup_for_secrets(request):
>  >> > [secrets]
>  >> > max_secrets = 10
>  >> > max_payload_size = 2
>  >> >+ client_idle_timeout = 10
>  >> >
>  >> >@lslebodn: I'm just quoting the commit message as I do believe it
> answers
>  >> your question about the 10 seconds:
>  >> >"The client timeout in the test has to be at least 10 seconds because
>  >> >internally, the responders don't allow a shorter timeout."
>  >> >
>  >> I checked only man page and that's not mention there.
>  >> And it still does not explain why do we need
>  >> to wait 50% longer then `client_idle_timeout`
>  >>
>  >
>  >The reason we have to wait 50% longer than the `client_idle_timeout`
>  >can be find here (please, see the commit message):
>  >https://github.com/SSSD/sssd/commit/560daa14ef013aa14e2aedeea10b07f623d84ec8
>  >
>  >And the commit that introduced that the client_idle_timeout has the
>  >minimum value of 10 seconds is this one:
>  >https://github.com/SSSD/sssd/commit/bb79e7559dae451a14150377099e32d6b5159a6c
>  >
>  >Hopefully everything is clear now.
>  >
>  A) It is not clear because it is not documented. 
I can submit a PR that fixes the manpage, but it suprises me that you're
still surprised that our documentation is missing something :)

>  B) I do not see a reason why minimal value have to be 10 seconds.
>     why not 11, 13, 17, 19 or any other value. 
Because I wasn't sure if 11 wouldn't cause false positives (which
ultimately waste more time b/c someone has to look at them)

But if you prefer, I can lower that to 11. I can't lower that below 10
though.

"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316628352

[sssd PR#327][synchronized] RESPONDERS: Fix terminating idle connections

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/327
Author: jhrozek
 Title: #327: RESPONDERS: Fix terminating idle connections
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/327/head:pr327
git checkout pr327

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

jhrozek commented:
"""
OK, new patches attached.
"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316633406

[sssd PR#327][-Changes requested] RESPONDERS: Fix terminating idle connections

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

Label: -Changes requested

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

lslebodn commented:
"""
On (20/07/17 08:03), Jakub Hrozek wrote:
> > A) It is not clear because it is not documented.
>
> I can submit a PR that fixes the manpage, but it suprises me that you're
> still surprised that our documentation is missing something :)
>  
I consider integration test as an additional documentation.
It was not clear to me why we used such values in the test.
That's the first problem. So I tried to check man page.
And it didn't help either.

> > B) I do not see a reason why minimal value have to be 10
> seconds.
> >    why not 11, 13, 17, 19 or any other value.
>
> Because I wasn't sure if 11 wouldn't cause false positives (which
> ultimately waste more time b/c someone has to look at them)
>
> But if you prefer, I can lower that to 11. I can't lower that below 10
> though. 
Use whatever value would work reliably but at least document why
you used such hardcoded value. People tend to forget and we might
not know in future why such value was used and whether it can be changed.

And I still would prefer faster test. But that can be done after 1.15.3

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316638620

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

jhrozek commented:
"""
On Thu, Jul 20, 2017 at 08:46:04AM +0000, lslebodn wrote:
>  On (20/07/17 08:03), Jakub Hrozek wrote:
>  >> A) It is not clear because it is not documented.
>  >
>  >I can submit a PR that fixes the manpage, but it suprises me that you're
>  >still surprised that our documentation is missing something :)
>  >
>  
>  I consider integration test as an additional documentation.
>  It was not clear to me why we used such values in the test.
>  That's the first problem. So I tried to check man page.
>  And it didn't help either.
>  
>  >> B) I do not see a reason why minimal value have to be 10 seconds.
>  >>    why not 11, 13, 17, 19 or any other value.
>  >
>  >Because I wasn't sure if 11 wouldn't cause false positives (which
>  >ultimately waste more time b/c someone has to look at them)
>  >
>  >But if you prefer, I can lower that to 11. I can't lower that below 10
>  >though.
>  Use whatever value would work reliably but at least document why
>  you used such hardcoded value. People tend to forget and we might
>  not know in future why such value was used and whether it can be changed.
>  
>  And I still would prefer faster test. But that can be done after 1.15.3 
Feel free to file a ticket to remove this limitation in tests. But I
think for real-world, the 10 seconds minimum makes sense (even though,
we probably shouldn't try to protect our users from shooting themselves
in the foot /too hard/..)

"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316639773

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

lslebodn commented:
"""
Setting change requested due to previous comments
* unexplained hardcoded values in tests
* changing `client_idle_timeout` for all tests instead of using custom configuration just
for test for regression https://pagure.io/SSSD/sssd/issue/3448
"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316640339

[sssd PR#327][+Changes requested] RESPONDERS: Fix terminating idle connections

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

Label: +Changes requested

[sssd PR#327][synchronized] RESPONDERS: Fix terminating idle connections

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/327
Author: jhrozek
 Title: #327: RESPONDERS: Fix terminating idle connections
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/327/head:pr327
git checkout pr327

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

jhrozek commented:
"""
On Thu, Jul 20, 2017 at 01:53:01AM -0700, lslebodn wrote:
>  Setting change requested due to previous comments
>  * unexplained hardcoded values in tests
>  * changing `client_idle_timeout` for all tests instead of using custom configuration just
> for test for regression https://pagure.io/SSSD/sssd/issue/3448 
All two should be addressed in the current PR.

"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-316654957

[sssd PR#327][-Changes requested] RESPONDERS: Fix terminating idle connections

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

Label: -Changes requested

[sssd PR#327][synchronized] RESPONDERS: Fix terminating idle connections

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/327
Author: jhrozek
 Title: #327: RESPONDERS: Fix terminating idle connections
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/327/head:pr327
git checkout pr327

[sssd PR#327][comment] RESPONDERS: Fix terminating idle connections

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

lslebodn commented:
"""
master:
* 74e2415f00d0cb27ae935c7d81aaf15d906cc927
* d24335e9bd611e6626a57da28ad8ed621bb473c1
* 422217c7ea5fbe94b386c93c19e2c1928151faa0
"""

See the full comment at https://github.com/SSSD/sssd/pull/327#issuecomment-317066662

[sssd PR#327][closed] RESPONDERS: Fix terminating idle connections

[lslebodn]

   URL: https://github.com/SSSD/sssd/pull/327
Author: jhrozek
 Title: #327: RESPONDERS: Fix terminating idle connections
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/327/head:pr327
git checkout pr327

[sssd PR#327][+Pushed] RESPONDERS: Fix terminating idle connections

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/327
Title: #327: RESPONDERS: Fix terminating idle connections

Label: +Pushed