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.

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

container: protect the health status with mutex #35517

Merged
merged 1 commit into from Nov 20, 2017

Conversation

Projects
None yet
5 participants
@stevvooe
Contributor

stevvooe commented Nov 16, 2017

Adds a mutex to protect the status, as well. When running the race
detector with the unit test, we can see that the Status field is written
without holding this lock. Adding a mutex to read and set status
addresses the issue.

Signed-off-by: Stephen J Day stephen.day@docker.com

Giant Bison

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 16, 2017

Member

There's a golint error;

04:01:26 container/health.go:21:1:warning: comment on exported method Health.Status should be of the form "Status ..." (golint)
04:01:26 container/health.go:39:1:warning: exported method Health.SetStatus should have comment or be unexported (golint)
Member

thaJeztah commented Nov 16, 2017

There's a golint error;

04:01:26 container/health.go:21:1:warning: comment on exported method Health.Status should be of the form "Status ..." (golint)
04:01:26 container/health.go:39:1:warning: exported method Health.SetStatus should have comment or be unexported (golint)
container: protect the health status with mutex
Adds a mutex to protect the status, as well. When running the race
detector with the unit test, we can see that the Status field is written
without holding this lock. Adding a mutex to read and set status
addresses the issue.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 17, 2017

Member

DockerSuite.TestLogsAPIUntilDefaultValue
Looks like a flaky test on Windows: https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/18169/console

00:34:16 ----------------------------------------------------------------------
00:34:16 FAIL: docker_api_logs_test.go:187: DockerSuite.TestLogsAPIUntilDefaultValue
00:34:16 
00:34:16 docker_api_logs_test.go:213:
00:34:16     c.Assert(defaultLogs, checker.DeepEquals, allLogs)
00:34:16 ... obtained []string = []string{""}
00:34:16 ... expected []string = []string{"2017-11-17T00:34:14.050775800Z log1", "2017-11-17T00:34:14.051776600Z log2", "2017-11-17T00:34:14.051776600Z log3", ""}
00:34:16 

May be similar to #33246

Member

thaJeztah commented Nov 17, 2017

DockerSuite.TestLogsAPIUntilDefaultValue
Looks like a flaky test on Windows: https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/18169/console

00:34:16 ----------------------------------------------------------------------
00:34:16 FAIL: docker_api_logs_test.go:187: DockerSuite.TestLogsAPIUntilDefaultValue
00:34:16 
00:34:16 docker_api_logs_test.go:213:
00:34:16     c.Assert(defaultLogs, checker.DeepEquals, allLogs)
00:34:16 ... obtained []string = []string{""}
00:34:16 ... expected []string = []string{"2017-11-17T00:34:14.050775800Z log1", "2017-11-17T00:34:14.051776600Z log2", "2017-11-17T00:34:14.051776600Z log3", ""}
00:34:16 

May be similar to #33246

@cpuguy83

LGTM

@yongtang

LGTM

@thaJeztah

all green

LGTM

@thaJeztah thaJeztah merged commit 9de84a7 into moby:master Nov 20, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37901 has succeeded
Details
janky Jenkins build Docker-PRs 46615 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7026 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18189 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6830 has succeeded
Details

@stevvooe stevvooe deleted the stevvooe:protect-the-health-status branch Nov 21, 2017

@thaJeztah thaJeztah removed this from backlog in maintainers-session Nov 30, 2017

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