Ido Barkan has posted comments on this change. Change subject: net: introducing KernelConfig ......................................................................
Patch Set 14: (12 comments) https://gerrit.ovirt.org/#/c/41973/14/lib/vdsm/netconfpersistence.py File lib/vdsm/netconfpersistence.py: Line 209: for bond, bond_attr in netinfo.bondings.iteritems(): Line 210: yield bond, self._translate_netinfo_bond(bond_attr) Line 211: Line 212: def _translate_netinfo_net(self, net, net_attr): Line 213: nics, vlan, bonds = self._netinfo.getNicsVlanAndBondingForNetwork(net) > bonds->bonding Done Line 214: attributes = {} Line 215: self._translate_bridged(attributes, net_attr) Line 216: self._translate_mtu(attributes, net_attr) Line 217: self._translate_vlan(attributes, vlan) Line 231: Line 232: def _translate_ipaddr(self, attributes, net_attr): Line 233: attributes['bootproto'] = 'dhcp' if net_attr['dhcpv4'] else 'none' Line 234: attributes['dhcpv6'] = net_attr['dhcpv6'] Line 235: ifcfg = net_attr.get('cfg') > TODO: we must not depend on 'cfg', which is configurator-dependent. Look up Done Line 236: if ifcfg and ifcfg.get('DEFROUTE') == 'yes': Line 237: attributes['defaultRoute'] = True Line 238: else: Line 239: attributes['defaultRoute'] = False Line 236: if ifcfg and ifcfg.get('DEFROUTE') == 'yes': Line 237: attributes['defaultRoute'] = True Line 238: else: Line 239: attributes['defaultRoute'] = False Line 240: # only report ip addresses of non dhcp interfaces. This is because we > I believe the following comment would be clearer Done Line 241: # don't know if the address is already configured. Line 242: if attributes['bootproto'] == 'none': Line 243: if net_attr['addr']: Line 244: attributes['ipaddr'] = net_attr['addr'] Line 300: net_attr['vlan'] = str(net_attr['vlan']) Line 301: Line 302: def _normalize_bridge(self, config_copy): Line 303: for net_attr in config_copy.networks.itervalues(): Line 304: bridged = net_attr.get('bridged') > get(..., True) Done Line 305: if bridged or bridged is None: Line 306: net_attr['bridged'] = True Line 307: net_attr['stp'] = self._netinfo.stpBooleanize( Line 308: net_attr.get('stp')) Line 330: return config_copy Line 331: Line 332: def _normalize_bonding_opts(self, config_copy): Line 333: for bond, bond_attr in config_copy.bonds.iteritems(): Line 334: non_default_opts = self._netinfo.getNonDefaultBondingOptions(bond) > _normalize*() functions should not read netinfo. Done Line 335: bond_opts = self._parse_bond_options(bond_attr.get('options', '')) Line 336: if bond_opts: Line 337: normalized_opts = {} Line 338: for k, v in bond_opts.iteritems(): Line 352: for bond_attr in config_copy.bonds.itervalues(): Line 353: if 'nics' in bond_attr: Line 354: bond_attr['nics'].sort() Line 355: Line 356: def _normalize_custom_properties(self, config_copy): > drop this function, and add hack to tests of custom properties. Done Line 357: # custom properties are not part of the kernel and are expected to be Line 358: # different Line 359: for net_attr in config_copy.networks.itervalues(): Line 360: if 'custom' in net_attr: Line 362: Line 363: def _normalize_address(self, config_copy): Line 364: for net_attr in config_copy.networks.itervalues(): Line 365: prefix = net_attr.pop('prefix', None) Line 366: if prefix: > if prefix is not None Done Line 367: net_attr['netmask'] = self._netinfo.prefix2netmask(int(prefix)) Line 368: if 'ipv6addr' in net_attr: Line 369: net_attr['ipv6addr'] = [net_attr['ipv6addr']] Line 370: Line 368: if 'ipv6addr' in net_attr: Line 369: net_attr['ipv6addr'] = [net_attr['ipv6addr']] Line 370: Line 371: def _parse_bond_options(self, opts): Line 372: return dict((pair.split('=') for pair in opts.split())) > split('=', 1) Done Line 373: Line 374: def _translate_net_qos(self, net_qos): Line 375: stripped_qos = {} Line 376: for part, part_config in net_qos.iteritems(): Line 370: Line 371: def _parse_bond_options(self, opts): Line 372: return dict((pair.split('=') for pair in opts.split())) Line 373: Line 374: def _translate_net_qos(self, net_qos): > these two _tranlate methods feel very lonely down here. Done Line 375: stripped_qos = {} Line 376: for part, part_config in net_qos.iteritems(): Line 377: stripped_qos[part] = dict(part_config) Line 378: for curve, curve_config in part_config.iteritems(): Line 371: def _parse_bond_options(self, opts): Line 372: return dict((pair.split('=') for pair in opts.split())) Line 373: Line 374: def _translate_net_qos(self, net_qos): Line 375: stripped_qos = {} > please add a docstring withan example of net_qos. it is much simpler then it looks. I changed it's name name and added a mighty docstring. Line 376: for part, part_config in net_qos.iteritems(): Line 377: stripped_qos[part] = dict(part_config) Line 378: for curve, curve_config in part_config.iteritems(): Line 379: stripped_qos[part][curve] = dict((k, v) for k, v Line 375: stripped_qos = {} Line 376: for part, part_config in net_qos.iteritems(): Line 377: stripped_qos[part] = dict(part_config) Line 378: for curve, curve_config in part_config.iteritems(): Line 379: stripped_qos[part][curve] = dict((k, v) for k, v > it seems that we duplicate content, which does make much sense to me. it's not duplicated, it's too many nesting levels for a human mind to grasp. see docstring. Line 380: in curve_config.iteritems() Line 381: if v != 0) Line 382: return stripped_qos Line 383: https://gerrit.ovirt.org/#/c/41973/14/tests/functional/networkTests.py File tests/functional/networkTests.py: Line 1050: The test should assert that it" : " is true (it doesn't) > The test should assert that the reported MTUs are equal to the requested on Done -- To view, visit https://gerrit.ovirt.org/41973 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I79f1eb553a42f1398ad12aa1bc33522f8af30c79 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ido Barkan <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
