Change in vdsm[master]: Advertise aggregator ID in bonding interfaces

edwardh at redhat.com edwardh at redhat.com
Mon Feb 15 07:04:34 UTC 2016


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 <sshnaidm 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: Sagi Shnaidman <sshnaidm at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list