Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 15:

Edy, was it on EL7 where you ran the tests? I believe it was because of ELs 
that we needed waiting for bridges (or at least we were afraid of them not 
appearing timely).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 15
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 14: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 14
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[danken]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


ifcfg: re-enable IPv6 before device configuration, or disable afterwards

If a network has been restored with no IPv6 requested, all its devices
have disable_ipv6 set to 1 and up to now there was no other way to
re-enable IPv6 than doing so manually or rebooting.

This patch enables IPv6 on the top-level device ("the network") prior
to ifup, sets the underlying devices' disable_ipv6 to 1 as they are
not directly involved in communication. If IPv6 is not requested, it
is disabled on all devices after ifup.

Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Signed-off-by: Ondřej Svoboda 
Reviewed-on: https://gerrit.ovirt.org/54555
Continuous-Integration: Jenkins CI
Reviewed-by: Edward Haas 
Tested-by: Edward Haas 
Reviewed-by: Dan Kenigsberg 
---
M lib/vdsm/network/configurators/ifcfg.py
M tests/functional/networkTests.py
2 files changed, 80 insertions(+), 26 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Dan Kenigsberg: Looks good to me, approved
  Edward Haas: Verified; Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 15
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 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 15:

* Update tracker: IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 15
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 14: Verified+1

Unit and Functional (networking) tests passed.

Ran 118 tests in 749.023s
OK (SKIP=6)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 14
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 14:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 14
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 13:

(2 comments)

https://gerrit.ovirt.org/#/c/54555/13/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 789: ifup because it may have been disabled during network restoration 
after
Line 790: boot. If IPv6 is not requested, disable it after ifup.
Line 791: """
Line 792: if iface.ipv6:
Line 793: # If IPv6 was disabled on boot, 
FileNotFoundError/IOError(ENOENT) will
> This is regarding the remark and the addition check on line 799:
I removed the extra care.
Line 794: # be raised when accessing a nonexistent sysctl knob.
Line 795: enable_ipv6(iface.name)
Line 796: 
Line 797: _exec_ifup_by_name(iface.name, cgroup)


https://gerrit.ovirt.org/#/c/54555/13/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 2190: 
self.assertEqual(getRouteDeviceTo(IP_ADDRESS_IN_NETWORK), nic)
Line 2191: finally:
Line 2192: addrFlush(nic)
Line 2193: 
Line 2194: # Silently skip this part if IPv6 was disabled on boot.
> We expect IPv6 to be enabled (at boot), there is no need to check this per 
Done
Line 2195: if ipv6_supported():
Line 2196: enable_ipv6(nic)
Line 2197: addrAdd(nic, IPv6_ADDRESS, IPv6_CIDR, family=6)
Line 2198: try:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 13
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 13:

(1 comment)

https://gerrit.ovirt.org/#/c/54555/13/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

> I still think that there is no need to touch this file, but I prefer this p
To make backporting simple, I'll move the code to ifcfg. And if needed by other 
configurators (which, BTW, still use wait_for_device), I'll move it back here.
Line 1: # Copyright 2013-2014 Red Hat, Inc.
Line 2: #
Line 3: # This program is free software; you can redistribute it and/or modify
Line 4: # it under the terms of the GNU General Public License as published by


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 13
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 13: Code-Review-1

(3 comments)

https://gerrit.ovirt.org/#/c/54555/13/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

I still think that there is no need to touch this file, but I prefer this patch 
in over my opinion on this.
Line 1: # Copyright 2013-2014 Red Hat, Inc.
Line 2: #
Line 3: # This program is free software; you can redistribute it and/or modify
Line 4: # it under the terms of the GNU General Public License as published by


https://gerrit.ovirt.org/#/c/54555/13/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 789: ifup because it may have been disabled during network restoration 
after
Line 790: boot. If IPv6 is not requested, disable it after ifup.
Line 791: """
Line 792: if iface.ipv6:
Line 793: # If IPv6 was disabled on boot, 
FileNotFoundError/IOError(ENOENT) will
This is regarding the remark and the addition check on line 799:
Same remark as in the tests: IPv6 is expected to be enabled system wide, we do 
not support running on a host that has IPv6 disabled system wide.

* If we should, it will can be validated at the API entry point... but I don't 
see the need for this at the moment.
Line 794: # be raised when accessing a nonexistent sysctl knob.
Line 795: enable_ipv6(iface.name)
Line 796: 
Line 797: _exec_ifup_by_name(iface.name, cgroup)


https://gerrit.ovirt.org/#/c/54555/13/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 2190: 
self.assertEqual(getRouteDeviceTo(IP_ADDRESS_IN_NETWORK), nic)
Line 2191: finally:
Line 2192: addrFlush(nic)
Line 2193: 
Line 2194: # Silently skip this part if IPv6 was disabled on boot.
We expect IPv6 to be enabled (at boot), there is no need to check this per test.
The tests should actually fail, not skip.

If you want to add this check in general, I would suggest doing so at module 
level and fail the whole module (in a separated patch).
Line 2195: if ipv6_supported():
Line 2196: enable_ipv6(nic)
Line 2197: addrAdd(nic, IPv6_ADDRESS, IPv6_CIDR, family=6)
Line 2198: try:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 13
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 13:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 13
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 12:

(4 comments)

https://gerrit.ovirt.org/#/c/54555/12/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

PS12, Line 214: ipv6_supported
> We seem to need this only when enable=False.
Good point. We better not be silent when the user orders to set up an IPv6 
network on a system where it is not possible. So I extracted the condition out.


PS12, Line 227: logging.warning
> Do we really need it? Won't we explode if something unexpected will happen?
I am afraid I don't follow you.

If you question the purpose of the context manager, well, the code touching 
possibly inexistent sysctl nodes is more likely to explode, unlike other parts 
of ifcfg, which delegates dirty work to initscripts.

But I cannot recall if this in fact protects us more from actually missing 
devices or from the missing /proc/sys/net/ipv6 tree.


https://gerrit.ovirt.org/#/c/54555/12/tests/functional/networkTests.py
File tests/functional/networkTests.py:

PS12, Line 1888: @cleanupNet
> Just checking with you: If the test fail, do we see the correct backtrace? 
This is how the test would fail before https://gerrit.ovirt.org/#/c/55078/ was 
merged. We can see both the inner and the outer function in the backtrace.

No dummies were left over after the tests.

[root@localhost tests]# cd /usr/share/vdsm/tests/; ip link del dev bond0; 
NOSE_TESTMATCH='test_setupNetworks_swap_slaves_between_bonds|test_static_ip_configuration_'
 ./run_tests.sh --enable-slow-tests functional/networkTests.py
Cannot find device "bond0"
nose.config: INFO: Ignoring files matching ['^\\.', '^_', '^setup\\.py$']
functional.networkTests.NetworkTest
test_setupNetworks_swap_slaves_between_bondsOK
test_static_ip_configuration_dual_to_v4 FAIL
test_static_ip_configuration_dual_to_v6_and_backOK
test_static_ip_configuration_v4_to_dual OK

==
FAIL: test_static_ip_configuration_dual_to_v4 
(functional.networkTests.NetworkTest)
--
Traceback (most recent call last):
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 1883, in 
test_static_ip_configuration_dual_to_v4
self._test_static_ip_configuration(([4, 6], [4]))
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 213, in wrapper
func(*args, **kwargs)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 1926, in 
_test_static_ip_configuration
change_ip_configuration_and_verify(ip_families)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 1909, in 
change_ip_configuration_and_verify
self.assertEqual(status, SUCCESS, msg)
AssertionError: Nic dummy_w13LD already enslaved to bond1
 >> begin captured logging << 
root: DEBUG: /usr/bin/taskset --cpu-list 0-0 /usr/sbin/tc qdisc show (cwd None)
root: DEBUG: SUCCESS:  = '';  = 0
root: INFO: Adding bond0({'nics': ['dummy_YIvgK', 'dummy_gPt3w'], 'options': 
'mode=0'})
root: INFO: Adding bond1({'nics': ['dummy_mN94G', 'dummy_w13LD'], 'options': 
'mode=0'})
SuperVdsmProxy: DEBUG: Trying to connect to Super Vdsm
- >> end captured logging << -

--
Ran 4 tests in 48.035s

FAILED (failures=1)


PS12, Line 1889: _test_static_ip_configuration
> Thanks, now I understand what is actually tested.
I haven't seen the helper running: what matters is the 'test' prefix.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 12
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 12:

(4 comments)

https://gerrit.ovirt.org/#/c/54555/12/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

PS12, Line 214: ipv6_supported
We seem to need this only when enable=False.
When the want to enable IPv6 and there is no support for it, we better explode.

If we want to disable IPv6, it makes sense, but why ask to disable something 
that does not exist... isn't it the caller responsibility?


PS12, Line 227: logging.warning
Do we really need it? Won't we explode if something unexpected will happen? The 
test will surely fail.
We did expect this to happen even with no relation to the waiting part, right?

BTW: In what scenario we found it to be missing before?


https://gerrit.ovirt.org/#/c/54555/12/tests/functional/networkTests.py
File tests/functional/networkTests.py:

PS12, Line 1888: @cleanupNet
Just checking with you: If the test fail, do we see the correct backtrace? 
Usually @cleanupNet is ran on the test method and not the helper...
If it works as expected then it looks good to me.


PS12, Line 1889: _test_static_ip_configuration
Thanks, now I understand what is actually tested.

Are you sure this helper function is not ran by the test runner directly? 
Asking because it has the string 'test' in it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 12
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 12:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 12
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 11:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 11
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 10:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 10
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 9:

(1 comment)

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

Line 1879: @permutations([[([4], [6])],
Line 1880:[([6], [4])],
Line 1881:[([4], [4, 6], [4])]])
Line 1882: @cleanupNet
Line 1883: def testStaticNetworkConfig(self, ip_reconfigurations):
> Aha, maybe I understand now. Do you suggest to extract the inner function a
I mean, to extract the inside of the outer function.
Line 1884: with dummyIf(1) as nics:
Line 1885: nic, = nics
Line 1886: IPv4 = dict(nic=nic, bootproto='none', ipaddr=IP_ADDRESS,
Line 1887: netmask=IP_MASK, gateway=IP_GATEWAY)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 9:

(2 comments)

https://gerrit.ovirt.org/#/c/54555/9/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 216: wait_for_device
> I think we had a problem with devices coming up, not about their creation/e
The tests on F23 showed no need for waiting so I'll add a warning when a device 
doesn't exist and retest.


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

Line 1879: @permutations([[([4], [6])],
Line 1880:[([6], [4])],
Line 1881:[([4], [4, 6], [4])]])
Line 1882: @cleanupNet
Line 1883: def testStaticNetworkConfig(self, ip_reconfigurations):
> I find a lot of tests hard to understand, including this one (before your a
Aha, maybe I understand now. Do you suggest to extract the inner function and 
just call it from different tests? That sounds okay.
Line 1884: with dummyIf(1) as nics:
Line 1885: nic, = nics
Line 1886: IPv4 = dict(nic=nic, bootproto='none', ipaddr=IP_ADDRESS,
Line 1887: netmask=IP_MASK, gateway=IP_GATEWAY)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 9:

(2 comments)

https://gerrit.ovirt.org/#/c/54555/9/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 216: wait_for_device
> This is nothing new. I remember back when adding disable_ipv6 initially tha
I think we had a problem with devices coming up, not about their 
creation/existence.


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

Line 1879: @permutations([[([4], [6])],
Line 1880:[([6], [4])],
Line 1881:[([4], [4, 6], [4])]])
Line 1882: @cleanupNet
Line 1883: def testStaticNetworkConfig(self, ip_reconfigurations):
> Isn't code duplication implied by splitting the test? How about I add comme
I find a lot of tests hard to understand, including this one (before your 
addition). I hope (but not hard require) not to add to this.
Hard to read tests are evil and must be refactored for the good of all man 
kind. :)

There does not need to be duplication if you split it, calling the same 
method/func with different input is not considered duplication.

This is my preference.
If you prefer documenting it instead, I'll live with it.
Line 1884: with dummyIf(1) as nics:
Line 1885: nic, = nics
Line 1886: IPv4 = dict(nic=nic, bootproto='none', ipaddr=IP_ADDRESS,
Line 1887: netmask=IP_MASK, gateway=IP_GATEWAY)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 9:

(1 comment)

https://gerrit.ovirt.org/#/c/54555/9/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 216: wait_for_device
> I don't understand this one.
This is nothing new. I remember back when adding disable_ipv6 initially that 
even bridges, just the links, took some time to show up in the system. Let me 
try to remove the waiting and run tests locally.

I think that moving the delay somewhere else would just complicate things. 
Here, the logic is all in one place.

If the device already exists then wait_for_device return immediately.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 9:

(1 comment)

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

Line 1879: @permutations([[([4], [6])],
Line 1880:[([6], [4])],
Line 1881:[([4], [4, 6], [4])]])
Line 1882: @cleanupNet
Line 1883: def testStaticNetworkConfig(self, ip_reconfigurations):
> I find it hard to read this test and if it will fail, it will be painful to
Isn't code duplication implied by splitting the test? How about I add 
comments/docstring to explain the purpose and phases of the test?

I find most of the tests hard to read. Do you feel the same about all tests or 
are my changes on this one just too much?
Line 1884: with dummyIf(1) as nics:
Line 1885: nic, = nics
Line 1886: IPv4 = dict(nic=nic, bootproto='none', ipaddr=IP_ADDRESS,
Line 1887: netmask=IP_MASK, gateway=IP_GATEWAY)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 9: Code-Review-1

(3 comments)

https://gerrit.ovirt.org/#/c/54555/9/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 216: wait_for_device
I don't understand this one.
The caller creates a device and then asks to wait for a device to be created? 
What if it was already created?
If you want to make sure a device is created before proceeding (blocking on 
it), it needs to be done in the creation context.

I know there are races with links coming up, but not with their creation. Do we 
really need to be so pedantic?


https://gerrit.ovirt.org/#/c/54555/9/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 805:  args=(iface, cgroup))
Line 806: t.daemon = True
Line 807: t.start()
Line 808: else:
Line 809: if not iface.master and (iface.ipv4 or iface.ipv6):
Nice catch
Line 810: if iface.ipv4:
Line 811: expected_event = {'label': iface.name, 'family': 
'inet',
Line 812:   'scope': 'global'}
Line 813: elif iface.ipv6:


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

Line 1879: @permutations([[([4], [6])],
Line 1880:[([6], [4])],
Line 1881:[([4], [4, 6], [4])]])
Line 1882: @cleanupNet
Line 1883: def testStaticNetworkConfig(self, ip_reconfigurations):
I find it hard to read this test and if it will fail, it will be painful to 
understand which scenario failed.

Can you please split this test into multiple ones?
Each should check a specific scenario and have a name that fits it.
Line 1884: with dummyIf(1) as nics:
Line 1885: nic, = nics
Line 1886: IPv4 = dict(nic=nic, bootproto='none', ipaddr=IP_ADDRESS,
Line 1887: netmask=IP_MASK, gateway=IP_GATEWAY)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 9:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 8:

This may be related to _wait_for_event in _ifup which expects an IPv6 
link-local address.

==
FAIL: testSetupNetworksMultiMTUsOverNic(False) 
(functional.networkTests.NetworkTest)
--
Traceback (most recent call last):
  File "/usr/share/vdsm/tests/testlib.py", line 73, in wrapper
return f(self, *args)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 213, in wrapper
func(*args, **kwargs)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 1253, in 
testSetupNetworksMultiMTUsOverNic
mtu=1400)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 1235, in 
_createVlanedNetOverNicAndCheck
self.assertEquals(status, SUCCESS, msg)
AssertionError: Unexpected exception
 >> begin captured logging << 
root: DEBUG: /usr/bin/taskset --cpu-list 0-0 /usr/sbin/tc qdisc show (cwd None)
root: DEBUG: SUCCESS:  = '';  = 0
root: INFO: Adding network test-network0({'vlan': 27, 'nic': 'dummy_i9fhY', 
'dhcpv6': False, 'mtu': 1500, 'bootproto': 'none', 'bridged': False, 
'defaultRoute': False})
root: DEBUG: /usr/bin/taskset --cpu-list 0-0 /usr/sbin/tc qdisc show (cwd None)
root: DEBUG: SUCCESS:  = '';  = 0
root: INFO: Adding network test-network0({'vlan': 27, 'nic': 'dummy_i9fhY', 
'dhcpv6': False, 'mtu': 1500, 'bootproto': 'none', 'bridged': False, 
'defaultRoute': False})
- >> end captured logging << -


MainProcess|Thread-654::ERROR::2016-03-21 
11:17:23,454::supervdsmServer::96::SuperVdsm.ServerCallback::(wrapper) Error in 
setupNetworks
Traceback (most recent call last):
  File "/usr/share/vdsm/supervdsmServer", line 94, in wrapper
res = func(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/vdsm/network/api.py", line 226, in 
setupNetworks
_setup_networks(networks, bondings, options)
  File "/usr/lib/python2.7/site-packages/vdsm/network/api.py", line 268, in 
_setup_networks
bondings, _netinfo)
  File "/usr/lib/python2.7/site-packages/vdsm/network/legacy_switch.py", line 
465, in add_missing_networks
_netinfo=_netinfo, **attrs)
  File "/usr/lib/python2.7/site-packages/vdsm/network/legacy_switch.py", line 
175, in wrapped
return func(network, configurator, **kwargs)
  File "/usr/lib/python2.7/site-packages/vdsm/network/legacy_switch.py", line 
245, in _add_network
net_ent_to_configure.configure(**options)
  File "/usr/lib/python2.7/site-packages/vdsm/network/models.py", line 142, in 
configure
self.configurator.configureVlan(self, **opts)
  File "/usr/lib/python2.7/site-packages/vdsm/network/configurators/ifcfg.py", 
line 113, in configureVlan
_ifup(vlan)
  File "/usr/lib/python2.7/site-packages/vdsm/network/configurators/ifcfg.py", 
line 820, in _ifup
_exec_ifup(iface, cgroup)
  File "/usr/lib64/python2.7/contextlib.py", line 24, in __exit__
self.gen.next()
  File "/usr/lib/python2.7/site-packages/vdsm/network/configurators/ifcfg.py", 
line 961, in _wait_for_event
mon.stop()
  File "/usr/lib/python2.7/site-packages/vdsm/netlink/monitor.py", line 164, in 
stop
raise MonitorError(E_NOT_RUNNING)
MonitorError: 1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 8:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 7:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 6:

(5 comments)

https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 211: 
Line 212: 
Line 213: def enable_ipv6(device_name, enable=True, await_device=False):
Line 214: if ipv6_supported():
Line 215: if await_device:
> have you not agreed that there is no need to wait for the device to be up?
wait_for_device (above) returns as soon as the devices is created, not up. I 
know I called the parameter 'await_device' wrongly at first.
Line 216: wait_for_device(device_name)
Line 217: # In broken networks, the device may be missing.
Line 218: with _pretend_path_exists():
Line 219: sysctl.disable_ipv6(device_name, disable=not enable)


https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 532
Line 533
Line 534
Line 535
Line 536
> sorry, I do not understand your response.
Well, there would not be an actual name clash, but still this variable has the 
same name as the newly-imported function.


Line 767: iface
> Having an argument just for one specific call is not recommended.
First, I'll send a simpler-looking version of this function, and later (for 
clarity) I'll probably extract the part which only needs the device name.


Line 767: iface
> Done
The only unusual caller (start_devices) is sufficiently explicit about the 
exceptional values of the 'iface' (None) and iface_name (not None) parameters.


https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/models.py
File lib/vdsm/network/models.py:

Line 73: def backing_device(self):
Line 74: return False
Line 75: 
Line 76: @property
Line 77: def top_level_device(self):
> why? haven't we decided to treat each device independently?
There is a patch coming to remove this, as I noted in the last comment to the 
patch.
Line 78: return self.master.top_level_device if self.master else self
Line 79: 
Line 80: 
Line 81: class Nic(NetDevice):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 6:

(1 comment)

https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 767: iface
> Done
Having an argument just for one specific call is not recommended.
The need to explain something or comment on it is a code smell already.

Lets find another way to solve this one...
I can think of two options, not sure which is better: 
One is to have the caller fake an iface structure, the other is to split this 
func.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 6:

(4 comments)

https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 211: 
Line 212: 
Line 213: def enable_ipv6(device_name, enable=True, await_device=False):
Line 214: if ipv6_supported():
Line 215: if await_device:
have you not agreed that there is no need to wait for the device to be up?
Line 216: wait_for_device(device_name)
Line 217: # In broken networks, the device may be missing.
Line 218: with _pretend_path_exists():
Line 219: sysctl.disable_ipv6(device_name, disable=not enable)


https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 532
Line 533
Line 534
Line 535
Line 536
> There was a name conflict to begin with.
sorry, I do not understand your response.


Line 767: iface
> Done
come to think of that, how about adding a new function

 _exec_ifup_name

because the suggested semantics is a bit too complex.


https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/models.py
File lib/vdsm/network/models.py:

Line 73: def backing_device(self):
Line 74: return False
Line 75: 
Line 76: @property
Line 77: def top_level_device(self):
> No, it was still used in ifcfg in this version of the patch.
why? haven't we decided to treat each device independently?
Line 78: return self.master.top_level_device if self.master else self
Line 79: 
Line 80: 
Line 81: class Nic(NetDevice):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 6:

(4 comments)

I'll submit to the favoured outcome of this patch for now. I have a gut feeling 
the approach to leave everything but the top-level device disabled caused 
problems in tests but maybe they have been handled by explicitly enabling IPv6 
in the tests.

https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 223: _pretend_path_exists
> _ignore_missing_device() is clearer
Done


https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 532
Line 533
Line 534
Line 535
Line 536
> this changes seems to deserve its own patch and commit message; to me it se
There was a name conflict to begin with.


Line 767: iface
> "if iface is not None" is more precise.
Done


https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/models.py
File lib/vdsm/network/models.py:

Line 73: def backing_device(self):
Line 74: return False
Line 75: 
Line 76: @property
Line 77: def top_level_device(self):
> leftover from former version?
No, it was still used in ifcfg in this version of the patch.
Line 78: return self.master.top_level_device if self.master else self
Line 79: 
Line 80: 
Line 81: class Nic(NetDevice):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 6: Code-Review-1

(4 comments)

https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 223: _pretend_path_exists
_ignore_missing_device() is clearer


https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 532
Line 533
Line 534
Line 535
Line 536
this changes seems to deserve its own patch and commit message; to me it seems 
not directly related.


Line 767: iface
"if iface is not None" is more precise.
Please document in docstring the now-complex logic of this function.


https://gerrit.ovirt.org/#/c/54555/6/lib/vdsm/network/models.py
File lib/vdsm/network/models.py:

Line 73: def backing_device(self):
Line 74: return False
Line 75: 
Line 76: @property
Line 77: def top_level_device(self):
leftover from former version?
Line 78: return self.master.top_level_device if self.master else self
Line 79: 
Line 80: 
Line 81: class Nic(NetDevice):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 6:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

(6 comments)

https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

> I would recommend considering that when it will be needed, not now. At the 
First, I'll apply your suggestions to the function as it stands here.

And since we probably don't want other configurators disadvantaged in the 
strict tests, I'll try to complete the implementation in them, seeing for 
myself if there is the need for common code, or not.
Line 1: # Copyright 2013-2014 Red Hat, Inc.
Line 2: #
Line 3: # This program is free software; you can redistribute it and/or modify
Line 4: # it under the terms of the GNU General Public License as published by


