Remove rkt pods when exiting #3562
Conversation
schmichael
force-pushed
the
b-3561-rkt-rm
branch
from
e03170e to
5f424de
Compare
schmichael
force-pushed
the
b-3561-rkt-rm
branch
from
5f424de to
ce3fbb6
Compare
| if err := cmd.Run(); err == nil { | ||
| t.Fatalf("expected error running 'rkt status %s' on removed container", handle.uuid) | ||
| } | ||
| if out := stderr.String(); !strings.Contains(out, "no matches found") { |
There was a problem hiding this comment.
This seems brittle if the returned error message ever changes, but other than checking for a non empty error message I don't have a better idea.
There was a problem hiding this comment.
Yeah, I hate resorting to strings.Contains, but if it ever fails due to a string change it should be a quick and easy fix (and caught by Travis, not a user at runtime).
client/driver/rkt.go
Outdated
| @@ -781,6 +789,13 @@ func (h *rktHandle) run() { | |||
| } | |||
| h.pluginClient.Kill() | |||
|
|
|||
| // Remove the pod | |||
| if err := rktRemove(h.uuid); err != nil { | |||
| h.logger.Printf("[ERR] driver.rkt: error removing pod %q - must gc manually", h.uuid) | |||
| @@ -781,6 +789,13 @@ func (h *rktHandle) run() { | |||
| } | |||
| h.pluginClient.Kill() | |||
|
|
|||
| // Remove the pod | |||
| if err := rktRemove(h.uuid); err != nil { | |||
There was a problem hiding this comment.
Thinking: Should it use the created resources feature so that cleanup gets retried?
There was a problem hiding this comment.
I just tried to match Docker's behavior and this is how we do it there. I'll update rkt to use created resources.
There was a problem hiding this comment.
Investigated this route and it's a bit more work than I'd like to put in this PR. It involves returning CreatedResources from Driver.Start which isn't already happening and ensuring TaskDriver cleans up containers exactly when desired. Following Dockers lead for now seems safest.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #3561