Fix empty LogPath with non-blocking logging mode #36272

mnussbaum · 2018-02-10T00:14:21Z

- What I did

This fixes #36255, an issue where the container LogPath was empty when the non-blocking logging mode was enabled. Prior to this change docker inspect returned an empty string for LogPath. This caused issues with tools that rely on docker inspect output to discover container logs, e.g. Kubernetes.

This PR also removes some LogPath methods that I believe are now unnecessary and are never invoked.

This PR was co-authored by @JunzeHe.

- How I did it

This change sets the LogPath on container structs as soon as the path is generated, instead of setting the LogPath on a logger struct and then attempting to pull it off that logger at a later point. That attempt to pull the LogPath off the logger was error prone since it assumed that the logger would only ever be a single type.

- How to verify it

docker run --name test-log-path --log-opt mode=non-blocking -d debian sleep infinity
docker inspect test-log-path | grep LogPath
# Observe the LogPath output of inspect is not an empty string, as it was before this change

- Description for the changelog

Fix empty LogPath with non-blocking logging mode

mnussbaum · 2018-02-13T02:02:23Z

It looks like my new test is failing under the Windows build. I've tried to run the test suite under Windows, but haven't been able to successfully kick it off. When I follow the instructions for building with Windows using Docker for Windows I get as far as:

docker build -t nativebuildimage -f Dockerfile.windows .

But it fails with the error "windows" cannot be used on this platform, and it does seem that the Docker server believes it's running under Linux.

Any suggestions for debugging the failing Windows build?

EDIT: I missed a step in the windows setup doc, I can hopefully make progress on this now

mnussbaum · 2018-02-14T17:26:19Z

I've just managed to get the all the builds passing and I think this PR is ready for review!

cpuguy83 · 2018-02-16T21:00:21Z

container/container_unit_test.go

@@ -66,3 +69,59 @@ func TestContainerSecretReferenceDestTarget(t *testing.T) {
 		t.Fatalf("expected secret dest %q; received %q", expected, d)
 	}
 }
+
+func TestContainerLogPathSetForFileLoggers(t *testing.T) {


As much as I'd prefer a unit test, I think this one is more appropriate as an integration test.
@dnephin WDYT?

I think a unit test is appropriate here. I really think we should only be writing integration tests for features, not bug fixes.

Works for me.

dnephin

You could use testify/require

dnephin · 2018-02-16T21:10:27Z

container/container_unit_test.go

@@ -66,3 +69,59 @@ func TestContainerSecretReferenceDestTarget(t *testing.T) {
 		t.Fatalf("expected secret dest %q; received %q", expected, d)
 	}
 }
