Split BlockedError to AttachedResourceError and PendingActionError #1044
Conversation
There was a problem hiding this comment.
I made a few comments in-line. In addition:
OBMErroris inappropriate for this case, as it subclassesServerErrorrather thanAPError, so will be reported to the client as an opaque 5xx status, rather than actually sending the error information. MaybeIllegalStateErrorinstead.- Would you add a section for an upcoming 0.5 release to
docs/UPGRADING.rst, and mention this change there? - You have some travis errors, which need fixing.
I was about to say you need to update rest_api.md, but we don't actually report the error types there (except in the one place you changed). We probably should actually document the format of the responses in the error messages (but that should be a separate pr). //cc @naved001, @ianballou.
| @@ -162,7 +163,7 @@ def network_revoke_project_access(project, network): | |||
| """Remove access to <network> from <project>. | |||
|
|
|||
| If the project or network does not exist, a NotFoundError will be raised. | |||
| If the project is the owner of the network a BlockedError will be raised. | |||
| If the project is the owner of the network a AttachedResourceError will be raised. | |||
There was a problem hiding this comment.
This seems conceptually wrong to me; this issue isn't that something needs detaching, but that something fundamentally cannot be detached.
There was a problem hiding this comment.
Yes exactly that's why I said some of the errors didn't quite fit into it so I named them AttachedResourceError for now. I think I wrote AuthorizationError previously for this but then had to redo all changes for some reason so I might've put the former. What do you think about AuthorizationError though?
There was a problem hiding this comment.
AuthorizationError isn't right either; it isn't a permissions issue. I think BadArgumentError is the closest thing we currently have. I'm not terribly opposed to adding a new type for this sort of thing.
@naved001, @ianballou, interested in your thoughts.
There was a problem hiding this comment.
I agree with @zenhack, BadArgumentError is the closest thing we have. This is the only case we have right now where we have the concept of ACLs. So I think BadArgumentError with a message explaining why the arguments are bad (cannot revoke access from project since project is the owner) is good enough.
If we were to create a new type we could call it IllegalOperationError since it's not legal to for a project to not have access to the network it created. And I think it would be generic enough to be used in similar situations that we may run into.
There was a problem hiding this comment.
Yeah I agree that AttachedResourceError doesn't work here. That error should alert the user that they need to detach something first before continuing with the command that raised the error. Here, there's nothing that needs detaching.
I agree with @naved001 that something like IllegalOperationError would be good. That plus a good error string stating why it's illegal.
There was a problem hiding this comment.
Either of those solutions is fine with me.
There was a problem hiding this comment.
Okay I'm a little confused here. We already have IllegalStateError to indicate the "generic" illegal operation. Is it serving any other purpose too? Also, according to the description, it is essentially doing the same thing as AttachedResourceError unless I'm missing out on something. Should we rename it to IllegalOperationError and give it a proper definition?
There was a problem hiding this comment.
IllegalStateError isn't quite just any generic illegal op. Right now I think it's only used with headnodes, in cases where somebody does something like try to modify the configuration of the VM while it's running -- it is in an "illegal state." The user could get things working by shutting the VM off first. Whereas with this case, removing the owner of a network is simply not allowed; there's nothing the user can do to make the operation legal.
There was a problem hiding this comment.
Oh alright. Then the example needs to be changed. It's quite misleading.
| @@ -319,11 +320,11 @@ def node_delete(node): | |||
| get_auth_backend().require_admin() | |||
| node = get_or_404(model.Node, node) | |||
| if node.project: | |||
| raise errors.BlockedError( | |||
| raise errors.Error( | |||
There was a problem hiding this comment.
I assume this was supposed to be AttachedResourceError.
There was a problem hiding this comment.
I don't know how I skipped these two this but I've corrected them now. Will push in the future commits.
| @@ -355,15 +356,15 @@ def node_delete_nic(node, nic): | |||
| """Delete nic with given name from it's node. | |||
|
|
|||
| If the node or nic does not exist, a NotFoundError will be raised. | |||
| If the nic is on a node that belongs to a project then a BlockedError will | |||
| If the nic is on a node that belongs to a project then a Error will | |||
There was a problem hiding this comment.
Again I assume AttachedResourceError.
|
@zenhack quick followup about mentioning the new error types in |
|
There's no entry for 0.5 yet, because this will be the first user-visible change we've merged so far (@naved001 tagged 0.4 just the other day). So you'll have to add a new section to the document. |
| if project.networks_created: | ||
| raise errors.BlockedError("Project still has networks") | ||
| raise errors.AttachedResourceError("Project still has networks") |
There was a problem hiding this comment.
I think in this instance we can use BlockedError since it's blocked on other thing (the project owning a network) and the user can delete the network to unblock. @zenhack thoughts?
There was a problem hiding this comment.
This seems like exactly what AttachedResourceError is for, what am I missing?
There was a problem hiding this comment.
Per your comment here, isn't this the same scenario, where we can't detach this resource to get past this error? (In this case, we would have to delete the resource).
There was a problem hiding this comment.
This seems more analagous to node_delete.
| "The network is already attached to the nic.") | ||
|
|
||
| if channel is None: | ||
| channel = allocator.get_default_channel() | ||
|
|
||
| if _have_attachment(nic, model.NetworkAttachment.channel == channel): | ||
| raise errors.BlockedError("The channel is already in use on the nic.") | ||
| raise errors.PendingActionError("The channel is already in use on the nic.") |
There was a problem hiding this comment.
This isn't a PendingActionError. I think it should just be a BlockedError for now.
There was a problem hiding this comment.
Maybe it's worth distinguishing a third case, AlreadyInUseError, which could be used here and also in e.g. project_connect_node. AttachedResourceError means "you release that resource because things need to be detached first", whereas AlreadyInUseError means "You can't acquire this resource because it's already in use." I kinda don't like BlockedError because it's rather vague.
There was a problem hiding this comment.
yeah, I like AlreadyInUseError.
I'm referring to issue #925 . Since there were two suggestions I am referring to the first one that was to replace
BlockedErrorwith aAttachedResourceErrorandPendingActionError. Also I noticed that in a lot of places the two new error types wouldn't fit into that particular case so I changed them toOBMError/DuplicateError, etc. Please let me know if they need to be reverted.Also I was kind of swaying between a couple of the error names like when a node/ network is not free. I wasn't sure if they need to be named
PendingActionErrororAttachedResourceErrorso I named them the latter for now.One little thing I wanted to point out was that in the comments of the
node_detach_networkfunction in api.py, it mentioned it raises an error if there is a pending network action but then the error isn't raised.