Change in vdsm[master]: net: Correctly apply MTU values on networks

2015-12-16 Thread danken 
Dan Kenigsberg has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-16 Thread danken 
Dan Kenigsberg has submitted this change and it was merged.

Change subject: net: Correctly apply MTU values on networks
..


net: Correctly apply MTU values on networks

Two issues have been resolved by this change:

- New networks with no MTU specification are being
set by default with their connected device (bond,
vlan, nic) mtu (which does not have to be 1500).
Fixed by detecting when no MTU is specified in the
configuration, and adding the default (1500)
explicitly.
The assumption of a single default mtu when one is
not specified in the setup is wrong, causing in
some cases an unnecessary restoration of networks
during network restoration.
- Test fix: The NIC/s mtu should be set to the
maximum mtu of the remaining networks.

Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Signed-off-by: Edward Haas 
Reviewed-on: https://gerrit.ovirt.org/50397
Continuous-Integration: Jenkins CI
Reviewed-by: Dan Kenigsberg 
---
M lib/vdsm/kernelconfig.py
M lib/vdsm/network/api.py
M tests/configNetworkTests.py
M tests/functional/networkTests.py
4 files changed, 83 insertions(+), 30 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Dan Kenigsberg: Looks good to me, approved
  Edward Haas: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-16 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 6:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-16 Thread edwardh 
Edward Haas has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 4: -Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-16 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


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/50397
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-16 Thread edwardh 
Edward Haas has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 5: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-16 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


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/50397
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-16 Thread edwardh 
Edward Haas has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 3:

(2 comments)

https://gerrit.ovirt.org/#/c/50397/3/lib/vdsm/network/api.py
File lib/vdsm/network/api.py:

Line 902: _complement_net_defaults
> in a future patch - rename to _canonize_networks() as you plan to do str->i
Done


Line 975: att
> att and attr represent the same thing, and as such, should have the same na
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-16 Thread edwardh 
Edward Haas has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-15 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


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/50397
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-15 Thread edwardh 
Edward Haas has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 1:

(2 comments)

https://gerrit.ovirt.org/#/c/50397/1//COMMIT_MSG
Commit Message:

Line 8: 
> IMO you should explain that because of the default behaviour of vdsm- letti
Done


Line 14: us
> is
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-15 Thread edwardh 
Edward Haas has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 1:

(10 comments)

https://gerrit.ovirt.org/#/c/50397/1/lib/vdsm/network/api.py
File lib/vdsm/network/api.py:

Line 268: if mtu:
: mtu = int(mtu)
> this transformation is not needed anymore
Done


Line 323: if mtu:
> This also need to be reconsidered now. We should update the bridges and the
Added a remark (TODO): The MTU should be updated only if different from current 
value.


Line 899: _compliment_net_defaults(networks)
> after this call- I think kernelconfig._normalize_mtu is not needed anymore,
Found to be needed after all for an edge case:
Normalization happens even on "remove" networks, where the defaults are not 
explicitly added (in our case, the mtu).
I will open a new patch for removing support of "remove" networks from the 
normalization.


Line 967: compliment
> complement
Done


Line 969: a network
> networks
Done


Line 972: net
> it is idiomatic to call unneeded variables '_'. Moreover, if you don't need
Done


Line 972: _nets_attr
> the convention is to not prefix method variables with '_'.
Done