+
+func TestContainerLogPathSetForFileLoggers(t *testing.T) {


I think a unit test is appropriate here. I really think we should only be writing integration tests for features, not bug fixes.

dnephin · 2018-02-16T21:10:49Z

container/container_unit_test.go

+		t.Fatalf("Expected LogPath for JSON file logger to be %s, got %s", expectedLogPath, c.LogPath)
+	}
+
+	c = &Container{


I would split this into two cases here, one for each logger type.

mnussbaum · 2018-02-16T21:53:00Z

Thanks for the feedback so far! I've amended the commit to use testify and include separate tests for each of the file-based logger types. Happy to make any more changes you'd like to see.

codecov · 2018-02-17T00:14:35Z

Codecov Report

Merging #36272 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #36272      +/-   ##
==========================================
+ Coverage   34.34%   34.41%   +0.06%     
==========================================
  Files         609      611       +2     
  Lines       45306    45493     +187     
==========================================
+ Hits        15562    15655      +93     
- Misses      27737    27819      +82     
- Partials     2007     2019      +12

Impacted Files	Coverage Δ
daemon/logger/loggerutils/logfile.go	`0% <ø> (ø)`	⬆️
daemon/logger/jsonfilelog/jsonfilelog.go	`57.14% <ø> (+1.22%)`	⬆️
container/container.go	`14.25% <100%> (+4.45%)`	⬆️
pkg/discovery/file/file.go	`90.38% <0%> (-5.77%)`	⬇️
builder/fscache/fscache.go	`58.84% <0%> (-0.58%)`	⬇️
vendor/github.com/docker/distribution/registry.go	`52.08% <0%> (ø)`
vendor/github.com/docker/distribution/errors.go	`21.42% <0%> (ø)`
daemon/graphdriver/devmapper/deviceset.go	`39.85% <0%> (+0.2%)`	⬆️
daemon/logger/awslogs/cloudwatchlogs.go	`72.83% <0%> (+0.3%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3831a6...20ca612. Read the comment docs.

dnephin

LGTM

cpuguy83 · 2018-02-20T16:55:15Z

container/container_unit_test.go

+
+func TestContainerLogPathSetForRingLogger(t *testing.T) {
+	rootPath := filepath.Join(string(filepath.Separator), "root")
+	os.MkdirAll(filepath.Join(rootPath, "root"), os.ModePerm)


Why not ioutil.TempDir()? This looks like it's creating a test dir in /root/root?

I've just amended the commit to use a tempdir. I originally struggled a bit to put together a path that satisfied the requirements of symlink.FollowSymlinkInScope under the Windows build, which is how I ended up with the weird root path.

My access to a Windows development environment is unfortunately pretty constrained, so we'll have to see if this tempdir path satisfies the Windows build on CI now.

Good news, it looks like the build passed using a TempDir. Let me know if there's any other changes you'd like here

This fixes an issue where the container LogPath was empty when the non-blocking logging mode was enabled. This change sets the LogPath on the container as soon as the path is generated, instead of setting the LogPath on a logger struct and then attempting to pull it off that logger at a later point. That attempt to pull the LogPath off the logger was error prone since it assumed that the logger would only ever be a single type. Prior to this change docker inspect returned an empty string for LogPath. This caused issues with tools that rely on docker inspect output to discover container logs, e.g. Kubernetes. This commit also removes some LogPath methods that are now unnecessary and are never invoked. Signed-off-by: junzhe and mnussbaum <code@getbraintree.com>

cpuguy83

LGTM

cpuguy83 · 2018-02-27T16:26:27Z

container/container_unit_test.go

@@ -66,3 +70,52 @@ func TestContainerSecretReferenceDestTarget(t *testing.T) {
 		t.Fatalf("expected secret dest %q; received %q", expected, d)
 	}
 }
+
+func TestContainerLogPathSetForJSONFileLogger(t *testing.T) {
+	containerRoot, err := ioutil.TempDir("", "TestContainerLogPathSetForJSONFileLogger")


Oh sorry... premature merge...
This needs to be cleaned up in a defer.

defer os.RemoveAll(containerRoot)

Can you open a PR to fix that?
Thanks!

Opened a cleanup PR at #36429

mnussbaum · 2018-02-27T19:12:09Z

Thanks for merging this!

mnussbaum · 2018-02-27T19:15:55Z

One question, @cpuguy83 or @dnephin, do you know what moby release we can expect to see this change in, and when that might go out?

cpuguy83 · 2018-02-27T19:22:22Z

@mnussbaum Moby doesn't have releases. Docker does releases and typically has a monthly release for the edge channel and a quarterly release for the stable channel.

mnussbaum · 2018-02-27T19:34:15Z

Gotcha, thanks. I should expect this work to land in the next Docker edge release then?

cpuguy83 · 2018-02-27T19:37:15Z

This would likely land in the April edge release.

GordonTheTurtle added the status/0-triage label Feb 10, 2018

mnussbaum mentioned this pull request Feb 10, 2018

Non-blocking logging causes container LogPath to be an empty string #36255

Closed

mnussbaum force-pushed the 36255-fix_log_path branch from 4a36a90 to eb34d2f Compare February 10, 2018 00:48

yongtang added status/2-code-review and removed status/0-triage labels Feb 11, 2018

mnussbaum force-pushed the 36255-fix_log_path branch 2 times, most recently from 68ad9d6 to 448c1e8 Compare February 12, 2018 18:34

mnussbaum force-pushed the 36255-fix_log_path branch 3 times, most recently from 4aee1cf to 754607d Compare February 14, 2018 06:14

cpuguy83 reviewed Feb 16, 2018

View reviewed changes

dnephin reviewed Feb 16, 2018

View reviewed changes

mnussbaum force-pushed the 36255-fix_log_path branch from 754607d to 719c312 Compare February 16, 2018 21:52

mnussbaum force-pushed the 36255-fix_log_path branch from 719c312 to 11143ee Compare February 17, 2018 00:14

thaJeztah added area/logging area/api labels Feb 19, 2018

dnephin approved these changes Feb 20, 2018

View reviewed changes

cpuguy83 requested changes Feb 20, 2018

View reviewed changes

mnussbaum force-pushed the 36255-fix_log_path branch 5 times, most recently from 5efa65e to bc0958c Compare February 21, 2018 04:41

mnussbaum force-pushed the 36255-fix_log_path branch from bc0958c to 20ca612 Compare February 21, 2018 07:13

GordonTheTurtle assigned johnstep Feb 25, 2018

cpuguy83 approved these changes Feb 27, 2018

View reviewed changes

cpuguy83 merged commit a1afe38 into moby:master Feb 27, 2018

cpuguy83 reviewed Feb 27, 2018

View reviewed changes

mnussbaum mentioned this pull request Feb 27, 2018

Clean up a temp dir created in container unit test #36429

Merged

thaJeztah mentioned this pull request Apr 17, 2018

[18.03] backport updates to tests, and fix empty LogPath with non-blocking logging mode docker-archive/docker-ce#512

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix empty LogPath with non-blocking logging mode #36272

Fix empty LogPath with non-blocking logging mode #36272

mnussbaum commented Feb 10, 2018

mnussbaum commented Feb 13, 2018 •

edited

Loading

mnussbaum commented Feb 14, 2018

cpuguy83 Feb 16, 2018

dnephin Feb 16, 2018

cpuguy83 Feb 16, 2018

dnephin left a comment

dnephin Feb 16, 2018

dnephin Feb 16, 2018

mnussbaum commented Feb 16, 2018 •

edited

Loading

codecov bot commented Feb 17, 2018 •

edited

Loading

dnephin left a comment

cpuguy83 Feb 20, 2018

mnussbaum Feb 20, 2018 •

edited

Loading

mnussbaum Feb 26, 2018 •

edited

Loading

cpuguy83 left a comment

cpuguy83 Feb 27, 2018

mnussbaum Feb 27, 2018

mnussbaum commented Feb 27, 2018

mnussbaum commented Feb 27, 2018

cpuguy83 commented Feb 27, 2018

mnussbaum commented Feb 27, 2018

cpuguy83 commented Feb 27, 2018

Fix empty LogPath with non-blocking logging mode #36272

Fix empty LogPath with non-blocking logging mode #36272

Conversation

mnussbaum commented Feb 10, 2018

mnussbaum commented Feb 13, 2018 • edited Loading

mnussbaum commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnussbaum commented Feb 16, 2018 • edited Loading

codecov bot commented Feb 17, 2018 • edited Loading

Codecov Report

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnussbaum Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

mnussbaum Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnussbaum commented Feb 27, 2018

mnussbaum commented Feb 27, 2018

cpuguy83 commented Feb 27, 2018

mnussbaum commented Feb 27, 2018

cpuguy83 commented Feb 27, 2018

mnussbaum commented Feb 13, 2018 •

edited

Loading

mnussbaum commented Feb 16, 2018 •

edited

Loading

codecov bot commented Feb 17, 2018 •

edited

Loading

mnussbaum Feb 20, 2018 •

edited

Loading

mnussbaum Feb 26, 2018 •

edited

Loading