search

CCI-MOC / hil

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

Spec for expressing switch driver capabilities #876

Closed naved001 closed 7 years ago

naved001 commented 7 years ago

Making the first spec here for Issue #410

naved001 commented 7 years ago

@shwsun @knikolla thoughts?

henn commented 7 years ago

Reiterating my above question (since I made it in an edit): are there capabilities that we might want to hide from end-users? For example, the user might not need to know that a port gets shut off on a particular switch.

naved001 commented 7 years ago

are there capabilities that we might want to hide from end-users?

I think, to begin with, we should only expose limited capabilities to end users.

And things like DHCP/ARP snooping should only be visible to admins. Actually, I don't know if they should be exposed at all since they seem like one time configuration changes. We already recommend a bunch of config settings to make when setting up a switch for HIL anyway.

the user might not need to know that a port gets shut off on a particular switch.

This is not a switch feature, right? So I don't understand how is this related to switch capabilities. But yeah, the end users don't need to be aware about switchport status.

henn commented 7 years ago

And things like DHCP/ARP snooping should only be visible to admins

I envisioned DHCP/ARP snooping being exposed to the HIL tenant, since tenants could select which ports they want to be enabled for DHCP serving. I expect there will be an associates API calls for enable_arp_snooping() and enable/disable_dhcp_server() that takes a host, NIC and network argument.

This actually brings up an interesting point -- DHCP/ARP snooping support can differ based on the switch, but it's something that would only make sense to have globally enabled on all the switches or none of them.

How do folks think we should do that?

One way would be to have that capability enabled globally (either through an API call that gets stored in the DB or via the config file), then reject any switch registrations for switches that didn't support that feature.

Other capabilities (like disabling STP) might be hidden though.

naved001 commented 7 years ago

@zenhack @henn: updated with your recent suggestions.

This actually brings up an interesting point -- DHCP/ARP snooping support can differ based on the switch, but it's something that would only make sense to have globally enabled on all the switches or none of them.

How do folks think we should do that?

One way would be to have that capability enabled globally (either through an API call that gets stored in the DB or via the config file), then reject any switch registrations for switches that didn't support that feature.

Putting it in the config file sounds good to me.

henn commented 7 years ago

Do we really need get_capabilities() now that we have has_capability() ? I don't feel too strongly on this, but I think we should be intentional.

It's always better not to commit to an API until we're sure we need it, since then we'll have to support it forever.

zenhack commented 7 years ago

@henn, show_node wants to list all of the capabilities. Imagine a driver that fishes these out of the database: implementing get_capabilities in terms of have_capability means doing a round-trip to the db for every potential element of the list.

I don't have an objection to only supporting one of these, but I think if we do that get_capabilities should be the primitive.

henn commented 7 years ago

@henn, show_node wants to list all of the capabilities. Imagine a driver that fishes these out of the database: implementing get_capabilities in terms of have_capability means doing a round-trip to the db for every potential element of the list.

Ya - that might be worth having both. I had been thinking that the common case would be has_capability(), which could eventually be implemented as an SQLAlchemy exists() call when we move the capabilities into the DB. Exists() would likely be much faster/more efficient than fetching + collating all of the entries in the table.

zenhack commented 7 years ago

The point of hiding this behind a method at all is to keep the implementation independent from the interface -- using .exists() assumes DB storage, which may not be the case for all drivers.

naved001 commented 7 years ago

@henn and @zenhack

so what's the consensus? We are going with get_capabilities() then?

zenhack commented 7 years ago

get_capabilities() would be my preference. @henn?

naved001 commented 7 years ago

sent an email to @henn, if he doesn't respond by today I'll dismiss his review and get someone else to review.

zenhack commented 7 years ago

No response from @henn, so I've dismissed the review.

zenhack commented 7 years ago

@Izhmash, would you do a second review?

ianballou commented 7 years ago

After some clarifications by @naved001, this looks good to me.

okrieg commented 7 years ago

One comment, the only capabilities that should be exposed are capabilities needed to interact with HIL, e.g., capability of jumbo frames, or capability of supporting no native VLANs; these are capabilities that depend on both the nature of the hardware and how HILs drivers interact with it. What you don't want is to make HIL the source of truth for the characteristics of the hardware. We want discovery services.. for that.