Split BlockedError to AttachedResourceError and PendingActionError by RobinKaul · Pull Request #1044 · CCI-MOC/hil

RobinKaul · 2018-10-10T15:18:15Z

I'm referring to issue #925 . Since there were two suggestions I am referring to the first one that was to replace BlockedError with a AttachedResourceError and PendingActionError. 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 to OBMError/ 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 PendingActionError or AttachedResourceError so I named them the latter for now.
One little thing I wanted to point out was that in the comments of the node_detach_network function in api.py, it mentioned it raises an error if there is a pending network action but then the error isn't raised.

zenhack

I made a few comments in-line. In addition:

OBMError is inappropriate for this case, as it subclasses ServerError rather than APError, so will be reported to the client as an opaque 5xx status, rather than actually sending the error information. Maybe IllegalStateError instead.
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.

zenhack · 2018-10-10T17:20:27Z


    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.


This seems conceptually wrong to me; this issue isn't that something needs detaching, but that something fundamentally cannot be detached.

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?

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.

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.

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.

Either of those solutions is fine with me.

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?

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.

Oh alright. Then the example needs to be changed. It's quite misleading.

zenhack · 2018-10-10T17:29:47Z

    node = get_or_404(model.Node, node)
    if node.project:
-        raise errors.BlockedError(
+        raise errors.Error(


I assume this was supposed to be AttachedResourceError.

I don't know how I skipped these two this but I've corrected them now. Will push in the future commits.

zenhack · 2018-10-10T17:30:25Z


    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


Again I assume AttachedResourceError.

RobinKaul · 2018-10-10T23:30:23Z

@zenhack quick followup about mentioning the new error types in UPGRADING.rst, has someone already posted a change for 0.5 and I can add to that? Or do I create a new PR for version 0.5?
Side note: the formatting for the steps in version 0.4 is wonky. I will correct that too.

zenhack · 2018-10-11T04:15:30Z

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.

naved001

Like @zenhack said, OBMError is used incorrectly. I think we should keep blocked error for more generic cases, as not everything fits in the category of AttachedResource or PendingAction.

naved001 · 2018-10-11T14:41:39Z

+        raise errors.AttachedResourceError("Project has nodes still")
    if project.networks_created:
-        raise errors.BlockedError("Project still has networks")
+        raise errors.AttachedResourceError("Project still has networks")


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?

This seems like exactly what AttachedResourceError is for, what am I missing?

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).

This seems more analagous to node_delete.

naved001 · 2018-10-11T16:57:05Z


    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.")


This isn't a PendingActionError. I think it should just be a BlockedError for now.

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.

yeah, I like AlreadyInUseError.

Split BlockedError to AttachedResourceError and PendingActionError

60d9afc

zenhack suggested changes Oct 10, 2018

View reviewed changes

naved001 suggested changes Oct 11, 2018

View reviewed changes

Conversation

RobinKaul commented Oct 10, 2018

Uh oh!

zenhack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobinKaul commented Oct 10, 2018

Uh oh!

zenhack commented Oct 11, 2018

Uh oh!

naved001 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants