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

controlapi: Allow removal of network when only dead tasks reference it #2018

Merged
merged 1 commit into from Mar 9, 2017

Conversation

Projects
None yet
4 participants
@aaronlehmann
Collaborator

aaronlehmann commented Mar 8, 2017

RemoveNetwork currently won't allow a network to be removed if any task in the store references it. This is a change relative to Docker 1.12, where only services were checked.

This might be a little heavy handed. Some tasks in the store may be long dead. Resource attachment tasks, in particular, must be detached by the API user, and this might be neglected after a failure.

Change RemoveNetwork to only fail the network deletion if a task has actual and desired state of "running" or below. This means that neither a failed resource attachment task (whose desired state won't be adjusted) or a down node (where actual state won't reflect desired state) can block the network deletion.

There may be some corner cases where deleting the network while a task is in the process of shutting down prevents the allocator from deallocating that task, but on balance this seems better than allowing many situations to block network deletion indefinitely.

cc @alexmavr @dongluochen @aboch

controlapi: Allow removal of network when only dead tasks reference it
RemoveNetwork currently won't allow a network to be removed if any
task in the store references it. This is a change relative to Docker
1.12, where only services were checked.

This might be a little heavy handed. Some tasks in the store may be long
dead. Resource attachment tasks, in particular, must be detached by the
API user, and this might be neglected after a failure.

Change RemoveNetwork to only fail the network deletion if a task has
actual and desired state of "running" or below. This means that neither
a failed resource attachment task (whose desired state won't be
adjusted) or a down node (where actual state won't reflect desired
state) can block the network deletion.

There may be some corner cases where deleting the network while a task
is in the process of shutting down prevents the allocator from
deallocating that task, but on balance this seems better than allowing
many situations to block network deletion indefinitely.

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

@aaronlehmann aaronlehmann added this to the 17.03.1 milestone Mar 8, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Mar 8, 2017

Codecov Report

Merging #2018 into master will increase coverage by 0.07%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2018      +/-   ##
==========================================
+ Coverage   53.66%   53.73%   +0.07%     
==========================================
  Files         109      109              
  Lines       18991    18978      -13     
==========================================
+ Hits        10191    10198       +7     
+ Misses       7578     7549      -29     
- Partials     1222     1231       +9

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 e4762bc...b00598c. Read the comment docs.

codecov bot commented Mar 8, 2017

Codecov Report

Merging #2018 into master will increase coverage by 0.07%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2018      +/-   ##
==========================================
+ Coverage   53.66%   53.73%   +0.07%     
==========================================
  Files         109      109              
  Lines       18991    18978      -13     
==========================================
+ Hits        10191    10198       +7     
+ Misses       7578     7549      -29     
- Partials     1222     1231       +9

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 e4762bc...b00598c. Read the comment docs.

@aboch

This comment has been minimized.

Show comment
Hide comment
@aboch

aboch Mar 8, 2017

Looks good to me

aboch commented Mar 8, 2017

Looks good to me

@aboch

This comment has been minimized.

Show comment
Hide comment
@aboch

aboch Mar 8, 2017

Just to make sure,
if t.DesiredState <= api.TaskStateRunning && t.Status.State <= api.TaskStateRunning
the agent has reported the state therefore there is no way the corresponding container is still running. Is that correct ?
Just want to make sure we do not allow a swarm level network remove while the effective network remove in libnetwork would fail.

aboch commented Mar 8, 2017

Just to make sure,
if t.DesiredState <= api.TaskStateRunning && t.Status.State <= api.TaskStateRunning
the agent has reported the state therefore there is no way the corresponding container is still running. Is that correct ?
Just want to make sure we do not allow a swarm level network remove while the effective network remove in libnetwork would fail.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Mar 8, 2017

Collaborator

Just to make sure,
if t.DesiredState <= api.TaskStateRunning && t.Status.State <= api.TaskStateRunning
the agent has reported the state therefore there is no way the corresponding container is still running. Is that correct ?

