Add proper deletion of workloads tasks in shim #1234
Conversation
helsaawy
changed the title
helsaawy
force-pushed
the
he/restart
branch
2 times, most recently
from
3a6b782 to
cb125ea
Compare
This commit supports restarting containers and pods using CRI. Delete task request now removes pods from `pod`'s `workloadTasks` map, and added `DeleteTask` function to `shimPod` so new containers can use the same ID (ie, so pods can be restarted). Updated runc interface with errors, and tied to HResult errors. Fixed bug where `runc delete` command was preemptively issued by runc run time, which prevented proper clean up of container resources in the shim Added tests for stopping and starting (or resetting) containers and pods. There are issues with stopping immediately after restarting, so waits/retries are necessary to make tests pass. Signed-off-by: Hamza El-Saawy <[email protected]>
Need to use changes to CRI (plugin) API for testing, so vendor that in preemptively until the CRI PR is merged. Signed-off-by: Hamza El-Saawy <[email protected]>
| runcErr := getRuncLogError(logPath) | ||
| c.r.cleanupContainer(c.id) | ||
| return errors.Wrapf(err, "runc start failed with %v: %s", runcErr, string(out)) | ||
| return errors.Wrapf(runcErr, "runc start failed with %v: %s", err, string(out)) |
There was a problem hiding this comment.
Wait I think it was right the original way, as the format string is stating that runc failed with %v and is supposed to get replaced with whatever was found in the Runc log file which runcErr has the contents of
There was a problem hiding this comment.
Although maybe I'm missing something
There was a problem hiding this comment.
because of Wrapf, both will get formatted into the string when printed, but err is usually a generic "runc command failed". Wrapping the runc log file error allows the use of errors.Is(, vs having to search through the error string for whatever the log file message was.
|
I feel as if this PR may be doing too much. Can we split this up into separate PRs that all make sense on their own (if possible). The tests won't make sense to add yet, but the deletion of the tasks from being tracked seems like something we were meaning to do all along based on Justin's comment. |
I should be able to split this into three PRs, so sounds good |
3 participants
Delete task request now removes pods from
pod'sworkloadTasksMap, and addedDeleteTaskfunction toshimPodso new containers can use the same ID (ie, so pods can be restarted).Updated runc interface with errors, and tied to HResult errors.
Also updated errors returned by runc interface/wrapper to be wrap new errors, so
errors.Isworks.Fixed bug where
runc deletecommand was preemptively issued by runc run time, which prevented proper clean up of container resources in the shim.PR relies on accompanying CRI PR (kevpar/cri#13) being merged.
Signed-off-by: Hamza El-Saawy [email protected]