https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 50: 
Line 51: if utils.isOvirtNode():
Line 52: from ovirt.node.utils import fs as node_fs
Line 53: 
Line 54: from . import Configurator, enable_ipv6, getEthtoolOpts
> Please do not fix this in this context.
Done
Line 55: from . import dhclient
Line 56: from . import libvirt
Line 57: from ..errors import ConfigNetworkError, ERR_FAILED_IFUP
Line 58: from ..models import Nic, Bridge, IPv4, IPv6


Line 100: self.runningConfig.save()
Line 101: self.runningConfig = None
Line 102: 
Line 103: def configureBridge(self, bridge, **opts):
Line 104: if bridge.ipv6.requested:
> As I see it, if no IPv6 has been set for a network, none of its devices sho
For the record, I believe we all agreed to keep to my all-or-nothing approach. 
If this is not true, please remind me.

One more point perhaps: while we can rely on the kernel not changing its 
networking behaviour, we cannot be sure about any higher-level components 
(NetworkManager/systemd-networkd/others?). So if we always set disable_ipv6 on 
all devices below the bridge and expected networking to work, we might run into 
problems someday.

I really want the property to have the "natural" value, thus for all devices to 
get their link-local addresses. Unless, by our rule, the top-level device ("the 
network") had IPv6 disabled.

IPv6 is enabled, "natural", on current systems by default, and this is very 
unlikely to change in the foreseeable future.
Line 105: enable_ipv6(bridge.name)
Line 106: self.configApplier.addBridge(bridge, **opts)
Line 107: ifdown(bridge.name)
Line 108: if bridge.port:


https://gerrit.ovirt.org/#/c/54555/5/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 1924: self.assertEqual(IPv6_GATEWAY, 
test_net['ipv6gateway'])
Line 1925: else:
Line 1926: self.assertEqual([], test_net['ipv6addrs'])
Line 1927: 
Line 1928: with self.vdsm_net.pinger():
> Sounds good to me. 
I gave the helper function a better name. I think it is sufficient.
Line 1929: for families in ip_reconfigurations:
Line 1930: run_test_instance(self, families)
Line 1931: 
Line 1932: delete = {NETWORK_NAME: {'remove': True}}


Line 1930: self, 
> this is not a method; do not pass self.
Done


Line 2194: enable_ipv6
> Better use sysctl (and in all other places)
Why? enable_ipv6 protects from trying to touch /proc sysctl when IPv6 is 
disabled on boot. It doesn't wait for the NIC by default, so it is actually a 
useful wrapper even in tests.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5: Verified-1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/54555/5/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 1930: self, 
this is not a method; do not pass self.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5: Verified+1

The failure might be because I've tested this over piotr's yaml tests but I get

==
ERROR: testStaticNetworkConfig(([4], [4, 6], [4])) 
(functional.networkTests.NetworkTest)
--
Traceback (most recent call last):
  File "/usr/share/vdsm/tests/testlib.py", line 73, in wrapper
return f(self, *args)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 213, in wrapper
func(*args, **kwargs)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 1913, in 
testStaticNetworkConfig
run_test_instance(self, families)
TypeError: run_test_instance() takes exactly 1 argument (2 given)

==
ERROR: testStaticNetworkConfig(([4], [6])) (functional.networkTests.NetworkTest)
--
Traceback (most recent call last):
  File "/usr/share/vdsm/tests/testlib.py", line 73, in wrapper
return f(self, *args)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 213, in wrapper
func(*args, **kwargs)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 1913, in 
testStaticNetworkConfig
run_test_instance(self, families)
TypeError: run_test_instance() takes exactly 1 argument (2 given)

==
ERROR: testStaticNetworkConfig(([6], [4])) (functional.networkTests.NetworkTest)
--
Traceback (most recent call last):
  File "/usr/share/vdsm/tests/testlib.py", line 73, in wrapper
return f(self, *args)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 213, in wrapper
func(*args, **kwargs)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 1913, in 
testStaticNetworkConfig
run_test_instance(self, families)
TypeError: run_test_instance() takes exactly 1 argument (2 given)

==
FAIL: testSetupNetworksAddDelDhcp(False, (6,)) 
(functional.networkTests.NetworkTest)
--
Traceback (most recent call last):
  File "/usr/share/vdsm/tests/testlib.py", line 73, in wrapper
return f(self, *args)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 213, in wrapper
func(*args, **kwargs)
  File "/usr/share/vdsm/tests/modprobe.py", line 59, in wrapper
return f(*args, **kwargs)
  File "/usr/share/vdsm/tests/functional/networkTests.py", line 2015, in 
testSetupNetworksAddDelDhcp
self.assertEqual(status, SUCCESS, msg)
AssertionError: Unexpected exception

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 100: self.runningConfig.save()
Line 101: self.runningConfig = None
Line 102: 
Line 103: def configureBridge(self, bridge, **opts):
Line 104: if bridge.ipv6.requested:
> By such a rule, non-top level devices that would "naturally" get an IPv6LL 
As I see it, if no IPv6 has been set for a network, none of its devices should 
have IPv6 at all, not even link scope addresses.
The logic from my side is this: If IPv6 was not mentioned, it should be 
disabled. Or another way to put it, based on how ifcfg configures the 
interface: If ipv6 was not mentioned, IPV6INIT=no, and when that is the case, 
IPv6 should be disabled.

