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

Delete shim workloads tasks in pod. #1271

Merged
merged 3 commits into from
Feb 8, 2022

Conversation

helsaawy
Copy link
Contributor

@helsaawy helsaawy commented Jan 10, 2022

This commit supports restarting containers and pods using CRI:
kevpar/cri#13

This PR allows the service to remove tasks from a pods workloadTasks map after the task and associated execs have been shut down during in a delete task request, allowing for proper deletion of the task and freeing up associated resources when
received by the service. Namely, this frees up the deleted task's ID, so that new tasks can be created with that same ID after the original task has been deleted (ie, so a task can be restarted within a running pod).

A DeleteTask function was added to the shimPod interface to implement most of this functionality.

Additionally, the service, in deleteInternal, resets its internal reference to the init task (shimPod or shimTask) reference, taskOrPod, if the delete is issued for the init task, as a marker that the service is no longer operational and to prevent future operations from occurring.

Signed-off-by: Hamza El-Saawy hamzaelsaawy@microsoft.com

This commit supports restarting containers and pods using CRI.

Delete task request now removes tasks from `pod`'s `workloadTasks` map,
and added `DeleteTask` function to `shimPod` interface so new tasks can
use the same ID (ie, so a task can be restarted in a running pod).

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@dcantah
Copy link
Contributor

dcantah commented Jan 10, 2022

Thanks for splitting these up!

Copy link
Contributor

@anmaxvl anmaxvl left a comment

Choose a reason for hiding this comment

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

lgtm. small nit

cmd/containerd-shim-runhcs-v1/pod.go Outdated Show resolved Hide resolved
Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
if err != nil {
return nil, errors.Wrapf(err, "could not get pod %q to delete task %q", s.tid, req.ID)
if req.ExecID == "" {
// delete is for a task, and not an exec
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be above req.ExecID

err = p.DeleteTask(ctx, req.ID)
if err != nil {
return nil, fmt.Errorf("could not delete task %q in pod %q: %w", req.ID, s.tid, err)

Copy link
Contributor

Choose a reason for hiding this comment

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

empty new line

Copy link
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

LGTM, small comments

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants