CCI-MOC / hil

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

Read inactive vlans #998

Closed ianballou closed 6 years ago

ianballou commented 6 years ago
ianballou commented 6 years ago

@zenhack I've added some documentation for the Dell switch, if the docs there are good I can add them to the other drivers too.

Overall, my get_port_networks() iterates through ports' port configurations and gets every native + non-native trunking VLAN mentioned in the configuration. The majority of the code here is string parsing, which some logic that branches off depending on if the switch reports VLANs or if it reports 'none'.

I also cleaned up the strange moving of lines in the n3000 driver.

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 1917


Changes Missing Coverage Covered Lines Changed/Added Lines %
hil/ext/switches/nexus.py 0 17 0.0%
hil/ext/switches/_dell_base.py 0 29 0.0%
<!-- Total: 0 46 0.0% -->
Files with Coverage Reduction New Missed Lines %
hil/ext/switches/_dell_base.py 1 0.0%
hil/ext/switches/nexus.py 2 0.0%
<!-- Total: 3 -->
Totals Coverage Status
Change from base Build 1914: 0.3%
Covered Lines: 2140
Relevant Lines: 3857

💛 - Coveralls
naved001 commented 6 years ago

I don't understand how this fixes the bug? in #921 the discussion settled on the problem probably being the newline, not the inactive string. I don't understand where the solution is here?

I think @Izhmash ended up rewriting the whole method so that in addition to reading inactive vlans, it also parses the vlan range correctly.

zenhack commented 6 years ago

Ah, okay, I understand what's going on now.

ianballou commented 6 years ago

You've got a fair amount of copypasta from one driver to another. Ideally we'd factor out the common bits.

The n3000 function I wrote is the same as the PowerConnect's; I can have them use the same code. I'll look into factoring out the Nexus code too.

ianballou commented 6 years ago

So I've removed the n3000 port networks codes and created a unified one in _dell_base. The n3000 and PowerConnect are very similar; only small changes in key word ordering and none representations got in the way.

I've tried to make it more readable by separating some of the code out.

ianballou commented 6 years ago

My latest commit allows the networking deployment tests to run successfully. Tested for Nexus and PowerConnect.

ianballou commented 6 years ago

N3000 deployment tests working as well.

ianballou commented 6 years ago

I've added a deployment test that tests the discovery of multiple (>2) networks on one port using get_port_networks.

ianballou commented 6 years ago

I think this is ready for another review

ianballou commented 6 years ago

@naved001 I fixed those up.

ianballou commented 6 years ago

@zenhack I think I addressed your comments. Also I'd like to test this again since it's been a little while, I'll put in a comment when that's done. Just waiting on some fixes to the testing network.

ianballou commented 6 years ago

Deployment tests are still passing