I understand your point on this, especially with the bridge example.
Lets see what is Dan take on this.
Line 105: enable_ipv6(bridge.name)
Line 106: self.configApplier.addBridge(bridge, **opts)
Line 107: ifdown(bridge.name)
Line 108: if bridge.port:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 100: self.runningConfig.save()
Line 101: self.runningConfig = None
Line 102: 
Line 103: def configureBridge(self, bridge, **opts):
Line 104: if bridge.ipv6.requested:
> Consistency is preserved: If there is no IPv6 address, IPv6 is disabled.. T
By such a rule, non-top level devices that would "naturally" get an IPv6LL 
address would actually be subjected to disable_ipv6 regardless of (the 
network's) configuration.

If we manually removed a bridge, its host port would not be directly usable for 
IPv6 without manually re-enabling IPv6. This is bad.

I want disable_ipv6 to be consistent in the chain, either disable_ipv6 
everything, or nothing. The restoration code does exactly this and I like it.
Line 105: enable_ipv6(bridge.name)
Line 106: self.configApplier.addBridge(bridge, **opts)
Line 107: ifdown(bridge.name)
Line 108: if bridge.port:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

(2 comments)

https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 100: self.runningConfig.save()
Line 101: self.runningConfig = None
Line 102: 
Line 103: def configureBridge(self, bridge, **opts):
Line 104: if bridge.ipv6.requested:
> Only the top level device carries IP configuration so I have to query it to
Consistency is preserved: If there is no IPv6 address, IPv6 is disabled.. This 
is true for interfaces with other IP addresses or with ones without any IP 
address (non top level ones).
Line 105: enable_ipv6(bridge.name)
Line 106: self.configApplier.addBridge(bridge, **opts)
Line 107: ifdown(bridge.name)
Line 108: if bridge.port:


https://gerrit.ovirt.org/#/c/54555/5/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 1924: self.assertEqual(IPv6_GATEWAY, 
test_net['ipv6gateway'])
Line 1925: else:
Line 1926: self.assertEqual([], test_net['ipv6addrs'])
Line 1927: 
Line 1928: with self.vdsm_net.pinger():
> Yes and no. Before, a network was configured and removed.
Sounds good to me. 
The permutation hides the intention and reasoning of the test you just 
described. It will be more readable and clear to do it using helper functions 
that describe well what you are testing.
Line 1929: for families in ip_reconfigurations:
Line 1930: run_test_instance(self, families)
Line 1931: 
Line 1932: delete = {NETWORK_NAME: {'remove': True}}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

(5 comments)

https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 208: raise ConfigNetworkError(ERR_FAILED_IFUP, 'Device %s was not 
created '
Line 209:  'during a %ss timeout.' % (name, 
timeout))
Line 210: 
Line 211: 
Line 212: def enable_ipv6(device_name, enable=True, wait_for_ifup=False):
> You are right.
Sure, your version is more readable.
Line 213: if ipv6_supported():
Line 214: if wait_for_ifup:
Line 215: wait_for_device(device_name)
Line 216: try:


https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 100: self.runningConfig.save()
Line 101: self.runningConfig = None
Line 102: 
Line 103: def configureBridge(self, bridge, **opts):
Line 104: if bridge.ipv6.requested:
> This seems to be a common thing to all devices, we can locate it in _ifup o
Only the top level device carries IP configuration so I have to query it to 
achieve consistency.
Line 105: enable_ipv6(bridge.name)
Line 106: self.configApplier.addBridge(bridge, **opts)
Line 107: ifdown(bridge.name)
Line 108: if bridge.port:


https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/models.py
File lib/vdsm/network/models.py:

Line 444: # TODO: remove in favour of the explicit check below
Line 445: def __nonzero__(self):
Line 446: return bool(self.address or self.ipv6autoconf or self.dhcpv6)
Line 447: 
Line 448: @property
> You can suggest this replacement in a separated patch.
I was afraid the use of __nonzero__ (replaced by __bool__ in Python 3) would 
make code review harder. But since you are aware now that I want to move to an 
explicit property, I will do this change to another patch.
Line 449: def requested(self):
Line 450: return bool(self.address or self.ipv6autoconf or self.dhcpv6)
Line 451: 
Line 452: def __repr__(self):


https://gerrit.ovirt.org/#/c/54555/5/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 1922: if 6 in families:
Line 1923: self.assertIn(IPv6_ADDRESS_AND_CIDR, 
test_net['ipv6addrs'])
Line 1924: self.assertEqual(IPv6_GATEWAY, 
test_net['ipv6gateway'])
Line 1925: else:
Line 1926: self.assertEqual([], test_net['ipv6addrs'])
> self.assertEqual(1, sysctl.is_disabled_ipv6(NETWORK_NAME))
I'll add this.
Line 1927: 
Line 1928: with self.vdsm_net.pinger():
Line 1929: for families in ip_reconfigurations:
Line 1930: run_test_instance(self, families)


Line 1924: self.assertEqual(IPv6_GATEWAY, 
test_net['ipv6gateway'])
Line 1925: else:
Line 1926: self.assertEqual([], test_net['ipv6addrs'])
Line 1927: 
Line 1928: with self.vdsm_net.pinger():
> I got confused. Do we have two permutation levels here?
Yes and no. Before, a network was configured and removed.

Now, to simulate some anticipated scenarios of reconfiguration, the network is 
created e.g. as IPv4-only, then changed to dual-stack, and then back to 
IPv4-only.

The key thing I want to test here is that IPv6 is successfully reenabled (after 
being disabled, say, on boot) and disabled again, none of which we had before.

And only then the network is removed.
Line 1929: for families in ip_reconfigurations:
Line 1930: run_test_instance(self, families)
Line 1931: 
Line 1932: delete = {NETWORK_NAME: {'remove': True}}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 208: raise ConfigNetworkError(ERR_FAILED_IFUP, 'Device %s was not 
created '
Line 209:  'during a %ss timeout.' % (name, 
timeout))
Line 210: 
Line 211: 
Line 212: def enable_ipv6(device_name, enable=True, wait_for_ifup=False):
> You are right that the sole existence is sufficient to be able to alter dis
You are right.
Can we 'prettify' using a context manager?

while pathexist(): # I don't have a good name for this one
sysctl.disable_ipv6(device_name, disable=not enable)
Line 213: if ipv6_supported():
Line 214: if wait_for_ifup:
Line 215: wait_for_device(device_name)
Line 216: try:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 208: raise ConfigNetworkError(ERR_FAILED_IFUP, 'Device %s was not 
created '
Line 209:  'during a %ss timeout.' % (name, 
timeout))
Line 210: 
Line 211: 
Line 212: def enable_ipv6(device_name, enable=True, wait_for_ifup=False):
> The only additional logic added here, if I understand correctly, is the wai
You are right that the sole existence is sufficient to be able to alter 
disable_ipv6.

