Bug #11682

[API] Deleting container request doesn't cancel containers

Added by Peter Amstutz 2 months ago. Updated 7 days ago.

Status:ResolvedStart date:07/11/2017
Priority:NormalDue date:
Assignee:Radhika Chippada% Done:

100%

Category:-
Target version:2017-07-19 sprint
Story points-Remaining (hours)0.00 hour
Velocity based estimate-

Description

Deleting a container request record should have the same effect as setting priority to 0: it should reevaluate the priority of the container and possible cancel it. Currently that doesn't happen.


Subtasks

Task #11936: Review 11682-delete-container-requestResolvedRadhika Chippada

Associated revisions

Revision 3a5b003b
Added by Radhika Chippada 7 days ago

closes #11682

Merge branch '11682-delete-container-request'

Arvados-DCO-1.1-Signed-off-by: Radhika Chippada <>

Revision 67a56c0f
Added by Radhika Chippada 7 days ago

refs #11682

Merge branch '11682-fix-test'

Arvados-DCO-1.1-Signed-off-by: Radhika Chippada <>

History

#1 Updated by Peter Amstutz 2 months ago

  • Description updated (diff)

#2 Updated by Tom Morris 19 days ago

  • Assignee set to Radhika Chippada
  • Target version set to 2017-07-19 sprint

#3 Updated by Tom Clegg 19 days ago

Deleting a CR should invoke the same hook that runs when priority changes, so (if a container is assigned and the deleted CR had the highest priority of any CR for the assigned container) the container's priority gets updated accordingly.

#4 Updated by Radhika Chippada 18 days ago

  • Status changed from New to In Progress

#5 Updated by Tom Clegg 13 days ago

Would it work just as well to use the existing container and CR called "running" instead of adding new fixtures?

I like the approach of calling update_attributes!(priority: 0) but this seems more like a model concern than a controller concern. Does it work to call self.update_attributes!() in a before_destroy hook on the model instead of the controller? Otherwise, perhaps we just have to call update_priority in an after_destroy (or before_destroy?) hook, and add "...or !persisted?" to the list of conditions in update_priority that trigger an update.

#6 Updated by Radhika Chippada 11 days ago

Branch 11682-delete-container-request @ 27b7b66

I like the approach of calling update_attributes!(priority: 0) but this seems more like a model concern than a controller concern. Does it work to call self.update_attributes!() in a before_destroy hook on the model instead of the controller? ...

Good suggestion. Changed to use a before_destroy filter

Would it work just as well to use the existing container and CR called "running" instead of adding new fixtures?

Test works (worked) with the fixture "running" as well. However, if we were to add any other fixtures with priority>0 using the same container_uuid in future, then the test would fail. We could delete all such container_requests before checking priority in the test. However, I think it would be nice to have the test be independent. I can change the test to use "running" if you prefer to.

#7 Updated by Tom Clegg 11 days ago

Thanks. I think this version looks cleaner.

This test looks like it can/should be done as a unit test -- calling "post :destroy" doesn't add any value compared to calling c.destroy directly, does it?

Can you add a test that destroys a container_request that has priority>0 and state=Final? That should be possible, but I'm suspicious it might fail with "can't update priority when state=Final".

#8 Updated by Radhika Chippada 10 days ago

@ commit:27027c4

Thanks. I think this version looks cleaner.

It sure does. Thanks.

This test looks like it can/should be done as a unit test -- calling "post :destroy" doesn't add any value compared to calling c.destroy directly, does it?

You are right. Replaced the controller test with a unit test.

Can you add a test that destroys a container_request that has priority>0 and state=Final? That should be possible, but I'm suspicious it might fail with "can't update priority when state=Final".

Thanks for noticing the bug I introduced when moving the code around. Corrected the before_destroy implementation and added a unit test to delete a container_request in Final state.

#9 Updated by Tom Clegg 8 days ago

LGTM, thanks!

#10 Updated by Radhika Chippada 7 days ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:3a5b003b5cf784e96799d8ddadc939834620fd34.

Also available in: Atom PDF