whole-tale / girder_wholetale

Girder plugin providing basic Whole Tale functionality
BSD 3-Clause "New" or "Revised" License
3 stars 5 forks source link

Deal with Instances upon Tale delete/unshare. Fixes #505 #506

Closed Xarthisius closed 2 years ago

Xarthisius commented 2 years ago

This PR handles cleaning Instances upon changes to the Tale object:

  1. DELETE /tale/:id?force=False, PUT /tale/:id/access?force=False and PUT /tale/:id/relinquish?force=False now returns 409 if there are running Instances for that Tale and user access would fall beneath WRITE or Tale would be deleted.
  2. DELETE /tale/:id?force=True, PUT /tale/:id/access?force=True and PUT /tale/:id/relinquish?force=True removes the Tale or change ACLs respectively and shutdowns all related Instances if necessary.

TODO:

codecov[bot] commented 2 years ago

Codecov Report

Merging #506 (fde88d2) into master (2275f71) will increase coverage by 0.04%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
+ Coverage   93.33%   93.38%   +0.04%     
==========================================
  Files          54       54              
  Lines        4125     4156      +31     
==========================================
+ Hits         3850     3881      +31     
  Misses        275      275              
Impacted Files Coverage Δ
server/models/tale.py 98.09% <100.00%> (+0.01%) :arrow_up:
server/rest/tale.py 96.49% <100.00%> (+0.35%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2275f71...fde88d2. Read the comment docs.

craig-willis commented 2 years ago

I'm probably missing something, but why we can't we do the same thing on both deleteTale and updateTaleAccess? Check for instances where users would lose access and return and error if present, otherwise have a force option?

Xarthisius commented 2 years ago

I'm probably missing something, but why we can't we do the same thing on both deleteTale and updateTaleAccess? Check for instances where users would lose access and return and error if present, otherwise have a force option?

Mostly because that would require figuring out what changed in access without applying it, which is tedious and I'm lazy. In case of DELETE we know that everyone "loses" access, so it's easy.

Do we really want that feature?

craig-willis commented 2 years ago

As discussed, we can defer implementing on unshare until someone asks for it. It just means that reducing permissions or unsharing will result in terminating other users' instances without warning.

Xarthisius commented 2 years ago

As discussed, we can defer implementing on unshare until someone asks for it. It just means that reducing permissions or unsharing will result in terminating other users' instances without warning.

We also discussed that it may actually get implemented by 3pm today and it did!

Xarthisius commented 2 years ago

Even better. Since some of the instances_to_kill logic is duplicated between relinquishTaleAccess and updateTaleAccess, I wonder if it could be moved into a utility method -- in case we forget to update one in the future? But also not much code.

I tried to refactor it, but it yielded more code in the end...