Skip to content

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 [email protected]

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 <[email protected]>
@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

Signed-off-by: Hamza El-Saawy <[email protected]>
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

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