Change in vdsm[master]: net: OVS netinfo
edwardh at redhat.com
edwardh at redhat.com
Tue May 10 07:06:59 UTC 2016
Edward Haas has posted comments on this change.
Change subject: net: OVS netinfo
......................................................................
Patch Set 27: Code-Review-1
(11 comments)
https://gerrit.ovirt.org/#/c/56972/26/lib/vdsm/network/ovs/info.py
File lib/vdsm/network/ovs/info.py:
Line 109:
Line 110: return {'slaves': slaves, 'active_slave': active_slave,
Line 111: 'fake_iface': fake_iface, 'mode': mode, 'lacp': lacp}
Line 112:
Line 113: def southbound_port(self, bridge):
> I'd prefer to do that in another patch. There is also another problem: What
If there are multiple southbound ports, we have a configuration corruption, and we need to recover (teardown the bridge and recreate it is an option).
As the decision to have a single southbound port is in the core design, representing it in our data structure seems fair and can serve as a protection to a configuration corroption.
I'm ok with patching this optimization later on.
Line 114: for port, attrs in six.iteritems(self.bridges[bridge]['ports']):
Line 115: if attrs['level'] == SOUTHBOUND:
Line 116: return port
Line 117: return None
https://gerrit.ovirt.org/#/c/56972/27/lib/vdsm/network/ovs/info.py
File lib/vdsm/network/ovs/info.py:
PS27, Line 120: for port, attrs in six.iteritems(self.bridges[bridge]['ports']):
: if attrs['level'] == NORTHBOUND:
: yield port, attrs
Will this work?
return (port, attrs for port, attrs in six.iteritems(ports) if attrs['level'] == NORTHBOUND)
Same for bonds
PS27, Line 135: networking
nit: _netinfo?
PS27, Line 138: bridge
You can pass 'ports' instead.
Same for northbound_ports() and bonds().
PS27, Line 141: net
northbound_port
As there is no nicer way to do it (maybe you can come up with one), perhaps just add a comment:
# The northbound port represents a net
PS27, Line 153: port_attrs
Only used once, better call it with port_attrs['tag']
Same for bridge_attrs['stp']
PS27, Line 153: port
northbound
PS27, Line 155: ports[southbound]['bond']
southbound_bond = ports[southbound]['bond']
and reuse it...
Line 172: def _get_net_ports(bridge, northbound, southbound, ports):
Line 173: net_ports = []
Line 174: for port, port_attrs in six.iteritems(ports):
Line 175: tag = port_attrs['tag']
Line 176: if tag == ports[northbound]['tag'] and port != bridge:
I do not fully understand this one.
For the northbound(net) tag, except the bridge port itself:
Ports that are with the same tag except the northbound port are reported.
Other ports that have a tag are reported as vlan ifaces.
But the 'other', includes not only southbound ports, but also possible taps etc...
Feels like this one should have been done in _bridge_attr (OvsInfo), having there {'vlans': {vlan_id: [<ports>]}}
Line 177: if port != northbound:
Line 178: net_ports.append(port.name)
Line 179: elif tag is not None:
Line 180: expected_vlan_name = '%s.%s' % (southbound, tag)
PS27, Line 194: if bond_attrs['fake_iface']:
: bond_info.update(_get_iface_info(port, addresses, routes))
Why do we need to report the bond interface with the network IP info?
Isn't this needed for faking bridgeless only?
Line 200:
Line 201: def _to_bond_opts(mode, lacp):
Line 202: opts = {}
Line 203: if mode or lacp:
Line 204: custom_opts = []
You can just update opts directly and there id no need for checking 'if mode or lacp', the individual checks are enough.
Line 205: if mode:
Line 206: custom_opts.append('ovs_mode:%s' % mode)
Line 207: if lacp:
Line 208: custom_opts.append('ovs_lacp:%s' % lacp)
--
To view, visit https://gerrit.ovirt.org/56972
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5dd351faf108fd04afa78208d9c34451a856d4a
Gerrit-PatchSet: 27
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list