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