Line 973:   if 'remove' not in att or att['remove'] is not True)
Line 974: for attr in _nets_attr:
Line 975: if 'mtu' not in attr:
Line 976: attr['mtu'] = DEFAULT_MTU
Line 977: 
> I prefer a functional approach:
Making it immutable here looks strange to me, at least at this stage.
Line 978: 
Line 979: def setSafeNetworkConfig():
Line 980: """Declare current network configuration as 'safe'"""
Line 981: utils.execCmd([constants.EXT_VDSM_STORE_NET_CONFIG,


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

Line 43: from vdsm.netinfo.mtus
> we try to keep import statements sorted alphabetically (it make sit easier 
Done


Line 1045: # Add additional nic to the bond
 : # Expect to see nics MTU to be DEFAULT.
 : status, msg = self.setupNetworks(
 : {}, {BONDING_NAME: dict(nics=nics)}, NOCHK)
 : 
 : self.assertEquals(status, SUCCESS, msg)
 : 
 : self.assertMtu(DEFAULT_MTU, BONDING_NAME,
 :nics[0], nics[1], nics[2])
> this part should come at a stage when the expected MTU of the bond+slaves i
Existing test is valid by itself, I'll add an additional test to cover the 
scenario you mentioned.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-15 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


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/50397
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-15 Thread edwardh 
Edward Haas has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-15 Thread Jenkins CI 
Jenkins CI has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 3: Continuous-Integration+1

Propagate review hook: Continuous Integration value inherited from patch 2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-15 Thread ibarkan 
Ido Barkan has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-15 Thread danken 
Dan Kenigsberg has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 3: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/50397/3/lib/vdsm/network/api.py
File lib/vdsm/network/api.py:

Line 902: _complement_net_defaults
in a future patch - rename to _canonize_networks() as you plan to do str->int 
translation there.


Line 975: att
att and attr represent the same thing, and as such, should have the same name. 
The name should better be 'attrs' as it is multiple attributes of the network.

btw, I find the following simpler:

for attrs in six.itervalues(nets):
if attrs.get('remove'):
continue

if 'mtu' not in attrs:
attr['mtu'] = DEFAULT_MTU

your call.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-14 Thread edwardh 
Edward Haas has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 1:

Eliminating the default MTU from engine:
https://gerrit.ovirt.org/#/c/28097/

Therefore setupNetwork will always have the network MTU set when coming from 
Engine.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-14 Thread edwardh 
Edward Haas has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 1:

IMO, having independent guards on both Engine and VDSM is preferred.
Either handle defaults at the VDSM side (like in the current patch) or to 
validate the network command and assure MTU exist, otherwise to fail the setup.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-14 Thread ibarkan 
Ido Barkan has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 1:

(12 comments)

-1 for code, +1 for this approach.
We cannot change the API. If we currently allow MTU to be None we should keep 
allowing it. However, from the very beginning of the setupNetworks API call, 
the code is a lot easier to handle if it can assume as many predicates on the 
input as possible. This solution adds the first one and this is good.

https://gerrit.ovirt.org/#/c/50397/1//COMMIT_MSG
Commit Message:

Line 8: 
IMO you should explain that because of the default behaviour of vdsm- letting 
the OS to determine the default MTU for created devices causes it to not keep 
the supremum MTU needed. Moreover, expecting a default MTU and discovering a 
non-default MTU  during networks restoration will cause unnecessary restoration 
of networks.


Line 14: us
is


https://gerrit.ovirt.org/#/c/50397/1/lib/vdsm/network/api.py
File lib/vdsm/network/api.py:

Line 268: if mtu:
: mtu = int(mtu)
this transformation is not needed anymore


Line 323: if mtu:
This also need to be reconsidered now. We should update the bridges and the 
existing tap devices if their current MTU is different then the requested one.


Line 899: _compliment_net_defaults(networks)
after this call- I think kernelconfig._normalize_mtu is not needed anymore, as 
we will always persist some mtu.


Line 967: compliment
complement


Line 969: a network
networks


Line 972: net
it is idiomatic to call unneeded variables '_'. Moreover, if you don't need the 
keys, call
  six.itervalues(nets)


Line 972: _nets_attr
the convention is to not prefix method variables with '_'.


Line 973:   if 'remove' not in att or att['remove'] is not True)
Line 974: for attr in _nets_attr:
Line 975: if 'mtu' not in attr:
Line 976: attr['mtu'] = DEFAULT_MTU
Line 977: 
I prefer a functional approach:
  netowrks = _compliment_net_defaults(networks)
treating parameters as immutable. This makes it easy to test later.
Line 978: 
Line 979: def setSafeNetworkConfig():
Line 980: """Declare current network configuration as 'safe'"""
Line 981: utils.execCmd([constants.EXT_VDSM_STORE_NET_CONFIG,


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

Line 43: from vdsm.netinfo.mtus
we try to keep import statements sorted alphabetically (it make sit easier to 
find them later)


Line 1045: # Add additional nic to the bond
 : # Expect to see nics MTU to be DEFAULT.
 : status, msg = self.setupNetworks(
 : {}, {BONDING_NAME: dict(nics=nics)}, NOCHK)
 : 
 : self.assertEquals(status, SUCCESS, msg)
 : 
 : self.assertMtu(DEFAULT_MTU, BONDING_NAME,
 :nics[0], nics[1], nics[2])
this part should come at a stage when the expected MTU of the bond+slaves is 
different than the default (earlier).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-14 Thread ibarkan 
Ido Barkan has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 1: Code-Review-1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Alona Kaplan 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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]: net: Correctly apply MTU values on networks

2015-12-13 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


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/50397
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
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]: net: Correctly apply MTU values on networks

2015-12-13 Thread edwardh 
Edward Haas has uploaded a new change for review.

Change subject: net: Correctly apply MTU values on networks
..

net: Correctly apply MTU values on networks

Two issues have been resolved by this change:

- New networks with no MTU specification are being
set by default with their connected device (bond,
vlan, nic) mtu (which does not have to be 1500).
Fixed by detecting when no MTU us specified in the
configuration, and adding the default (1500)
explicitly.

- Test fix: The NIC/s mtu should be set to the
maximum mtu of the remaining networks.

Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Signed-off-by: Edward Haas 
---
M lib/vdsm/network/api.py
M tests/functional/networkTests.py
2 files changed, 28 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/97/50397/1

diff --git a/lib/vdsm/network/api.py b/lib/vdsm/network/api.py
index c3d82d4..1d4ea09 100755
--- a/lib/vdsm/network/api.py
+++ b/lib/vdsm/network/api.py
@@ -37,6 +37,7 @@
   get as netinfo_get, CachingNetInfo,
   networks as netinfo_networks, nics as netinfo_nics,
   NET_PATH)
+from vdsm.netinfo.mtus import DEFAULT_MTU
 from vdsm import udevadm
 from vdsm import utils
 from vdsm import ipwrapper
@@ -895,6 +896,8 @@
  "networks:%r, bondings:%r, options:%r" % (networks,
bondings, options))
 
