Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 9:

(7 comments)

I addressed some of the comments and I am leaving two of them for the patch set 
11.

https://gerrit.ovirt.org/#/c/56059/9/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 2957: for nic in nics:
Line 2958: self.assertNotIn('ad_aggregator_id', nic_caps[nic])
Line 2959: 
Line 2960: @cleanupNet
Line 2961: @permutations([['none'], ['ipv4'], ['dhcpv4']])
> Lets try and loose the permutations...
Done
Line 2962: @ValidateRunningAsRoot
Line 2963: def test_consume_nm_bond(self, ip_config):
Line 2964: options = dict(bond_mode='802.3ad', miimon='200')
Line 2965: vdsm_network = {'bonding': BONDING_NAME, 'bridged': True,


PS9, Line 2966: 'dhcp' if ip_config == 'dhcpv4' else
  : 'none'
> Please take it out to its own variable (bootproto) so it will be more reada
Done


Line 2980:dhcp_range_from=DHCP_RANGE_FROM,
Line 2981:dhcp_range_to=DHCP_RANGE_TO,
Line 2982:ipv4_gateway=IP_GATEWAY)
Line 2983: 
Line 2984: with nm_controlled_bond(BONDING_NAME, **options) as 
(conn_name,
> From the usage, I'm not clear why a contextmanager is used.
For querying information about the returned connection conn_name could be 
useful, but not now.

The contextmananger ensures that ifcfg files created by NetworkManager (with 
different, incompatible names to those initscripts/VDSM create) are removed 
after the test finishes.

I also had it set up the environment for the DHCP variant of the test, but 
since I am splitting the test into three, I will clean up this contextmanager, 
but not in the next iteration.
Line 2985:  slaves):
Line 2986: self.vdsm_net.refreshNetinfo()
Line 2987: self.assertBondExists(BONDING_NAME, slaves,
Line 2988:   'mode=4 miimon=200',


https://gerrit.ovirt.org/#/c/56059/9/tests/network/nettestlib.py
File tests/network/nettestlib.py:

Line 414: return wrapper
Line 415: 
Line 416: 
Line 417: @contextmanager
Line 418: def wait_for_address(iface, families=None, wait_for_scopes=None, 
timeout=20):
> This change seems to belong to a separate patch.
Done
Line 419: """
Line 420: Wait for the iface to get IP addresses of a specified family with 
netlink
Line 421: Monitor.
Line 422: """


PS9, Line 423: if not families:
 : families = (6,)
> What about setting this default at the func signature?
Done


PS9, Line 427: link
> For IPv4, usually there will be no link scope.
I plan to focus on scopes when testing this branch as I have also been 
concerned about IPv4LL.


https://gerrit.ovirt.org/#/c/56059/9/tests/network/networkmanager.py
File tests/network/networkmanager.py:

PS9, Line 31: nettestlib
> nm_controlled_bond is a test-only creature, that should move to nettestlib.
Done


-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 9:

(1 comment)

https://gerrit.ovirt.org/#/c/56059/9/tests/network/networkmanager.py
File tests/network/networkmanager.py:

PS9, Line 31: nettestlib
> There is a need to nicely split NM related functionality from test services
nm_controlled_bond is a test-only creature, that should move to nettestlib. the 
command-line wrappers _nmcli* should become public, be renamed (_nmcli prefix 
dropped) and stay in this module.


-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[edwardh]

Edward Haas has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 9: Code-Review-1

(8 comments)

https://gerrit.ovirt.org/#/c/56059/9/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 2957: for nic in nics:
Line 2958: self.assertNotIn('ad_aggregator_id', nic_caps[nic])
Line 2959: 
Line 2960: @cleanupNet
Line 2961: @permutations([['none'], ['ipv4'], ['dhcpv4']])
Lets try and loose the permutations...
It makes reading the test harder and their meaning is hidden in the 
implementation. I prefer having a clear test name that describes what it tests 
exactly.
Line 2962: @ValidateRunningAsRoot
Line 2963: def test_consume_nm_bond(self, ip_config):
Line 2964: options = dict(bond_mode='802.3ad', miimon='200')
Line 2965: vdsm_network = {'bonding': BONDING_NAME, 'bridged': True,


PS9, Line 2966: 'dhcp' if ip_config == 'dhcpv4' else
  : 'none'
Please take it out to its own variable (bootproto) so it will be more readable.


Line 2980:dhcp_range_from=DHCP_RANGE_FROM,
Line 2981:dhcp_range_to=DHCP_RANGE_TO,
Line 2982:ipv4_gateway=IP_GATEWAY)
Line 2983: 
Line 2984: with nm_controlled_bond(BONDING_NAME, **options) as 
(conn_name,
From the usage, I'm not clear why a contextmanager is used.
I have the feeling that nm_controlled_bond embeds some test resources that are 
not related to the actual bond creation using NM.

Why do we need 'conn_name' returned?
Line 2985:  slaves):
Line 2986: self.vdsm_net.refreshNetinfo()
Line 2987: self.assertBondExists(BONDING_NAME, slaves,
Line 2988:   'mode=4 miimon=200',


PS9, Line 2986: self.vdsm_net.refreshNetinfo()
  : self.assertBondExists(BONDING_NAME, slaves,
  :   'mode=4 miimon=200',
  :   assert_in_running_conf=False)
  : bond_info = 
self.vdsm_net.netinfo.bondings[BONDING_NAME]
  : if ip_config == 'ipv4':
  : self.assertEqual(DHCP_RANGE_FROM, 
bond_info['addr'])
  : self.assertEqual(IP_MASK, bond_info['netmask'])
  : self.assertEqual(ip_config == 'dhcpv4', 
bond_info['dhcpv4'])
These are integration tests, checking that the nm_controlled_bond worked 
correctly, so it will be better to create an integration test and push it there.

The only part that may be relevant here is to show that the bond existed and is 
under NM control before issuing setupNetworks.

So I would expect only two asserts before calling setupNetworks:
assertBondExists(...)
assertBondOwnedByNM(...)


https://gerrit.ovirt.org/#/c/56059/9/tests/network/nettestlib.py
File tests/network/nettestlib.py:

Line 414: return wrapper
Line 415: 
Line 416: 
Line 417: @contextmanager
Line 418: def wait_for_address(iface, families=None, wait_for_scopes=None, 
timeout=20):
This change seems to belong to a separate patch.
Line 419: """
Line 420: Wait for the iface to get IP addresses of a specified family with 
netlink
Line 421: Monitor.
Line 422: """


PS9, Line 423: if not families:
 : families = (6,)
What about setting this default at the func signature?
(A default immutable object is safe)


PS9, Line 427: link
For IPv4, usually there will be no link scope.
And the fact that the ipv4 tests passes with this implementation means we have 
a bug here.

We were supposed to wait for all scopes in wait_for_scopes, and not be ok with 
only a single one.


https://gerrit.ovirt.org/#/c/56059/9/tests/network/networkmanager.py
File tests/network/networkmanager.py:

PS9, Line 31: nettestlib
There is a need to nicely split NM related functionality from test services 
usage.
You can think of it like this: We should be able to move networkmanager module 
to the production code.

Test services should be used in the test.
I hope that after splitting the two, the complexity will be reduced.


-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: 

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[automation]

gerrit-hooks has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 9:

* #1304509::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1304509::OK, public bug
* Check Product::#1304509::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6'])

-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 8:

(5 comments)

https://gerrit.ovirt.org/#/c/56059/8/tests/functional/networkTests.py
File tests/functional/networkTests.py:

PS8, Line 2996: # VERIFY: if NM is configured with [main] 
monitor-connection-files=
  : # true, it should unmanage the bond and its slaves 
as soon as VDSM
  : # writes the respective ifcfg files
> please drop the comment now
Done


https://gerrit.ovirt.org/#/c/56059/8/tests/network/nettestlib.py
File tests/network/nettestlib.py:

PS8, Line 27: from multiprocessing import Process
> unrelated move
Done


Line 445: else:
Line 446: raise
Line 447: 
Line 448: 
Line 449: def _nmcli_connection_add(device_name, conn_type, conn_name=None,
> let's put these functions in a new nm module (still under tests/network)
Done
Line 450:   autoconnect=None, persistent=None, 
master=None,
Line 451:   bond_mode=None, miimon=None, 
ipv4_address=None,
Line 452:   ipv4_gateway=None):
Line 453: nmcli_command = [_NMCLI.cmd, '--terse', 'connection', 'add',


Line 551: 
Line 552: if dhcp_server_ipv4 and dhcp_server_ipv4_cidr:
Line 553: for port in router_ports:
Line 554: addrAdd(port, dhcp_server_ipv4, dhcp_server_ipv4_cidr)
Line 555: # TODO? 'nmcli device connect' instead, just to be 
consistent?
> I don't mind, you can drop the TODO.
I just tried the command and it is not even equivalent.
Line 556: linkSet(port, ['up'])
Line 557: 
Line 558: if dhcp_range_from and dhcp_range_to:
Line 559: with dnsmasq_run(router_ports, dhcp_range_from, 
dhcp_range_to,


PS8, Line 569: netlink.Monitor
> this is relatively easy, please do that.
Done


-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 8: Code-Review-1

(7 comments)

https://gerrit.ovirt.org/#/c/56059/8/tests/functional/networkTests.py
File tests/functional/networkTests.py:

PS8, Line 2996: # VERIFY: if NM is configured with [main] 
monitor-connection-files=
  : # true, it should unmanage the bond and its slaves 
as soon as VDSM
  : # writes the respective ifcfg files
please drop the comment now


https://gerrit.ovirt.org/#/c/56059/8/tests/network/nettestlib.py
File tests/network/nettestlib.py:

PS8, Line 27: from multiprocessing import Process
unrelated move


Line 445: else:
Line 446: raise
Line 447: 
Line 448: 
Line 449: def _nmcli_connection_add(device_name, conn_type, conn_name=None,
let's put these functions in a new nm module (still under tests/network)
Line 450:   autoconnect=None, persistent=None, 
master=None,
Line 451:   bond_mode=None, miimon=None, 
ipv4_address=None,
Line 452:   ipv4_gateway=None):
Line 453: nmcli_command = [_NMCLI.cmd, '--terse', 'connection', 'add',


Line 551: 
Line 552: if dhcp_server_ipv4 and dhcp_server_ipv4_cidr:
Line 553: for port in router_ports:
Line 554: addrAdd(port, dhcp_server_ipv4, dhcp_server_ipv4_cidr)
Line 555: # TODO? 'nmcli device connect' instead, just to be 
consistent?
I don't mind, you can drop the TODO.
Line 556: linkSet(port, ['up'])
Line 557: 
Line 558: if dhcp_range_from and dhcp_range_to:
Line 559: with dnsmasq_run(router_ports, dhcp_range_from, 
dhcp_range_to,


PS8, Line 561: mode!
please open an NM bugzilla. the comment here is not clear enough for me.


Line 558: if dhcp_range_from and dhcp_range_to:
Line 559: with dnsmasq_run(router_ports, dhcp_range_from, 
dhcp_range_to,
Line 560:  router=ipv4_gateway):
Line 561: # TODO: NM tries to use incompatible options with 
this mode!
Line 562: # TODO: clear these from the log: "SELinux is 
preventing agetty
better open a BZ on the relevant component.

but you can leave

# TODO: understand why "SELinux is preventing agetty from using the net_admin 
capability" are reported to /var/log/messages
Line 563: # from using the net_admin capability"
Line 564: with _nm_connection(device_name=bond_name, 
conn_type='bond',
Line 565: slaves=local_nics_to_enslave,
Line 566: bond_mode=bond_mode,


PS8, Line 569: netlink.Monitor
this is relatively easy, please do that.


-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[automation]

gerrit-hooks has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 8:

* #1304509::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1304509::OK, public bug
* Check Product::#1304509::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6'])

-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[automation]

gerrit-hooks has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 7:

* #1304509::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1304509::OK, public bug
* Check Product::#1304509::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6'])

-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[automation]

gerrit-hooks has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 6:

* #1304509::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1304509::OK, public bug
* Check Product::#1304509::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6'])

-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[automation]

gerrit-hooks has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 5:

* #1304509::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1304509::OK, public bug
* Check Product::#1304509::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6'])

-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[automation]

gerrit-hooks has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 4:

* #1304509::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1304509::OK, public bug
* Check Product::#1304509::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6'])

-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/56059/3/tests/functional/networkTests.py
File tests/functional/networkTests.py:

PS3, Line 3000: miimon=200
This is mysteriously lost in kernelconfig (reset to 0, even though NM's default 
is 100).


-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 3: Code-Review-1

-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[automation]

gerrit-hooks has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 3:

* #1304509::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1304509::OK, public bug
* Check Product::#1304509::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6'])

-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli

[automation]

gerrit-hooks has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli
..


Patch Set 2:

* #1304509::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1304509::OK, public bug
* Check Product::#1304509::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6'])

-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli (WIP)

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli (WIP)
..


Patch Set 1:

(1 comment)

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

PS1, Line 2983: rc, out, err = execCmd([NMCLI.cmd,
  : 'connection', 'modify', 
'--temporary',
  : conn_name,
  : 'ipv4.method', 'auto',
  : ])
I think ipv4.method is auto by default.


-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli (WIP)

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli (WIP)
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/56059/1/tests/network/nettestlib.py
File tests/network/nettestlib.py:

PS1, Line 402: firewall.allow_dhcp
With log messages like "veth_xxx not in any zone" I doubt we need this 
protection today.


-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli (WIP)

[automation]

gerrit-hooks has posted comments on this change.

Change subject: tests: let VDSM consume a bond created by nmcli (WIP)
..


Patch Set 1:

* #1304509::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1304509::OK, public bug
* Check Product::#1304509::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6'])

-- 
To view, visit https://gerrit.ovirt.org/56059
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: let VDSM consume a bond created by nmcli (WIP)

[osvoboda]

Ondřej Svoboda has uploaded a new change for review.

Change subject: tests: let VDSM consume a bond created by nmcli (WIP)
..

tests: let VDSM consume a bond created by nmcli (WIP)

This test should serve at this point to automate the process
of VDSM adopting networks managed by NetworkManager, to explore
and debug their competition over the network parts.

TODO: - figure out why NM won't run dhclient automatically,
what is it waiting for? (VDSM connects alright)
  - actually, it seems to run its dhclient when VDSM
ifup's the bridge
  - wait for NM's dhclient obtaining an address with
netlink.Monitor

Change-Id: I7047ce59a515d0b8ed2c4c5307b4c0d47d4aa92b
Bug-Url: https://bugzilla.redhat.com/1304509
Signed-off-by: Ondřej Svoboda 
---
M tests/functional/networkTests.py
M tests/network/dhcp.py
M tests/network/nettestlib.py
3 files changed, 126 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/59/56059/1

diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py
index 52130c1..e07d586 100644
--- a/tests/functional/networkTests.py
+++ b/tests/functional/networkTests.py
@@ -19,6 +19,7 @@
 from contextlib import contextmanager
 from functools import wraps
 import json
+import logging
 import re
 import os.path
 import six
@@ -2955,3 +2956,120 @@
 self.assertNotIn('ad_partner_mac', bond_caps[bond])
 for nic in nics:
 self.assertNotIn('ad_aggregator_id', nic_caps[nic])
+
+@contextmanager
+def nm_connection(self, device_name, conn_type, slaves=None, dhcpv4=False):
+if conn_type != 'bond':
+raise NotImplementedError('Only bonded NM connections supported.')
+
+NMCLI = CommandPath('nmcli', '/usr/bin/nmcli')
+
+conn_name = '{}-{}-{}'.format(conn_type, device_name, 123)
+rc, out, err = execCmd([NMCLI.cmd,
+'connection', 'add',
+'type', conn_type,
+'ifname', device_name,  # e.g. the bond name
+'con-name', conn_name,
+'autoconnect', 'yes',  # already a default
+'save', 'no',
+'mode', '802.3ad',
+])
+logging.debug('nmcli conn add type bond: %s (%s, %s)', rc,
+  ''.join(out), ''.join(err))
+if rc == 8:
+raise SkipTest('NetworkManager is not running.')
+
+if dhcpv4:
+rc, out, err = execCmd([NMCLI.cmd,
+'connection', 'modify', '--temporary',
+conn_name,
+'ipv4.method', 'auto',
+])
+logging.debug('nmcli conn modify: %s (%s, %s)', rc,
+  ''.join(out), ''.join(err))
+
+if conn_type == 'bond' and slaves:
+for slave in slaves:
+rc, out, err = execCmd([NMCLI.cmd,
+'connection', 'add',
+'type', 'bond-slave',
+'ifname', slave,
+'save', 'no',
+'master', device_name,
+])
+logging.debug('nmcli conn add type bond-slave: %s (%s, %s)',
+  rc, ''.join(out), ''.join(err))
+linkSet(slave, ['up'])
+
+rc, out, err = execCmd([NMCLI.cmd, 'connection', 'up', conn_name])
+logging.debug('nmcli conn up bond: %s (%s, %s)', rc,
+  ''.join(out), ''.join(err))
+
+try:
+yield
+finally:
+if slaves:
+for slave in slaves:
+rc, out, err = execCmd([NMCLI.cmd,
+'connection', 'delete',
+'bond-slave-' + slave])
+logging.debug('nmcli conn delete bond-slave: %s (%s, %s)',
+  rc, ''.join(out), ''.join(err))
+
+rc, out, err = execCmd([NMCLI.cmd,
+'connection', 'delete', conn_name])
+logging.debug('nmcli conn delete bond: %s (%s, %s)', rc,
+  ''.join(out), ''.join(err))
+
+@contextmanager
+def nm_controlled_bond(self):
+with veth_pair() as (n1, n2), veth_pair() as (n3, n4):
+addrAdd(n1, IP_ADDRESS, IP_CIDR)
+linkSet(n1, ['up'])
+addrAdd(n3, IP_ADDRESS, IP_CIDR)
+linkSet(n3, ['up'])
+
+# The address pool is limited so the bond would always get the same
+#