Add /api/tasks/restarttask endpoint

[ashmeet13]

Summary

Currently, the TaskAPI backend does not support restarting a task. To restart as it is now - we have to cancel/clear the task and then initiate it again. This PR adds -

1. The backend logic to support restart for a FAILED or CANCELLED state task
2. Endpoint to restart a task via a POST request containing the task_id to restart.

…

Reviewer guidance

I have added unit tests for the backend logic that is added for queue.py to restart a task.
I was not able to write the unit test for the API endpoint. Could someone guide me here?

…

References

Fixes #7348

…

---

Contributor Checklist

PR process:

• PR has the correct target branch and milestone
• PR has 'needs review' or 'work-in-progress' label
• If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
• If this is an important user-facing change, PR or related issue has a 'changelog' label
• If this includes an internal dependency change, a link to the diff is provided

Testing:

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical user journeys are covered by Gherkin stories
• Critical and brittle code paths are covered by unit tests

Reviewer Checklist

• Automated test coverage is satisfactory
• PR is fully functional
• PR has been tested for accessibility regressions
• External dependency files were updated if necessary (yarn and pip)
• Documentation is updated
• Contributor is in AUTHORS.md

[rtibbles]

A couple of suggestions for improvement, but this is a very solid implementation already, good work!

[ashmeet13]

@rtibbles could you please guide me on how to write the tests for API? I couldn't get Mock to work for me.
There is a test that I have added that is failing as it is now. Could you have a look please? Here is the permalink

[rtibbles]

Pass in id=2 here, and get rid of the line above. That will force your fake job to have an id of 2, so your assertion at the end of the test should now be true.

[ashmeet13]

I tried that. Still have the error.
There are two queue functions that I need to patch here -

1. restart_job
2. fetch_job

Here is how they are used in the implementation restarttask endpoint in the api -

            try:
                task_id = _queue.restart_job(request.data["task_id"])
                resp = _job_to_response(_queue.fetch_job(task_id))
                break

restart_job should return an integer with value 2
fetch_job should return a fake job with State=Queue, id=2

Error is at the stage where restart_job raises JobNotFound. Hence it isn't being patched.

[rtibbles]

Oh, sorry, I understand what you are trying to do now.

In that case, I think the only error here is that the second line should be:

queue_mock.fetch_job.return_value = fake_job(state=State.QUEUED, id=2)

Because at the moment you are only mocking restart_job and not fetch_job too.

[ashmeet13]

Oh my bad for that. But it still fails even after updating it to -

queue_mock.restart_job.return_value = 2
queue_mock.fetch_job.return_value = fake_job(state=State.QUEUED, id=2)

[rtibbles]

Ah yes, you are right - I just checked this out locally. The issue here is that the @patch("kolibri.core.tasks.api.queue") decorator on the TestCase does not seem to be mocking anything - the other tests in this test case all pass, as they all pass in the case that a job is not found.

I think the issue is due to the fact that the TaskViewset queues property is set to a list of [queue, priority_queue] because this is defined at module parse time, the mocking fails to intercept this, because at the time that [queue, priority_queue] is defined the references are to the original, non-mocked objects.

The easiest way to make this work would probably be to turn the queues property of the TaskViewset into:

@property
def queues(self):
    return [queue, priority_queue]

This change along with passing job_id=2 instead of id=2 (my mistake), causes the tests to now pass.

[ashmeet13]

I see. This was something new to me. Thank you for the help @rtibbles!
The test seems to be fixed.

[rtibbles]

Yeah, understanding when and how mocks actually apply can be a little bit difficult to wrap one's head around sometimes!

[rtibbles]

Looking good - left a comment on how to update the API test, once that's fixed, I think this is good to merge!

[jonboiser]

@ashmeet13 This is really great, thanks! Here it is in action when restarting a channel import task. I did find a potential bug, where I could not trigger a restart a second time, so I'll file a bug for that. Let me know if you have any ideas on why this might be happening. You might notice that when I retry the request after it was restarted, I don't get validation errors.

In any case, we will soon try to integrate this into the app and add the ability to restart more types of Tasks!

[jonboiser]

Ah, I think I see what's going on. The new Task actually has a new ID. I wonder if there's a way to let the new task keep the ID of the old one.

[rtibbles]

I suggested implementing it as a task with a new id, as a simpler way to copy over the arguments, but without having to replicate the previous state metadata.

I assume if the frontend just continues to read the current task id, this shouldn't be an issue though right? Just because you're POSTing and not updating the body.