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

ibarkan at redhat.com ibarkan at redhat.com
Mon Dec 14 11:30:01 UTC 2015


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 <edwardh at redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkaplan at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list