Change in vdsm[master]: net: test_setupNetworks_bond_with_custom_option with ifcfg p...

Petr Horáček has uploaded a new change for review.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

net: test_setupNetworks_bond_with_custom_option with ifcfg persistence

This test asserts bonding options in running configuration, but it is
not available with ifcfg persistence.

When ifcfg persistence is enabled, check bonding options in ifcfg file.

Change-Id: I178383e2c0125a1a661108ca9bbb28da74b1a11a
Signed-off-by: Petr Horáček 
---
M tests/functional/networkTests.py
1 file changed, 10 insertions(+), 2 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/67/43467/1

diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py
index 5752c7e..d64b533 100644
--- a/tests/functional/networkTests.py
+++ b/tests/functional/networkTests.py
@@ -2468,8 +2468,16 @@
                 NOCHK, test_kernel_config=False)
             self.assertEqual(status, SUCCESS, msg)
             self.assertBondExists(BONDING_NAME, nics)
-            opts = self.vdsm_net.config.bonds.get(BONDING_NAME).get('options')
-            self.assertEquals(opts, 'mode=balance-rr')
+            if vdsm.config.config.get('vars', 'net_persistence') == 'unified':
+                opts = self.vdsm_net.config.bonds.get(
+                    BONDING_NAME).get('options')
+                self.assertEquals(opts, 'mode=balance-rr')
+            else:  # ifcfg persistence
+                with open(NET_CONF_PREF + BONDING_NAME) as f:
+                    for l in f:
+                        if l.startswith('BONDING_OPTS='):
+                            self.assertIn('mode=balance-rr', l)
+                            self.assertNotIn('custom=foo', l)

status, msg = self.setupNetworks(
                 {}, {BONDING_NAME: {'remove': True}}, NOCHK)

[e-mail] has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Petr Horáček has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 1: Verified+1

Petr Horáček has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 1:

Tests is OK with unified persistence, it fails with ifcfg persistence:

======================================================================
ERROR: test_setupNetworks_bond_with_custom_option (functional.networkTests.NetworkTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 206, in wrapper
    func(*args, **kwargs)
  File "/usr/share/vdsm/tests/testValidation.py", line 105, in wrapper
    return f(*args, **kwargs)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 2483, in test_setupNetworks_bond_with_custom_option
    {}, {BONDING_NAME: {'remove': True}}, NOCHK)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 362, in setupNetworks
    self._assert_kernel_config_matches_running_config()
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 371, in _assert_kernel_config_matches_running_config
    normalized_config = kernel_config.normalize(running_config)
  File "/usr/lib/python2.7/site-packages/vdsm/netconfpersistence.py", line 313, in normalize
    self._normalize_bridge(config_copy)
  File "/usr/lib/python2.7/site-packages/vdsm/netconfpersistence.py", line 330, in _normalize_bridge
    for net_attr in config_copy.networks.itervalues():
AttributeError: 'NoneType' object has no attribute 'networks'

But it is not related to the test.

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/43467/1/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 2474:                 self.assertEquals(opts, 'mode=balance-rr')
Line 2475:             else:  # ifcfg persistence
Line 2476:                 with open(NET_CONF_PREF + BONDING_NAME) as f:
Line 2477:                     for l in f:
Line 2478:                         if l.startswith('BONDING_OPTS='):
why don't you use a simple equality?

BONDING_OPTS=mode=balance-rr

