CCI-MOC / hil

Hardware Isolation Layer, formerly Hardware as a Service
Apache License 2.0
24 stars 54 forks source link

Remove the API call for returning active extensions #962

Open naved001 opened 6 years ago

naved001 commented 6 years ago

See the discussion on #939

The consensus is that we don't really need that API. @SahilTikale I am tagging you again, please let us know if you think there's a reason to keep it.

SahilTikale commented 6 years ago

so @naved001 @Izhmash @xuhang57 we did have a discussion about it. Here is what I feel about this.

the heart of the problem is How do I know what arguments to send when I am trying to register a node or a switch currently the options are hard-coded in the cli.py which makes for bad design. we do not want the cli to be burdened with knowing argument requirements for each driver.

@naved001 suggested that once a call for node_register or switch_register fails the corresponding api error message should respond with what arguments are needed for registering a particular type of node or switch. I liked the idea but the problem with this approach is that the client library still needs to built a correct dictionary for which it needs to know the keys apriori. How do we manage that programmatically ?

I suggested the following: When the call to register a switch or node fails, the api will respond with what dictionary of arguments it requires to register the hardware. We send a complete json object (dictionary of arguments) from cli which is treated as payload by client library. That way client side does not have to know about any drivers available on the server side. Thus we can remove api call that queries the active extensions in hil.cfg

Alternatively, lets not have any support for node_register and switch_register in client library. Since their usage requires admin privileges and assumptions that the user knows the infrastructure being registered with HIL, we can also assume that these calls will be done only on the server itself. May be direct rest calls. Just an idea. What do you guys think @zenhack

xuhang57 commented 6 years ago

My thoughts:

We have two issues here.

  1. Do we want to keep the list_active_extensions
  2. How should we handle the hardcoding problem in switch_register (as Sahil talked above)

My humble opinions:

  1. For issue 1, we should determine who should use this function and if no one is going to use it, we should remove it. The idea is not to introduce an unused chunk of code to the code base. My personal opinion on this is to remove the call since we don't have any usage of it for now or in the near future. Also, the purpose of the list_active_extensions is actually trying to solve issue 2. However, it doesn't serve its purpose, at least, for now. That's saying, we can keep it if we figure out how we would want to use it.

  2. For issue 2, I am thinking maybe we should introduce some sort of new models about Switches. The pro is we can specify how many arguments it needs to construct the object so that we can make sure the user pass the proper numbers of arguments. I don't have implementation details in my mind yet. But this should be like:

new brocade = Brocade(arg1, arg2, arg3, arg4)
naved001 commented 6 years ago

I agree with @xuhang57 on 1.

On 2, what I had in mind was. just send a sorted list {"key1": <hostname>, "key2": <other attribute>}. The API server will check for the number of arguments, and if it matches the required number then it will map it to respective fields (in order). The downside is, the order here will matter and I don't know if it's a bad design idea or not. And will also require a change in API.

Will dig more into it on how can this be handled.

zenhack commented 6 years ago

I agree on (1) as well; the complaint was the api call doesn't have a clear use (that actually works anyway; as discussed it doesn't solve the problem it was designed for).

Somewhere (not sure which PR issue it was on, and can't find it now) I outlined a way to write the switch_register calls where we basically hard-code knowledge of certain drivers in the client library, but allow a user to just pass the raw values to be sent to the server, so the library is still usable if you're working with a server that has drivers the library doesn't know about. I still think this is a decent solution.

We could also look into using a more principled way of validating the format of our messages, e.g. json schema. We could build an api call to fetch the schema for the drivers it understands, and the client library could use those to validate the arguments. This solves the problem in a way that its somewhat similar to what the existing API call was trying to do, but it does so without needing to hard-code what each extension name "means," and it avoids exposing the implementation detail. The latter also means it wouldn't be hard to use this approach in OBMd as well, even though it doesn't re-use HIL's extension mechanism at all.

xuhang57 commented 6 years ago

@zenhack https://github.com/CCI-MOC/hil/pull/950 you wrote something here in this PR.

zenhack commented 6 years ago

Yep, that's what I was thinking of, thanks for digging it up.

naved001 commented 6 years ago

the heart of the problem is How do I know what arguments to send when I am trying to register a node or a switch

Node registration will now happen in OBMd; and the cli can now pass either subtype dependent info or raw info the client library. So I guess it's safe to remove the API in question.