Skip to content

Fix stopped containers with restart-policy showing as "restarting"#38737

Merged
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:fix_stopped_restart_containers
Feb 28, 2019
Merged

Fix stopped containers with restart-policy showing as "restarting"#38737
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:fix_stopped_restart_containers

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Feb 14, 2019 •
edited
Loading

fixes #38720 Flaky test: DockerDaemonSuite.TestDaemonRestartUnlessStopped

When manually stopping a container with a restart-policy, the container
would show as "restarting" in docker ps whereas its actual state
is "exited".

Stopping a container with a restart policy shows the container as "restarting"

docker run -d --name test --restart unless-stopped busybox false

docker stop test

docker ps
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS                       PORTS               NAMES
7e07409fa1d3        busybox             "false"             5 minutes ago       Restarting (1) 4 minutes ago                     test

However, inspecting the same container shows that it's exited:

docker inspect test --format '{{ json .State }}'
{
  "Status": "exited",
  "Running": false,
  "Paused": false,
  "Restarting": false,
  "OOMKilled": false,
  "Dead": false,
  "Pid": 0,
  "ExitCode": 1,
  "Error": "",
  "StartedAt": "2019-02-14T13:26:27.6091648Z",
  "FinishedAt": "2019-02-14T13:26:27.689427Z"
}

And killing the container confirms this;

docker kill test
Error response from daemon: Cannot kill container: test: Container 7e07409fa1d36dc8d8cb8f25cf12ee1168ad9040183b85fafa73ee2c1fcf9361 is not running

docker run -d --name test --restart unless-stopped busybox false

docker stop test

docker ps
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS                PORTS               NAMES
d0595237054a        busybox             "false"             5 minutes ago       Restarting (1)       4 minutes ago                       exit

Signed-off-by: Sebastiaan van Stijn [email protected]

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member Author

thaJeztah commented Feb 14, 2019

ping @x1022as @cpuguy83 @tonistiigi ptal

@tonistiigi
Copy link
Member

tonistiigi commented Feb 14, 2019

Could also be in

moby/daemon/monitor.go

Line 103 in 818d2dd

c.SetStopped(&exitStatus)

@codecov
Copy link

codecov bot commented Feb 14, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@91d934b). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #38737   +/-   ##
=========================================
  Coverage          ?   36.42%           
=========================================
  Files             ?      613           
  Lines             ?    45855           
  Branches          ?        0           
=========================================
  Hits              ?    16701           
  Misses            ?    26859           
  Partials          ?     2295

@thaJeztah thaJeztah force-pushed the fix_stopped_restart_containers branch from 63cd896 to ac35e50 Compare February 14, 2019 22:13
@thaJeztah
Copy link
Member Author

thaJeztah commented Feb 14, 2019

@tonistiigi thanks; that looks definitely better; updated

@cpuguy83
Copy link
Member

cpuguy83 commented Feb 14, 2019

I'm pretty sure I've seen a PR similar to this.
But this seems like it will race with the call to checkpoint below.

@thaJeztah
Copy link
Member Author

thaJeztah commented Feb 15, 2019

hm, was actually looking at that yes 🤔

@thaJeztah thaJeztah mentioned this pull request Feb 15, 2019
Merged
@kolyshkin
Copy link
Contributor

kolyshkin commented Feb 19, 2019

SGTM

@thaJeztah thaJeztah force-pushed the fix_stopped_restart_containers branch from ac35e50 to 19deacb Compare February 27, 2019 23:14
@thaJeztah
Copy link
Member Author

thaJeztah commented Feb 27, 2019

ping @cpuguy83 updated to move the first checkpoint to above the 'restart' part; PTAL

cpuguy83
cpuguy83 approved these changes Feb 27, 2019
View reviewed changes
Copy link
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 thaJeztah force-pushed the fix_stopped_restart_containers branch from 19deacb to 0c7f69e Compare February 27, 2019 23:17
@thaJeztah
Fix stopped containers with restart-policy showing as "restarting"
8c0ecb6 
When manually stopping a container with a restart-policy, the container
would show as "restarting" in `docker ps` whereas its actual state
is "exited".

Stopping a container with a restart policy shows the container as "restarting"

    docker run -d --name test --restart unless-stopped busybox false

    docker stop test

    docker ps
    CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS                       PORTS               NAMES
    7e07409fa1d3        busybox             "false"             5 minutes ago       Restarting (1) 4 minutes ago                     test

However, inspecting the same container shows that it's exited:

    docker inspect test --format '{{ json .State }}'
    {
      "Status": "exited",
      "Running": false,
      "Paused": false,
      "Restarting": false,
      "OOMKilled": false,
      "Dead": false,
      "Pid": 0,
      "ExitCode": 1,
      "Error": "",
      "StartedAt": "2019-02-14T13:26:27.6091648Z",
      "FinishedAt": "2019-02-14T13:26:27.689427Z"
    }

And killing the container confirms this;

    docker kill test
    Error response from daemon: Cannot kill container: test: Container 7e07409fa1d36dc8d8cb8f25cf12ee1168ad9040183b85fafa73ee2c1fcf9361 is not running

    docker run -d --name test --restart unless-stopped busybox false

    docker stop test

    docker ps
    CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS                PORTS               NAMES
    d0595237054a        busybox             "false"             5 minutes ago       Restarting (1)       4 minutes ago                       exit

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the fix_stopped_restart_containers branch from 0c7f69e to 8c0ecb6 Compare February 27, 2019 23:18
@thaJeztah
Copy link
Member Author

thaJeztah commented Feb 27, 2019

oh! you were fast; Moved the state counter as well

cpuguy83
cpuguy83 approved these changes Feb 27, 2019
View reviewed changes
Copy link
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 thaJeztah merged commit 348d793 into moby:master Feb 28, 2019
@thaJeztah thaJeztah deleted the fix_stopped_restart_containers branch February 28, 2019 01:41
@kolyshkin kolyshkin mentioned this pull request Feb 28, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Mar 2, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Mar 26, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flaky test: DockerDaemonSuite.TestDaemonRestartUnlessStopped

5 participants

@thaJeztah @tonistiigi @cpuguy83 @kolyshkin @GordonTheTurtle