This logic allows the network removal if either:

  • The container has stopped running (t.Status.State), OR
  • The manager wants the container to stop running (t.DesiredState)

I did it this way to lean towards being permissive about deletions. If I only relied on t.Status.State, a disconnected or unresponsive node might block the network from being removed, since we would not have received confirmation that the container has actually shut down. If we only relied on t.DesiredState, network attachment tasks that have stopped running would block networks from being removed, since their desired state is never updated (there is no orchestrator for them).

If the possibility of removing a network that's being used by a container which is about to shut down is a problem, we can revisit this. My thinking was that allowing some corner cases like this would be better than the alternative of cases where it's impossible to remove the network, say because it's blocked by a task on an unresponsive node.

Collaborator

aaronlehmann commented Mar 8, 2017

Just to make sure,
if t.DesiredState <= api.TaskStateRunning && t.Status.State <= api.TaskStateRunning
the agent has reported the state therefore there is no way the corresponding container is still running. Is that correct ?

This logic allows the network removal if either:

  • The container has stopped running (t.Status.State), OR
  • The manager wants the container to stop running (t.DesiredState)

I did it this way to lean towards being permissive about deletions. If I only relied on t.Status.State, a disconnected or unresponsive node might block the network from being removed, since we would not have received confirmation that the container has actually shut down. If we only relied on t.DesiredState, network attachment tasks that have stopped running would block networks from being removed, since their desired state is never updated (there is no orchestrator for them).

If the possibility of removing a network that's being used by a container which is about to shut down is a problem, we can revisit this. My thinking was that allowing some corner cases like this would be better than the alternative of cases where it's impossible to remove the network, say because it's blocked by a task on an unresponsive node.

@aboch

This comment has been minimized.

Show comment
Hide comment
@aboch

aboch Mar 8, 2017

My thinking was that allowing some corner cases like this would be better than the alternative of cases where it's impossible to remove the network

Agree, seems reasonable. Also, considering that proving the above theory about a corner case may be harder than actually work to a fix on a different part of the stack, should the issue presents.

aboch commented Mar 8, 2017

My thinking was that allowing some corner cases like this would be better than the alternative of cases where it's impossible to remove the network

Agree, seems reasonable. Also, considering that proving the above theory about a corner case may be harder than actually work to a fix on a different part of the stack, should the issue presents.

@aboch

This comment has been minimized.

Show comment
Hide comment
@aboch

aboch Mar 8, 2017

Still looks good to me

aboch commented Mar 8, 2017

Still looks good to me

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Mar 9, 2017

Contributor

And we assume a race where the a task is being rescheduled is not possible since first we're checking for services, correct?

Contributor

aluzzardi commented Mar 9, 2017

And we assume a race where the a task is being rescheduled is not possible since first we're checking for services, correct?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Mar 9, 2017

Collaborator

Yeah.

Collaborator

aaronlehmann commented Mar 9, 2017

Yeah.

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Mar 9, 2017

Contributor

LGTM

Contributor

aluzzardi commented Mar 9, 2017

LGTM

@aluzzardi aluzzardi merged commit c51357d into docker:master Mar 9, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 53.73% (target 0%)
Details
dco-signed All commits are signed
@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Mar 10, 2017

Contributor

I agree with this change. However I think we need to treat "node down" as a common case, not a corner case like this. We should have a way to cleanly evict tasks from a node and handle state properly in manager. Other components don't need to make assumption on it.

Contributor

dongluochen commented Mar 10, 2017

I agree with this change. However I think we need to treat "node down" as a common case, not a corner case like this. We should have a way to cleanly evict tasks from a node and handle state properly in manager. Other components don't need to make assumption on it.

@aaronlehmann aaronlehmann deleted the aaronlehmann:remove-network-failed-task branch Mar 10, 2017

@aaronlehmann aaronlehmann referenced this pull request Mar 11, 2017

Merged

bump to 17.03.1-rc1 #31754

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