I have used the "ask for forgiveness, not permission" paradigm here because 
generally, it protects you from races (like if the device disappeared between 
the check and the command) and also because you easily learn about 
non-existence by running into the exception, you don't need an explicit check.
Line 213: if ipv6_supported():
Line 214: if wait_for_ifup:
Line 215: wait_for_device(device_name)
Line 216: try:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

> Other configurators will follow, they should adapt the added function only 
I would recommend considering that when it will be needed, not now. At the 
moment it just looks redundant.
Line 1: # Copyright 2013-2014 Red Hat, Inc.
Line 2: #
Line 3: # This program is free software; you can redistribute it and/or modify
Line 4: # it under the terms of the GNU General Public License as published by


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

> As the implementation if for ifcfg only, there is no point in touching this
Other configurators will follow, they should adapt the added function only with 
minor modifications (e.g. no waiting, no checks).
Line 1: # Copyright 2013-2014 Red Hat, Inc.
Line 2: #
Line 3: # This program is free software; you can redistribute it and/or modify
Line 4: # it under the terms of the GNU General Public License as published by


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[edwardh]

Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5: Code-Review-1

(10 comments)

https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

As the implementation if for ifcfg only, there is no point in touching this 
abstraction layer.
We can keep things limited to ifcfg.
Line 1: # Copyright 2013-2014 Red Hat, Inc.
Line 2: #
Line 3: # This program is free software; you can redistribute it and/or modify
Line 4: # it under the terms of the GNU General Public License as published by


Line 208: raise ConfigNetworkError(ERR_FAILED_IFUP, 'Device %s was not 
created '
Line 209:  'during a %ss timeout.' % (name, 
timeout))
Line 210: 
Line 211: 
Line 212: def enable_ipv6(device_name, enable=True, wait_for_ifup=False):
The only additional logic added here, if I understand correctly, is the 
wait_for_ifup.
But I don't think we need it, as the device does not need to be up in order to 
change the disable_ipv6 parameter.

I would also prefer to see an explicit check if the device exists and then 
update it (avoiding the try-except), but it's a nit.
Line 213: if ipv6_supported():
Line 214: if wait_for_ifup:
Line 215: wait_for_device(device_name)
Line 216: try:


https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 50: 
Line 51: if utils.isOvirtNode():
Line 52: from ovirt.node.utils import fs as node_fs
Line 53: 
Line 54: from . import Configurator, enable_ipv6, getEthtoolOpts
Please do not fix this in this context.
Line 55: from . import dhclient
Line 56: from . import libvirt
Line 57: from ..errors import ConfigNetworkError, ERR_FAILED_IFUP
Line 58: from ..models import Nic, Bridge, IPv4, IPv6


Line 100: self.runningConfig.save()
Line 101: self.runningConfig = None
Line 102: 
Line 103: def configureBridge(self, bridge, **opts):
Line 104: if bridge.ipv6.requested:
This seems to be a common thing to all devices, we can locate it in _ifup or 
_exec_ifup.
There is also no need to check about the top level device, each device in the 
chain stands by its own. If no IPv6 address is defined on a device, we can 
disable IPv6 on it.
Line 105: enable_ipv6(bridge.name)
Line 106: self.configApplier.addBridge(bridge, **opts)
Line 107: ifdown(bridge.name)
Line 108: if bridge.port:


Line 792: _ifup
We can do both pre and post actions here:

if iface.ipv6 and device_exists(iface,name):
disable_ipv6(iface.name, disable=False)

...

if not iface.ipv6:
disable_ipv6(iface.name)

Due to the async nature when executing _exec_ifup (in the dhcp case), we should 
move this to the _exec_ifup somehow (and avoid the wait_for_ifup).


https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/models.py
File lib/vdsm/network/models.py:

Line 72: @property
Line 73: def backing_device(self):
Line 74: return False
Line 75: 
Line 76: @property
I don't think we need this one, see comments in ifcfg.py
Line 77: def top_level_device(self):
Line 78: return self.master.top_level_device if self.master else self
Line 79: 
Line 80: 


Line 444: # TODO: remove in favour of the explicit check below
Line 445: def __nonzero__(self):
Line 446: return bool(self.address or self.ipv6autoconf or self.dhcpv6)
Line 447: 
Line 448: @property
You can suggest this replacement in a separated patch.
Line 449: def requested(self):
Line 450: return bool(self.address or self.ipv6autoconf or self.dhcpv6)
Line 451: 
Line 452: def __repr__(self):


https://gerrit.ovirt.org/#/c/54555/5/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 1922: if 6 in families:
Line 1923: self.assertIn(IPv6_ADDRESS_AND_CIDR, 
test_net['ipv6addrs'])
Line 1924: self.assertEqual(IPv6_GATEWAY, 
test_net['ipv6gateway'])
Line 1925: else:
Line 1926: self.assertEqual([], test_net['ipv6addrs'])
self.assertEqual(1, sysctl.is_disabled_ipv6(NETWORK_NAME))
Line 1927: 
Line 1928: with self.vdsm_net.pinger():
Line 1929: for families in ip_reconfigurations:
Line 1930: run_test_instance(self, families)


Line 1924: self.assertEqual(IPv6_GATEWAY, 
test_net['ipv6gateway'])
Line 1925: else:
Line 1926: self.assertEqual([], test_net['ipv6addrs'])
Line 1927: 
Line 1928: with self.vdsm_net.pinger():
I got confused. Do we have two permutation levels here?
What was tested before and what is tested now? And why?

Won't it be simpler to just create a new test and check 

Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

On enabling/disabling IPv6 in the whole hierarchy: you can remember we had 
success pinging a bridge's IPv6 link-local address from a VM.

So maybe it is possible to ping e.g. a VLAN's LL address as well.

No service would likely be running there but if it were possible to communicate 
over IPv6 we should like to disable it altogether.

