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

Ensuring endpoint resources are freed even on delete failures #1853

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abhi
Copy link
Contributor

@abhi abhi commented Jul 24, 2017

Came across a code path where we might not be releasing ip
address assigned to an endpoint if we have a failure with
deleteEndpoint. Even if there is a failure it is better to
release the resource rather than holding them. This might
lead to issues where ip never gets released even though
the container has exited and the only way of recovery is a
reload.

Signed-off-by: Abhinandan Prativadi abhi@docker.com

@fcrisciani
Copy link

LGTM

endpoint.go Outdated
@@ -833,6 +833,12 @@ func (ep *endpoint) Delete(force bool) error {
logrus.Warnf("failed to recreate endpoint in store %s : %v", name, e)
}
}

ep.releaseAddress()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code makes sure that the resources are not freed up if deleteEndpoint fails.

I think the better fix is to make sandbox Delete() to call sb.delete(true), which will forcefully remove the endpoint.

@abhi abhi changed the title Ensuring endpoint resources are freed even on delete failures Ensuring endpoint resources are not freed even on delete failures Aug 2, 2017
@fcrisciani
Copy link

@mavenugo @abhinandanpb is this ready to go in?

@abhi abhi changed the title Ensuring endpoint resources are not freed even on delete failures Ensuring endpoint resources are freed even on delete failures Aug 3, 2017
@fcrisciani
Copy link

Considering that now the sb.delete(true) is always called with true, can we just remove the force flag?
The important thing to do while cleaning up is to be sure that all the error cases will leave a warning log and continue with the call processing. WDYT?

@vieux
Copy link
Contributor

vieux commented Aug 11, 2017

@fcrisciani I agree we should still log the error even on force.

@@ -220,13 +220,11 @@ func (sb *sandbox) delete(force bool) error {
continue
}

if !force {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering, this was never done then?

Copy link
Contributor Author

@abhi abhi Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be done during delete sandbox.
On the the contrary this would have been skipped during sandboxCleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually we should be skipping this by default now. Let me check this again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @abhi did you check on this?

Came across a code path where we might not be releasing ip
address assigned to an endpoint if we have a failure with
deleteEndpoint. Even if there is a failure it is better to
release the resource rather than holding them. This might
lead to issues where ip never gets released even though
the container has exited and the only way of recovery is a
reload.

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>
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.

6 participants