8000 Fix rate limiting for logger, increase refill rate by emosbaugh · Pull Request #39360 · moby/moby · GitHub
[go: up one dir, main page]
Skip to content

Fix rate limiting for logger, increase refill rate #39360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 24, 2019
Merged

Fix rate limiting for logger, increase refill rate #39360

merged 1 commit into from
Jun 24, 2019

Conversation

emosbaugh
Copy link
Contributor
@emosbaugh em 8000 osbaugh commented Jun 12, 2019 •
edited
Loading

Signed-off-by: Ethan Mosbaugh ethan@replicated.com

- What I did

fixes #38640

- How I did it

The rate limiter will get refilled at a rate of 1 token per second. My understanding of the code it is intended to get refilled at a rate of 10M / second.

moby/vendor/golang.org/x/time/rate/rate.go

Lines 31 to 37 in 39c8e88

// A Limiter controls how frequently events are allowed to happen.
// It implements a "token bucket" of size b, initially full and refilled
// at rate r tokens per second.
// Informally, in any large enough time interval, the Limiter limits the
// rate to r tokens per second, with a maximum burst size of b events.
// As a special case, if r == Inf (the infinite rate), b is ignored.
// See https://en.wikipedia.org/wiki/Token_bucket for more about token buckets.

- How to verify it

$ docker service create --name lotsologs --restart-condition on-failure -d debian:stretch-slim bash -c "for i in \$(seq 1 60000); do cat /dev/urandom | tr -dc 'a-zA-Z0-9 ' | fold -w 256 | head -n 1; done"
y0uhc0ulyxcqe2r5gehfzeiks

wait a while for the logs to fill up

$ docker service logs lotsologs > tmp # hangs
^C
$ ls -lh tmp
-rw-rw-r-- 1 ethan ethan 12M Jun 12 21:07 tmp

- Description for the changelog

Fixed an issue that caused requests for docker swarm service and task logs to hang indefinitely for logs with size greater than 10 MB.

- A picture of a cute animal (not mandatory but encouraged)

Gizmo PNG

@emosbaugh
Fix rate limiting for logger, increase refill rate
50c6a5f 
Signed-off-by: Ethan Mosbaugh <ethan@replicated.com>
@thaJeztah
Copy link
Member

@dperny @kolyshkin ptal

@dperny
Copy link
Contributor
dperny commented Jun 14, 2019

Thank you for fixing this. I did not have a good first-glance understanding of the rate limiter and what was wrong with the implementation, and I had not yet prioritized fixing it. I appreciate you taking the time to understand and fix the problem, and including a good comment update to boot

LGTM. Test seem to be failing for unrelated reasons and can be rerun.

thaJeztah
thaJeztah approved these changes Jun 24, 2019
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

ping @cpuguy83 @kolyshkin PTAL

cpuguy83
cpuguy83 approved these changes Jun 24, 2019
View reviewed changes
Copy link
Member
@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Member

RS5 is broken currently; see #39402 and #39403

merging

@thaJeztah thaJeztah merged commit c2deb9f into moby:master Jun 24, 2019
@NicolasMahe NicolasMahe mentioned this pull request Nov 25, 2019
Closed
@a-abella
Copy link
a-abella commented Mar 5, 2020

Sorry to bump an old PR, but is this planned for release sometime soon? I believe I'm experiencing this issue on a recently built cluster (19.03.7).

@cpuguy83
Copy link
Member
cpuguy83 commented Mar 5, 2020

This could be backported to 19.03, I think.

@cpuguy83 cpuguy83 mentioned this pull request Mar 5, 2020
Merged
@cpuguy83
Copy link
Member
cpuguy83 commented Mar 5, 2020

Backport is here: #40628

@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
@a-abella
Copy link
a-abella commented May 26, 2020 •
edited
Loading

Just applied this fix to a couple clusters with 19.03.9 🎉🎉 Thank you! It's a big quality of life improvement!

EDIT: nope, still happening. #39360 (comment)
8000

@FranciscoKnebel
Copy link
FranciscoKnebel commented Aug 10, 2020 •
edited
Loading

Experiencing this on Docker 19.03.06, and after upgrading to 19.03.12 it remains.
3 replicas, each log individually is less than 1MB, still hangs.

Temp fix is just getting logs individually from each container.

@a-abella
Copy link

Same, it seemed better at first but then I noticed service logs hanging again. I had documented my experience with logs still hanging in the original closed issue: #38640 (comment)

@ohadasulin ohadasulin mentioned this pull request Dec 29, 2020
Closed
@JeFawk JeFawk mentioned this pull request Nov 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees
No one assigned
Labels
area/logging process/cherry-picked status/4-merge
Projects
None yet
Milestone
20.10.0
Development

Successfully merging this pull request may close these issues.

docker service logs hangs
7 participants
@emosbaugh @thaJeztah @dperny @a-abella @cpuguy83 @FranciscoKnebel @GordonTheTurtle
0