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

Fix some issues with locking on the container #35501

Merged
merged 1 commit into from Nov 30, 2017

Conversation

Projects
None yet
7 participants
@cpuguy83
Contributor

cpuguy83 commented Nov 15, 2017

  • Fix OOM event updating healthchecks and persisting container state
    without locks
  • Fix healthchecks being updated without locks on container stop

@thaJeztah thaJeztah added this to backlog in maintainers-session Nov 16, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 16, 2017

Member

@stevvooe could you and @cpuguy83 look at this PR, compared to #35517 - both address the issue but in a different way, so perhaps you can discuss which direction we should take?

Member

thaJeztah commented Nov 16, 2017

@stevvooe could you and @cpuguy83 look at this PR, compared to #35517 - both address the issue but in a different way, so perhaps you can discuss which direction we should take?

Show outdated Hide outdated daemon/monitor.go Outdated
@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Nov 16, 2017

Contributor

@thaJeztah Either one or both are fine. #35517 is feeding the race detector that gets tripped during the unit test that was added, so maybe its not ideal. If we can avoid having CloseMonitorChannel be called in parallel, that might preserve the status quo.

Contributor

stevvooe commented Nov 16, 2017

@thaJeztah Either one or both are fine. #35517 is feeding the race detector that gets tripped during the unit test that was added, so maybe its not ideal. If we can avoid having CloseMonitorChannel be called in parallel, that might preserve the status quo.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 16, 2017

Contributor

I think both are fine.

Contributor

cpuguy83 commented Nov 16, 2017

I think both are fine.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 17, 2017

Contributor

And by fine I mean needed.

Contributor

cpuguy83 commented Nov 17, 2017

And by fine I mean needed.

Fix some issues with locking on the container
- Fix OOM event updating healthchecks and persisting container state
without locks
- Fix healthchecks being updated without locks on container stop

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Nov 21, 2017

Collaborator

LGTM

Collaborator

tiborvass commented Nov 21, 2017

LGTM

@AkihiroSuda AkihiroSuda merged commit 3d80a69 into moby:master Nov 30, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37950 has succeeded
Details
janky Jenkins build Docker-PRs 46664 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7075 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18221 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6894 has succeeded
Details

@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