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

constraintenforcer: Trigger task restarts when appropriate #1958

Merged
merged 1 commit into from Feb 15, 2017

Conversation

Projects
None yet
4 participants
@aaronlehmann
Collaborator

aaronlehmann commented Feb 15, 2017

The constraint enforcer currently sets task desired state to "shutdown"
directly, which means the orchestrator will consider these tasks already
processed, and won't trigger restarts. While this is appropriate for
global services, replicated services should keep the desired number of
replicas running, so each task shut down by the constraint enforcer
should be restarted somewhere else.

This changes the constraint enforcer to trigger a task shutdown by
updating the actual state rather than desired state. This will cause the
orchestrator to restart the task when necessary. It's not a perfect
solution, because it bends rules about field ownership, and may cause
the replacement task to start before the old one stops. However, it's a
good compromise solution for this problem that doesn't require absorbing
the constraint enforcer into each orchestrator (which wouldn't fit the
model well), or adding a third type of state to every task.

Addresses moby/moby#31014

cc @dongluochen @aluzzardi

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Feb 15, 2017

Codecov Report

Merging #1958 into master will increase coverage by 0.08%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #1958      +/-   ##
==========================================
+ Coverage    54.5%   54.58%   +0.08%     
==========================================
  Files         108      108              
  Lines       18577    18586       +9     
==========================================
+ Hits        10125    10146      +21     
+ Misses       7217     7205      -12     
  Partials     1235     1235

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 9ef1e42...8c896af. Read the comment docs.

codecov-io commented Feb 15, 2017

Codecov Report

Merging #1958 into master will increase coverage by 0.08%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #1958      +/-   ##
==========================================
+ Coverage    54.5%   54.58%   +0.08%     
==========================================
  Files         108      108              
  Lines       18577    18586       +9     
==========================================
+ Hits        10125    10146      +21     
+ Misses       7217     7205      -12     
  Partials     1235     1235

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 9ef1e42...8c896af. Read the comment docs.

// will bypass actions such as
// restarting the task on another node
// (if applicable).
t.Status.State = api.TaskStateRejected

This comment has been minimized.

@dongluochen

dongluochen Feb 15, 2017

Contributor

Change function name from shutdownNoncompliantTasks to rejectNoncompliantTasks?

@dongluochen

dongluochen Feb 15, 2017

Contributor

Change function name from shutdownNoncompliantTasks to rejectNoncompliantTasks?

This comment has been minimized.

@aaronlehmann

aaronlehmann Feb 15, 2017

Collaborator

Done.

@aaronlehmann

aaronlehmann Feb 15, 2017

Collaborator

Done.

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Feb 15, 2017

Contributor

LGTM

Contributor

aluzzardi commented Feb 15, 2017

LGTM

constraintenforcer: Trigger task restarts when appropriate
The constraint enforcer currently sets task desired state to "shutdown"
directly, which means the orchestrator will consider these tasks already
processed, and won't trigger restarts. While this is appropriate for
global services, replicated services should keep the desired number of
replicas running, so each task shut down by the constraint enforcer
should be restarted somewhere else.

This changes the constraint enforcer to trigger a task shutdown by
updating the actual state rather than desired state. This will cause the
orchestrator to restart the task when necessary. It's not a perfect
solution, because it bends rules about field ownership, and may cause
the replacement task to start before the old one stops. However, it's a
good compromise solution for this problem that doesn't require absorbing
the constraint enforcer into each orchestrator (which wouldn't fit the
model well), or adding a third type of state to every task.

Also, update the global orchestrator to only restart a task when the
node still meets the constraints.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Feb 15, 2017

Contributor

LGTM

Contributor

dongluochen commented Feb 15, 2017

LGTM

@dongluochen dongluochen merged commit 569defc into docker:master Feb 15, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
dco-signed All commits are signed

@aaronlehmann aaronlehmann deleted the aaronlehmann:constraint-enforcer-task-replacement branch Feb 15, 2017

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