Skip to content
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

orchestrator: Avoid restarting a task that has already been restarted #1305

Merged
merged 1 commit into from
Aug 3, 2016

Conversation

aaronlehmann
Copy link
Collaborator

When I was debugging integration test failures a week ago, I dug deep
into the recent restart supervisor changes, and noticed a possible race
which is almost impossible in practice.

This is the sequence of events that has to happen:

  1. Restart task A -> new task B.

  2. B fails before the restart delay has elapsed (pull failure):
    Restart(B) starts waiter goroutine.

  3. B's node fails at exact moment after the restart delay finishes. The
    orchestrator calls Restart(B) again, this time there's no delay in
    progress so Restart proceeds to set B.DesiredState = SHUTDOWN and create
    a new task.

  4. The waiter goroutine from step 2 is scheduled. It calls Restart(B) to
    resume the first restart attempt. This sets B.DesiredState = SHUTDOWN
    (which means no change) and creates a new task.

We'd end up with an extra task.

Again, this would be almost impossible to trigger, but I'm fixing it for
the sake of correctness. The general principle here is that Restart
should never been called on a task that already has DesiredState >
RUNNING, since what Restart does is "shut down this task and replace it
with a new one".

I also added a sanity check to Restart. I'm not sure this is really
helpful because returning an error probably does more harm than good in
this case. But at least it would cause an error message to be logged.

cc @dongluochen

When I was debugging integration test failures a week ago, I dug deep
into the recent restart supervisor changes, and noticed a possible race
which is almost impossible in practice.

This is the sequence of events that has to happen:

1) Restart task A -> new task B.

2) B fails before the restart delay has elapsed (pull failure):
Restart(B) starts waiter goroutine.

3) B's node fails at exact moment after the restart delay finishes. The
orchestrator calls Restart(B) again, this time there's no delay in
progress so Restart proceeds to set B.DesiredState = SHUTDOWN and create
a new task.

4) The waiter goroutine from step 2 is scheduled. It calls Restart(B) to
resume the first restart attempt. This sets B.DesiredState = SHUTDOWN
(which means no change) and creates a new task.

We'd end up with an extra task.

Again, this would be almost impossible to trigger, but I'm fixing it for
the sake of correctness. The general principle here is that Restart
should never been called on a task that already has DesiredState >
RUNNING, since what Restart does is "shut down this task and replace it
with a new one".

I also added a sanity check to Restart. I'm not sure this is really
helpful because returning an error probably does more harm than good in
this case. But at least it would cause an error message to be logged.

Signed-off-by: Aaron Lehmann <[email protected]>
@codecov-io
Copy link

Current coverage is 54.85% (diff: 0.00%)

Merging #1305 into master will decrease coverage by 0.22%

@@             master      #1305   diff @@
==========================================
  Files            78         78          
  Lines         12466      12470     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           6867       6841    -26   
- Misses         4659       4677    +18   
- Partials        940        952    +12   

Sunburst

Powered by Codecov. Last update f808bd4...1f40cec

@dongluochen
Copy link
Contributor

LGTM

@aaronlehmann aaronlehmann merged commit f3bcaff into moby:master Aug 3, 2016
@aaronlehmann aaronlehmann deleted the restart-race branch August 4, 2016 00:21
@aaronlehmann
Copy link
Collaborator Author

There's another way this could be triggered:

  1. Restart task A -> new task B.

  2. B fails before the restart delay has elapsed (task rejected, etc): Restart(B) starts waiter goroutine.

  3. The orchestrator calls Restart(B) multiple times because B's task object gets updated multiple times. Normally Restart(B) would only be called once, but the first Restart(B) didn't set the desired state to SHUTDOWN, because it only started the waiter goroutine. So multiple waiter goroutines are started.

  4. When the restart delay elapses, all the waiter goroutines wake up, and they will each call Restart, which results in more than one new task.

I think this explains moby/moby#27382

We should probably backport this to 1.12.3.

cc @aluzzardi @thaJeztah

@thaJeztah
Copy link
Member

@aaronlehmann thanks for looking into this; can you open a tracking issue? I think it'll depend on the amount of risk / code change to backport it

@aaronlehmann
Copy link
Collaborator Author

Very low risk - take a look at the code changes. It adds some sanity checks that were missing before.

Can we use moby/moby#27382 as the tracking issue?

@thaJeztah
Copy link
Member

@aaronlehmann 😊 didn't look at the code, yes, that looks fairly low risk

I added moby/moby#27382 to the 1.12.3 milestone for docker

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.

5 participants