Client API for switch_register

[xuhang57]

This patch adds the client API function for switch_register. The cli code change accordingly.

Trying to find a call to return all existing switch types.

Any idea/suggestion/opinion/ is more than welcome.

This relates to the issue: #924

Joint effort with @naved001

[zenhack]

There's no programmatic way to enumerate all possible drivers. Furthermore the drivers are plugabble; you could concievably install a driver for a new switch in HIL without it being part of the main HIL source tree (this is why extensions are listed by full module path in the config file).

It would be nice for the call to be more generic. One thing we could do is to have the call just have a subtype argument and then some kwargs, which would map directly to the body of the request. We could validate the types we recognize and pass others through, maybe using the schema library. So something like:

# constants for known_switch_types. The values correspond to the `api_name` field on the server's
# switch classes.
POWERCONNECT_55XX = 'http://.../powerconnect55xx'
NEXUS = 'http://.../nexus'
...

# dictionary holding known switch types, and schema to validate them
_known_types = {
    POWERCONNECT_55XX: Schema({
        'hostname': basestring,
        'username': basestring,
        'password': basestring,
    }),
    ...
}

def switch_register(type, *kwargs):
    if type in known_types:
        try:
            known_types[type].validate(kwargs)
        except SchemaError:
            ...
    # make call to server

This allows the clients to use switches the library doesn't know about yet, and providing constants for the api_names of known switches means if a user mis-types the switch name, they'll just get a NameError, instead of the library just assuming they know what they're doing.

[naved001]

so this goes in the api or the client lib?

_known_types = {
    POWERCONNECT_55XX: Schema({
        'hostname': basestring,
        'username': basestring,
        'password': basestring,
    }),

[xuhang57]

IIRC, this goes in the client lib as a semiprivate dictionary.

[naved001]

so in essence we are hardcoding it in the client lib?

[zenhack]

Right, the dictionary & constants go in the clientlib, hard-coded. For any switch it doesn't know about, you can still make the call, it just can't validate the extra arguments for you.

[xuhang57]

@zenhack hmm, given the sample code you are writing, you put it in the api.py rather than client/switch.py. Should I put this in the API code instead? I don't know whether it makes sense to put Schema related code in the client lib. or not.

[xuhang57]

And having the key in the dictionary like that causing a problem, since

if subtype in self._known_types: is looking for the mock rather than http://schema.massopencloud.org/haas/v0/switches/mock.

I am a bit worried that this is a bit over engineered?

[naved001]

Should I put this in the API code instead

The API server already rejects bad arguments or invalid subtypes*. See this line

[zenhack]

The intention was that this would go in the client lib, not the api server (as @naved001 points out, the validation is already there on the server).

Also, the idea is that the user of the client lbrary would typically pass in the constants e.g. NEXUS, rather than a literal string. The constant is defined to be equal to the URL, so I don't see the problem there.

One way or another the url has to get into the request, so we either need some kind of translation mechanism (like I describe above), or the user will have to provide the literal url; there's no getting around that.

[xuhang57]

Hmm, I was worried about the kind of translation mechanism (above), is not efficient because the arguments we have in the def register() is subtype, which is merely a string such as mock rather than the entire URL.

I do see the advantage of that mechanism. I will write something and I am sure you guys would have a better idea how to improve it.

[coveralls]

Pull Request Test Coverage Report for Build 1768

• 29 of 44 (65.91%) changed or added relevant lines in 6 files are covered.
• 3 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.5%) to 59.787%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
hil/cli.py	1	16	6.25%
<!--	Total:	29	44	65.91%	-->

Files with Coverage Reduction	New Missed Lines	%
hil/ext/obm/mock.py	1	97.62%
hil/ext/obm/ipmi.py	2	53.85%
<!--	Total:	3		-->

Totals
Change from base Build 1742:	0.5%
Covered Lines:	2132
Relevant Lines:	3566

---

💛 - Coveralls

[naved001]

Another idea that I have is, ask the CLI users to provide the key and value when entering args for switch regsiter. So, for instance the call to register a brocade switch will look like hil switch register my-brocade-switch brocade username hostname password interface_type:TenGigabitEthernet for nexus hil switch register my-nexus-switch nexus username hostname password dummy_vlan:2222 then the client library, can just shove those colon delimited key value pairs into switchinfo and make the API call.

So the generic way to register a switch will be: hil switch register my-switch-name switch-subtype username hostname password param1:value1 param2:value2 and so on.

What do you think @zenhack ?

