Edward Haas has posted comments on this change. Change subject: Advertise aggregator ID in bonding interfaces ......................................................................
Patch Set 14: Code-Review-1 (5 comments) I like! Just some suggestions for the test. https://gerrit.ovirt.org/#/c/53100/14/tests/functional/networkTests.py File tests/functional/networkTests.py: Line 2850: bonds If we can make the test more explicit and avoid parsing based on input structure, it will be better. And maybe naming bond_info to bond_caps (or similar). Will this work: ? bond_caps = info['bondings'] Line 2852: nics Same here Line 2850: bond_info = [info['bondings'][b] for b in bonds] Line 2851: bond_ag_macs = [b.get('ad_partner_mac') for b in bond_info] Line 2852: nics_info = [info['nics'][nic] for nic in nics] Line 2853: Line 2854: if bond_options == 'mode=4 lacp_rate=1 miimon=100': If we have such differences inside the test due to the permutations, it will be more clear to just have two tests and use helpers for common code. Line 2855: macs = list(reversed([b['hwaddr'] for b in bond_info])) Line 2856: for bond in bond_info: Line 2857: self.assertIn('ad_aggregator_id', bond) Line 2858: self.assertIn('ad_partner_mac', bond) Line 2859: self.assertEqual(macs, bond_ag_macs) I would prefer to see what is compared here exactly, not as a group. Like mac of bond-slave A equals mac detected in bond-agg B. Same for all the rest.. Line 2865: self.assertNotIn('ad_aggregator_id', bond) Line 2866: self.assertNotIn('ad_partner_mac', bond) Line 2867: for nic_info in nics_info: Line 2868: self.assertNotIn('ad_aggregator_id', nic_info) Line 2869: for bond in bonds: What about having a context manager for creating/removing the bonds? Seems to fit here nicely. Line 2870: status, msg = self.setupNetworks( Line 2871: {}, {bond: {'remove': True}}, NOCHK) Line 2872: self.assertEqual(status, SUCCESS, msg) -- To view, visit https://gerrit.ovirt.org/53100 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85267967c9cb1b0a626d91cb1953361ed4de727a Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Sagi Shnaidman <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Edward Haas <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: Sagi Shnaidman <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
