Skip to content

Fix rate limiting for logger, increase refill rate#39360

Merged
thaJeztah merged 1 commit intomoby:masterfrom
emosbaugh:logging-rate-limiter-bucket-fix
Jun 24, 2019
Merged

Fix rate limiting for logger, increase refill rate#39360
thaJeztah merged 1 commit intomoby:masterfrom
emosbaugh:logging-rate-limiter-bucket-fix

Conversation

@emosbaugh
Copy link
Copy Markdown
Contributor

@emosbaugh emosbaugh 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
Copy Markdown
Member

thaJeztah commented Jun 14, 2019

@dperny @kolyshkin ptal

@dperny
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

thaJeztah commented Jun 24, 2019

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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)

@FranciscoKnebel
Copy link
Copy Markdown

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
Copy Markdown

a-abella commented Aug 10, 2020

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