[xuhang57]

known_types[type].validate(kwargs) this won't work since it is expecting of getting a dictionary of keyword arguments. However, we are getting args which is a tuple:

('ip', 'user', pass')

Maybe there is a better way of validating the inputs? still thinking

[zenhack]

We could do mirror the way the client library does things; in the generic case, we'd have something like this:

hil switch register --type 'http://example.com/my/switch/schema/url' --info '{"foo": "bar", "baz": "quux"}'

and maybe do custom sub-command parsers for switches we know about:

hil switch register --type nexus --user alice --password secret ...

Obviously this will be easier to do once the CLI refactoring is in.

[naved001]

@zenhack I am fine with the way you are suggesting it too. Just one question. --type 'http://example.com/my/switch/schema/url why make the user type the whole URL? why can't we only get the subtype and then in the client library we add the rest of the uri to it?

So I think we can skip the checks in the client library, since we already have server side checks. @xuhang57 you on board with this idea? Let's just finish this PR.

[xuhang57]

I agree that we shouldn't let the user type the entire URL.

And, I don't know, I think what Ian has suggested here is more on the CLI end. For the first approach that Ian suggests, rather than an args, we will have a kwargs passed to the client library switch_register function. And we can validate it.

And the reason we are doing the validation is that we would like to throw an error before we send the request and getting an error back from the API server.

so @naved001 , deciding whether we want to do the validation is something we need to discuss I think.

[zenhack]

The full url is there so the user can deal with switch drivers that the cli doesn't know about; per above we can have shorthands for known switches.

[xuhang57]

And should we do the checks/validations in the client library? I think we need to solve that argument.

[naved001]

I think we can skip the additional checks in the client library since we already have it sever side, but I am not against it either.

[zenhack]

Right now the client lib does validation, so it's just a question of how the CLI should report errors from that to the user.

[naved001]

CLI should report errors from that to the user.

It will catch a FailedAPICallException which will probably have some schema error because the validation in the driver failed. I mean, it won't be super helpful for the end user; they would need to read the docs.

[naved001]

@zenhack

per above we can have shorthands for known switches.

Cool, I did that in the latest commit. For known switches, the CLI does the helpful validation and forms the switchinfo. For unknown switch types, it still passes the arguments to the client library. Any error raised by the API server is passed to the user. For example:

(.venv) naved:~/switch-register/hil$ hil switch_register switchnname http://schema.massopencloud.org/haas/v0/switches/dellnos9 '{"adasd": "asdasd"}'
Error: Request arguments are not valid for this request

I don't know if it's worth validating the request body in the client library (it's pretty much the same checks what we have in the driver code).

[xuhang57]

Not sure whether I understand @zenhack correctly, I will try: In practice, we don't want the users to query the API server with some simple mistakes such as typo since it is expensive and not effective. This is very practical if we have a huge traffic and this will make us less worried about increasing the capacity of the server.

However, we don't have a use case for handling huge traffic and I believe that's not on our roadmap either so I don't know whether we want to implement the checks. For sure there might be other reasons and we can discuss. But for now, I will agree with @naved001 and we should keep the client library as simple as possible.

[zenhack]

If we're going to provide any kind of short-hands that knowledge needs to be in the client lib regardless, so doing validation doesn't seem like much of an extra step. It may also help use generate better error messages.

We could split the validation code off into a common module, so we at least don't have to write it twice (and keep things in sync).

My inclination is to just keep things simple for now; I'm fine leaving the client-side validation out if you guys think that's the right approach.

[naved001]

@zenhack sorry to keep ping ponging you on this issue.

If we're going to provide any kind of short-hands that knowledge needs to be in the client lib regardless,

Right now the cli does some useful checks for the known types and prints useful errors, why would it be needed in the client library? Do you mean to get rid of those checks from the cli and move it into the client library?

We could split the validation code off into a common module, so we at least don't have to write it twice

The validation code is in the switch drivers, how would we put it in a common module? Wouldn't that require changes in every driver to import the validation code from that common module?

My inclination is to just keep things simple for now; I'm fine leaving the client-side validation out if you guys think that's the right approach.

yeah, I would like to keep it simple too. In my most recent commit that's what I am trying to do. When this gets merged, I'll update #969 to handle switch registration for known types.

Is there anything else that need to be fixed, besides the failing tests? Thanks!

[xuhang57]

@naved001 thanks for figuring out what's missing in the api.py @zenhack @Izhmash Please take a look 👍 So we can get the new CLI ready for review