do you expect having something else there?
Line 2479:                             self.assertIn('mode=balance-rr', l)
Line 2480:                             self.assertNotIn('custom=foo', l)
Line 2481: 
Line 2482:             status, msg = self.setupNetworks(

Petr Horáček has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/43467/1/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 2474:                 self.assertEquals(opts, 'mode=balance-rr')
Line 2475:             else:  # ifcfg persistence
Line 2476:                 with open(NET_CONF_PREF + BONDING_NAME) as f:
Line 2477:                     for l in f:
Line 2478:                         if l.startswith('BONDING_OPTS='):
> why don't you use a simple equality?

Because of there could be (the is) a new line char and we would have to test both cases, with and without \n. But it is probably ok to compare it with just the usual line.
Line 2479:                             self.assertIn('mode=balance-rr', l)
Line 2480:                             self.assertNotIn('custom=foo', l)
Line 2481: 
Line 2482:             status, msg = self.setupNetworks(

[e-mail] has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Petr Horáček has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 2: Verified+1

test "passed" (it failed afterwards on assert_kernel_config_matches_running_config) with both ifcfg and unified persistence

Ido Barkan has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 2:

you should rebase on master, where setupNetworks doe snot test kernelConfig if persistence =='ifcfg'

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 2: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/43467/2/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 2458:             self.vdsm_net.save_config()
Line 2459: 
Line 2460:     @cleanupNet
Line 2461:     @ValidateRunningAsRoot
Line 2462:     def test_setupNetworks_bond_with_custom_option(self):
please state that this test only checks that "custom=foo" is does not harm the configuration of the bond.
Line 2463:         with dummyIf(2) as nics:
Line 2464:             status, msg = self.setupNetworks(
Line 2465:                 {},
Line 2466:                 {BONDING_NAME: {'nics': nics,

Line 2467:                                 'options': 'custom=foo mode=balance-rr'}},
Line 2468:                 NOCHK, test_kernel_config=False)
Line 2469:             self.assertEqual(status, SUCCESS, msg)
Line 2470:             self.assertBondExists(BONDING_NAME, nics)
Line 2471:             if vdsm.config.config.get('vars', 'net_persistence') == 'unified':
it is a functional test - why don't you just add the expected bond options to assertBondExists() ?
Line 2472:                 opts = self.vdsm_net.config.bonds.get(
Line 2473:                     BONDING_NAME).get('options')
Line 2474:                 self.assertEquals(opts, 'mode=balance-rr')
Line 2475:             else:  # ifcfg persistence

[e-mail] has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 4:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Petr Horáček has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 4: Verified+1

Test passed with both unified and ifcfg persistence.

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 4: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/43467/4/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 2624:         with dummyIf(2) as nics:
Line 2625:             status, msg = self.setupNetworks(
Line 2626:                 {},
Line 2627:                 {BONDING_NAME: {'nics': nics,
Line 2628:                                 'options': 'custom=foo mode=802.3ad'}},
I find that this patch fixes the wrong bug: with unified persistence, we should persist custom=foo. Let's solve this issue first.
Line 2629:                 NOCHK, test_kernel_config=False)
Line 2630:             self.assertEqual(status, SUCCESS, msg)
Line 2631:             self.assertBondExists(BONDING_NAME, nics)
Line 2632:             if vdsm.config.config.get('vars', 'net_persistence') == 'unified':

Line 2628:                                 'options': 'custom=foo mode=802.3ad'}},
Line 2629:                 NOCHK, test_kernel_config=False)
Line 2630:             self.assertEqual(status, SUCCESS, msg)
Line 2631:             self.assertBondExists(BONDING_NAME, nics)
Line 2632:             if vdsm.config.config.get('vars', 'net_persistence') == 'unified':
again: let's not test Vdsm's implementation here. Functional tests should test the API, and if Vdsm reports the correct values, we should be happy.
Line 2633:                 opts = self.vdsm_net.config.bonds.get(
Line 2634:                     BONDING_NAME).get('options')
Line 2635:                 self.assertEquals(opts, 'mode=802.3ad')
Line 2636:             else:  # ifcfg persistence

Petr Horáček has abandoned this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Abandoned

Was replaced by https://gerrit.ovirt.org/#/c/43891/

[e-mail] has posted comments on this change.

Change subject: net: test_setupNetworks_bond_with_custom_option with ifcfg persistence
......................................................................

Patch Set 4:

* Update tracker::IGNORE, no Bug-Url found