Comment 111 for bug 2004555

reviewed cinder patch cc20649efa7383f495

Primary issue is that (at least on my reading) the api-ref and the commit message/release note conflict over whether users are allowed to make the 3 action API calls. The api-ref says that they can, with success dependent on satisfying a safe delete (as for the Attachments API delete call), but the commit message/relnote say the action calls are service-only. The code looks like it's implementing what the api-ref says.

docs: changes are good, read well, render correctly in HTML, links all work (only discuss configuration, so not affected by the above)

api-ref: nit: os-detach, os-force_detach are missing the 409 in the response codes list (only mentioning it because you have it for the os-terminate_connection action)

cinder/volume/api.py
nit: line 2583 (reason=) s/atachment/attachment/

cinder/tests/unit/api/contrib/test_admin_actions.py
nit: line 1040: mock_deletion_allowed returns True, but the real function either raises or returns None; I think it would be better to return None from the mock

cinder/tests/unit/api/v3/test_attachments.py
nit: test_attachment_deletion_allowed_service_call() relies on the default keystone_auth.service_token_roles containing 'service' (which is fine), but since there was talk at some point of hard-coding a 'service' check, it would be good to have a test that uses a non-default value for the config opt

cinder/tests/unit/policies/test_volume_actions.py
in test_owner_can_attach_detach_volume, line 973: I think you deleted 'body = {"os-detach":{}}' by mistake and not the test is checking 2 attach calls

release note: if you revise, the single backticks produce italics; you need double backticks for monospace font

The code in volume/api.py looks fine and the tests are thorough