For now, it just looks cleaner to me when all the devices don't have their ugly 
LL addresses when the top level didn't ask for IPv6, and similarly, that they 
all do when it asked.

If there IS really a complication when the whole hierarchy is disabled, I 
should think about VLANs which share a NIC, and not disable IPv6 on the NIC.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5: Verified+1

All functional tests passed, including testStaticNetworkConfig which now 
executes three IP use-cases which involve disable_ipv6 at some point: 
4->4,6->4; 4->6, 6->4. Let me know if you can think of other progressions.

Manual testing with NIC-only to bridged networks and back, 4->4,6 and 4,6->4, 
was successful as well.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


Patch Set 5:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
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: 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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 4
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 3
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 2
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[automation]

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..


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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
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]: ifcfg: re-enable IPv6 before device configuration, or disabl...

[osvoboda]

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

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable 
afterwards
..

ifcfg: re-enable IPv6 before device configuration, or disable afterwards

If a network has been restored with no IPv6 requested, all its devices have
disable_ipv6 set to 1 and up to now there was no other way to re-enable
IPv6 than doing so manually or rebooting.

This patch enables IPv6 on all devices prior to ifup, or disables IPv6
on them after ifup.

Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85
Signed-off-by: Ondřej Svoboda 
---
M lib/vdsm/network/configurators/__init__.py
M lib/vdsm/network/configurators/ifcfg.py
M lib/vdsm/network/models.py
3 files changed, 39 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/55/54555/1

diff --git a/lib/vdsm/network/configurators/__init__.py 
b/lib/vdsm/network/configurators/__init__.py
index 69e97e7..d8e9e15 100644
--- a/lib/vdsm/network/configurators/__init__.py
+++ b/lib/vdsm/network/configurators/__init__.py
@@ -18,14 +18,17 @@
 #
 
 from __future__ import absolute_import
+import errno
 import logging
 from six.moves import configparser
 
 from vdsm.config import config
 from vdsm.netconfpersistence import RunningConfig
 from vdsm import ipwrapper
+from vdsm.netinfo.misc import ipv6_supported
 from vdsm.netinfo import mtus
 from vdsm.netlink import monitor
+from vdsm import sysctl
 
 from .dhclient import DhcpClient
 from ..errors import ConfigNetworkError, ERR_FAILED_IFUP
@@ -204,3 +207,19 @@
 return
 raise ConfigNetworkError(ERR_FAILED_IFUP, 'Device %s was not created '
  'during a %ss timeout.' % (name, timeout))
+
+
+def enable_ipv6(device_name, enable=True, wait_for_ifup=False):
+if ipv6_supported():
+if wait_for_ifup:
+wait_for_device(device_name)
+try:
+sysctl.disable_ipv6(device_name, disable=not enable)
+except IOError as e:
+if e.errno == errno.ENOENT:
+   # In broken networks, a device may be missing. If wait_for_ifup
+   # was used however, ConfigNetworkError would already have been
+   # raised.
+   pass
+else:
+raise
diff --git a/lib/vdsm/network/configurators/ifcfg.py 
b/lib/vdsm/network/configurators/ifcfg.py
index 395dd31..eb1f7da 100644
--- a/lib/vdsm/network/configurators/ifcfg.py
+++ b/lib/vdsm/network/configurators/ifcfg.py
@@ -44,7 +44,6 @@
 from vdsm.netinfo import (bonding as netinfo_bonding, mtus, nics, vlans, misc,
   NET_PATH)
 from vdsm.netinfo.cache import ifaceUsed
-from vdsm import sysctl
 from vdsm import utils
 from vdsm.netconfpersistence import RunningConfig, PersistentConfig
 from vdsm.netlink import monitor
@@ -52,7 +51,9 @@
 if utils.isOvirtNode():
 from ovirt.node.utils import fs as node_fs
 
-from . import Configurator, dhclient, getEthtoolOpts, libvirt, wait_for_device
+from . import Configurator, enable_ipv6, getEthtoolOpts
+from . import dhclient
+from . import libvirt
 from ..errors import ConfigNetworkError, ERR_FAILED_IFUP
 from ..models import Nic, Bridge, IPv4, IPv6
 from ..sourceroute import StaticSourceRoute, DynamicSourceRoute
@@ -100,24 +101,26 @@
 self.runningConfig = None
 
 def configureBridge(self, bridge, **opts):
+if bridge.ipv6.requested:
+enable_ipv6(bridge.name)
 self.configApplier.addBridge(bridge, **opts)
 ifdown(bridge.name)
 if bridge.port:
 bridge.port.configure(**opts)
 self._addSourceRoute(bridge)
 _ifup(bridge)
-if not bridge.ipv6.address and not bridge.ipv6.ipv6autoconf and (
-not bridge.ipv6.dhcpv6 and misc.ipv6_supported()):
-wait_for_device(bridge.name)
-sysctl.disable_ipv6(bridge.name)
 
 def configureVlan(self, vlan, **opts):
+if vlan.ipv6.requested:
+enable_ipv6(vlan.name)
 self.configApplier.addVlan(vlan, **opts)
 vlan.device.configure(**opts)
 self._addSourceRoute(vlan)
 _ifup(vlan)
 
 def configureBond(self, bond, **opts):
+if bond.ipv6.requested:
+enable_ipv6(bond.name)
 self.configApplier.addBonding(bond, **opts)
 if not vlans.is_vlanned(bond.name):
 for slave in bond.slaves:
@@ -174,6 +177,8 @@
 'nics': [slave.name for slave in bond.slaves]})
 
 def configureNic(self, nic, **opts):
+if nic.ipv6.requested:
+enable_ipv6(nic.name)
 self.configApplier.addNic(nic, **opts)
 self._addSourceRoute(nic)
 if nic.bond is None:
@@ -533,9 +538,8 @@
 if ipv4.defaultRoute is not None:
 cfg += 'DEFROUTE=%s\n' % _to_ifcfg_bool(ipv4.defaultRoute)
 cfg +=