8000 Fix gcplogs memory/connection leak by phaas · Pull Request #41513 · moby/moby · GitHub
[go: up one dir, main page]

Skip to content

Conversation

phaas
Copy link
Contributor
@phaas phaas commented Sep 30, 2020

The cloud logging client should be closed when the log driver is closed. Otherwise dockerd will keep a gRPC connection to the logging endpoint open indefinitely.

This results in a slow leak of tcp sockets (1) and memory (~200Kb) any time that a container using --log-driver=gcplogs is terminates.

fixes #41512

Testing
Tested locally in docker dev environment, docker run --rm --log-driver=gcplogs hello-world no longer grows the connection count of dockerd.

- Description for the changelog
Fix gcplogs memory/connection leak

thaJeztah
thaJeztah previously approved these changes Sep 30, 2020
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.

LGTM, thanks!

@thaJeztah thaJeztah dismissed their stale review September 30, 2020 22:47

erm, doesn't seem to work

@thaJeztah
Copy link
Member

Actually, this doesn't compile;

 daemon/logger/gcplogs/gcplogging.go:240:17: l.logger.Close undefined (type *"github.com/docker/docker/vendor/cloud.google.com/go/logging".Logger has no field or method Close)

@thaJeztah thaJeztah added status/failing-ci Indicates that the PR in its current state fails the test suite and removed process/cherry-pick labels Sep 30, 2020
@phaas
Copy link
Contributor Author
phaas commented Sep 30, 2020

Ack. I'm figuring out the build/test process right now (first time working on this project). This method should exist according to the docs, though our vendored version may be behind.

@thaJeztah
Copy link
Member

I recalled I had a PR that updates the dependencies to a more recent version (may have to do a follow-up after that, because it stalled some time, so is now quite behind on current latest); I just rebased it to see if the CI issues on that one are resolved; #39838

@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Oct 1, 2020
@phaas
Copy link
Contributor Author
phaas commented Oct 1, 2020

Should be working now (got my local environment setup and verified the fix)

I had mixed up the internals (gcplogs creates a NewClient and then a Logger, discarding the former, while that's the object that requires closing)

@thaJeztah
Copy link
Member

Thanks! Could you perhaps squash the two commits?

The cloud logging client should be closed when the log driver is closed. Otherwise dockerd will keep a gRPC connection to the logging endpoint open indefinitely.

This results in a slow leak of tcp sockets (1) and memory (~200Kb) any time that a container using `--log-driver=gcplogs` is terminates.

Signed-off-by: Patrick Haas <patrickhaas@google.com>
@phaas
Copy link
Contributor Author
phaas commented Oct 1, 2020

Gladly. Squashed (and force pushed), hope everything looks ok still :)

< 8000 svg aria-label="Show options" role="img" height="16" viewBox="0 0 16 16" version="1.1" width="16" data-view-component="true" class="octicon octicon-kebab-horizontal"> 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.

LGTM if green 👍

@thaJeztah
Copy link
Member

@cpuguy83 ptal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gcplogs driver is leaking gRPC connections (cloud logger instances)
3 participants
0