+_compliment_net_defaults(networks)
+
 logging.debug("Validating configuration")
 _validateNetworkSetup(networks, bondings)
 
@@ -961,6 +964,18 @@
 return vlan
 
 
+def _compliment_net_defaults(nets):
+"""
+Given a network configuration, explicitly add missing defaults.
+:param nets: The network configuration
+"""
+_nets_attr = (att for net, att in nets.iteritems()
+  if 'remove' not in att or att['remove'] is not True)
+for attr in _nets_attr:
+if 'mtu' not in attr:
+attr['mtu'] = DEFAULT_MTU
+
+
 def setSafeNetworkConfig():
 """Declare current network configuration as 'safe'"""
 utils.execCmd([constants.EXT_VDSM_STORE_NET_CONFIG,
diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py
index 5e23172..aa25b50 100644
--- a/tests/functional/networkTests.py
+++ b/tests/functional/networkTests.py
@@ -40,6 +40,7 @@
 from vdsm.netinfo.dhcp import get_dhclient_ifaces
 from vdsm.netinfo.nics import operstate, OPERSTATE_UNKNOWN, OPERSTATE_UP
 from vdsm.netinfo.routes import getRouteDeviceTo
+from vdsm.netinfo.mtus import DEFAULT_MTU
 from vdsm.netlink import monitor
 from vdsm.network.configurators.ifcfg import (Ifcfg, stop_devices,
   NET_CONF_BACK_DIR)
@@ -987,14 +988,13 @@
 
 @cleanupNet
 @permutations([[True], [False]])
-@brokentest("This test fails because the 2 different networks are getting"
-" configured with the same MTU. The test should assert that "
-"the reported MTUs are equal to the requested ones.")
 def testSetupNetworksMtus(self, bridged):
 JUMBO = '9000'
 MIDI = '4000'
 
 with dummyIf(3) as nics:
+# Add two networks, one with default MTU and the other with MIDI.
+# Expect to see nics MTU to be MIDI. (MIDI > DEFAULT MTU)
 networks = {NETWORK_NAME + '1':
 dict(bonding=BONDING_NAME, bridged=bridged,
  vlan='100'),
@@ -1010,6 +1010,8 @@
 self.assertMtu(MIDI, NETWORK_NAME + '2', BONDING_NAME, nics[0],
nics[1])
 
+# Add a 3rd network, with JUMBO MTU.
+# Expect to see nics MTU to be JUMBO. (JUMBO > MIDI)
 network = {NETWORK_NAME + '3':
dict(bonding=BONDING_NAME, vlan='300', mtu=JUMBO,
 bridged=bridged)}
@@ -1021,6 +1023,8 @@
 self.assertMtu(JUMBO, NETWORK_NAME + '3', BONDING_NAME, nics[0],
nics[1])
 
+# Remove the 3rd network (with JUMBO MTU)
+# Expect to see nics MTU to be MIDI.
 status, msg = self.setupNetworks(
 {NETWORK_NAME + '3': dict(remove=True)}, {}, NOCHK)
 
@@ -1029,21 +1033,24 @@
 self.assertMtu(MIDI, NETWORK_NAME + '2', BONDING_NAME, nics[0],
nics[1])
 
-# Keep last custom MTU on the interfaces
+# Remove the 2nd network (with MIDI MTU)
+# Expect to see nics MTU to be DEFAULT.
 status, msg = self.setupNetworks(
 {NETWORK_NAME + '2': dict(remove=True)}, {}, NOCHK)
 
 self.assertEquals(status, SUCCESS, msg)
 
-self.assertMtu(MIDI, BONDING_NAME, nics[0], nics[1])
+self.assertMtu(DEFAULT_MTU, BONDING_NAME,

Change in vdsm[master]: net: Correctly apply MTU values on networks

2015-12-13 Thread edwardh 
Edward Haas has posted comments on this change.

Change subject: net: Correctly apply MTU values on networks
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba24363f5b9ea70392a68885cf5374800ab58549
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Ido Barkan 
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