Corsa rainbow vlans #3
Conversation
| @@ -100,7 +100,7 @@ def add_network(self, segmentation_id, network_id, of_controller=None): | |||
| url_switch = protocol + sw_ip_addr | |||
|
|
|||
| #./create-vfc.py br1 5 openflow VFC-1 192.168.201.164 6653 100-105 | |||
| c_br_res = self.config['dafaultVFCRes'] | |||
|
|
||
|
|
||
| isVFCHost = True | ||
| if not 'VFCHost' in self.config or not self.config['VFCHost'] == 'True': |
There was a problem hiding this comment.
I'm surprised this config value isn't coerced into a boolean for you, I would have thought not self.config['VFCHost'] would be enough.
There was a problem hiding this comment.
It bombs if this config value isn't set. We wanted to have the default be the same behavior as before this setting was needed (i.e. when there is only one switch like at TACC)
There was a problem hiding this comment.
Yes, I just thought the config object (which is created as a result of parsing a text file?) should have coerced 'True' into True and 'False' into False. Not commenting on the logic per se :)
|
|
||
| isVFCHost = True | ||
| if not 'VFCHost' in self.config or not self.config['VFCHost'] == 'True': | ||
| LOG.info("PRUTH: Skipping VFC Creation: isVFCHost " + str(isVFCHost)) |
There was a problem hiding this comment.
Are these debug statements still useful? Should they at least be logged as LOG.debug level instead?
| if not 'VFCHost' in self.config or not self.config['VFCHost'] == 'True': | ||
| LOG.info("PRUTH: Skipping VFC Creation: isVFCHost " + str(isVFCHost)) | ||
| isVFCHost = False | ||
| return |
There was a problem hiding this comment.
Given the return here, it looks like there is no need to have the isVFCHost variable set, as it will always be True if it makes it past this line, therefore it is tautological.
There was a problem hiding this comment.
What you say is correct. At least for now, there is nothing to do if it is not a VFC host. We were thinking that we might dynamically plumb the VLANs on those switches but ultimatly decided to statically provision them.
| isVFCHost = True | ||
| if not 'VFCHost' in self.config or not self.config['VFCHost'] == 'True': | ||
| LOG.info("PRUTH: Skipping VFC Deletion: isVFCHost " + str(isVFCHost)) | ||
| isVFCHost = False |
There was a problem hiding this comment.
Same comment as above; I don't think this var is necessary.
There was a problem hiding this comment.
Same comment about static provisioning.
| token = self.config['token'] | ||
| headers = {'Authorization': token} | ||
|
|
||
| logging.basicConfig(format='%(levelname)s:%(message)s', level=logging.DEBUG) |
There was a problem hiding this comment.
This overrides the base logging configuration, which is global - if it is going to be used, it should be placed prominently at the top of the file.
There was a problem hiding this comment.
I don't think this is necessary. Mert, do you know why this is here?
|
|
||
| try: | ||
| r = requests.get(info_url, headers=headers, verify=False) | ||
| except Exception as e: |
There was a problem hiding this comment.
This except block can be removed if it is not doing anything but re-raising.
There was a problem hiding this comment.
This is a utility file that interacts directly with the switch. There is a lot more functionality that we might need to add here as we add features.
I think these functions need to be filled out to check the returned error status (like the others) but we haven't done that yet.
| corsavfc.bridge_attach_tunnel_ctag_vlan(headers, url_switch, br_id = br_id, ofport = ofport, port = int(dst_port), vlan_id = node_vlan) | ||
|
|
||
| except Exception as e: | ||
| raise e |
There was a problem hiding this comment.
You don't need to catch exceptions if they are being re-raised.
There was a problem hiding this comment.
Same as the other one... I think this needs to check error codes and raise a new exception.
|
|
||
| dst_port=None | ||
| if dst_switch_name in self.config: | ||
| dst_port=self.config[dst_switch_name] |
There was a problem hiding this comment.
what if dst_switch_name is not in the config? If it's required, I would recommend raising a ValueError or similar.
Also, in Python a nicer way of doing this, which avoids the error you get when you try to reference a non-existent key, is to use the .get function on a dict:
dst_port = self.config.get(dst_switch_name) # will return None if not found.
if dst_port is None:
raise ValueError('Missing configuration for switch {}'.format(dst_switch_name))
|
I didn't get the option to add context to my review - I was leaving some style suggestions and a few questions. I didn't spot anything of note in the logic or flow of the code itself, though I'm a bit shaky on what the code is doing and why, just due to not having a clear understanding yet of how the NGS plugin works in general. |
No description provided.