Dan Kenigsberg has posted comments on this change. Change subject: net: introducing KernelConfig ......................................................................
Patch Set 4: Code-Review-1 (5 comments) incomplete review https://gerrit.ovirt.org/#/c/41973/4/lib/vdsm/netconfpersistence.py File lib/vdsm/netconfpersistence.py: Line 33: # The persistent path is inside of an extra "persistence" dir in order to get Line 34: # oVirt Node to persist the symbolic links that are necessary for the Line 35: # atomic storage of running config into persistent config. Line 36: CONF_PERSIST_DIR = constants.P_VDSM_LIB + 'persistence/netconf/' Line 37: BONDING_MODES = { Please add a TODO: avoid code duplication with similar dicts in modules.py and netinfo.BONDING_DEFAULTS Can it be private? Line 38: '0': 'balance-rr', '1': 'active-backup', '2': 'balance-xor', Line 39: '3': 'broadcast', '4': '802.3ad', '5': 'balance-tlb', '6': 'balance-alb' Line 40: } Line 41: Line 428: else: # isolated bridged network Line 429: return [] Line 430: Line 431: Line 432: def unicodize(d): I'm not sure what is the motivation for this function. I do know that we have a bug about storing vlanid as a string instead of an int in the unified persistence. Is it related? I believe it could (and should) be private. https://gerrit.ovirt.org/#/c/41973/4/lib/vdsm/netinfo.py File lib/vdsm/netinfo.py: Line 306: Line 307: def prefix2netmask(prefix): Line 308: if not 0 <= prefix <= 32: Line 309: raise ValueError('%s is not a valid prefix value. It must be between ' Line 310: '0 and 32' % prefix) unrelated bugfix. It deserves its own backportable patch. Line 311: return socket.inet_ntoa( Line 312: struct.pack("!I", int('1' * prefix + '0' * (32 - prefix), 2))) Line 313: Line 314: https://gerrit.ovirt.org/#/c/41973/4/tests/functional/networkTests.py File tests/functional/networkTests.py: Line 330: self.assertIn(bondName, netinfo.bondings) Line 331: self.assertEqual(netinfo.bondings[bondName]['active_slave'], '') Line 332: Line 333: def setupNetworks(self, *args, **kwargs): Line 334: status, msg = self.vdsm_net.setupNetworks(*args, **kwargs) this local wrapper creates SO much graphical noise. It seems to me much cleaner and simpler to edit the existing setupNetworks wrapper in tests/functional/utils.py. Even if you disagree, you'd have to tweak that function - or else you "kill" the _caller feature of it. Line 335: self._assert_kernel_config_matches_running_config() Line 336: return status, msg Line 337: Line 338: def _assert_kernel_config_matches_running_config(self): Line 1049: @cleanupNet Line 1050: @permutations([[True], [False]]) Line 1051: @brokentest("This test fails because the 2 different networks are getting" Line 1052: " configured with the same MTU. The test should assert that it" Line 1053: " is true (it doesn't).") the text here is not clear. If I understood correctly, we have a vdsm *bug* that is found by this test. If so, please explain the bug here (or open a BZ entry about it). Line 1054: def testSetupNetworksMtus(self, bridged): Line 1055: JUMBO = '9000' Line 1056: MIDI = '4000' Line 1057: -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ido Barkan <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
