Change in vdsm[master]: ifcfg: pass a NetDevice to _createConfFile

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

Change subject: ifcfg: pass a NetDevice to _createConfFile
......................................................................

ifcfg: pass a NetDevice to _createConfFile

If we want to handle DNS1/2 in this function, we should give
it all information stored in a NetDevice instance.

Change-Id: I67db38cb7253bff14f86019a52fef3d736a65c1f
Signed-off-by: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
---
M lib/vdsm/network/configurators/ifcfg.py
1 file changed, 20 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/71/61471/1

diff --git a/lib/vdsm/network/configurators/ifcfg.py
b/lib/vdsm/network/configurators/ifcfg.py
index e9f67e1..3487103 100644
--- a/lib/vdsm/network/configurators/ifcfg.py
+++ b/lib/vdsm/network/configurators/ifcfg.py
@@ -517,8 +517,9 @@
         if self.unifiedPersistence and utils.isOvirtNode():
             node_fs.Config().persist(fileName)
 
-    def _createConfFile(self, conf, name, ipv4, ipv6, mtu=None, **kwargs):
+    def _createConfFile(self, conf, device):
         """ Create ifcfg-* file with proper fields per device
"""
+        name, ipv4, ipv6 = device.name, device.ipv4, device.ipv6
         cfg = 'DEVICE=%s\n' % pipes.quote(name)
         cfg += conf
         if ipv4.address:
@@ -536,8 +537,8 @@
                 # Ask dhclient to stop any dhclient running for the device
                 dhclient.kill(name)
                 address.flush(name, family=4)
-        if mtu:
-            cfg += 'MTU=%d\n' % mtu
+        if device.mtu:
+            cfg += 'MTU=%d\n' % device.mtu
         if ipv4.defaultRoute is not None:
             cfg += 'DEFROUTE=%s\n' % _to_ifcfg_bool(ipv4.defaultRoute)
         cfg += 'NM_CONTROLLED=no\n'
@@ -575,8 +576,7 @@
         if 'custom' in opts and 'bridge_opts' in opts['custom']:
             conf += 'BRIDGING_OPTS="%s"\n' %
opts['custom']['bridge_opts']
 
-        self._createConfFile(conf, bridge.name, bridge.ipv4, bridge.ipv6,
-                             bridge.mtu, **opts)
+        self._createConfFile(conf, bridge)
 
     def addVlan(self, vlan, **opts):
         """ Create ifcfg-* file with proper fields for VLAN
"""
@@ -585,8 +585,7 @@
         if vlan.bridge:
             conf += 'BRIDGE=%s\n' % pipes.quote(vlan.bridge.name)
         conf += 'ONBOOT=yes\n'
-        self._createConfFile(conf, vlan.name, vlan.ipv4, vlan.ipv6, vlan.mtu,
-                             **opts)
+        self._createConfFile(conf, vlan)
 
     def addBonding(self, bond, **opts):
         """ Create ifcfg-* file with proper fields for bond
"""
@@ -598,8 +597,8 @@
             conf += 'BRIDGE=%s\n' % pipes.quote(bond.bridge.name)
         conf += 'ONBOOT=yes\n'
 
-        ipv4, ipv6, mtu = self._getIfaceConfValues(bond)
-        self._createConfFile(conf, bond.name, ipv4, ipv6, mtu, **opts)
+        bond_updated_from_ifcfg = self._update_from_ifcfg(bond)
+        self._createConfFile(conf, bond_updated_from_ifcfg)
 
         # create the bonding device to avoid initscripts noise
         with open(netinfo_bonding.BONDING_MASTERS) as info:
@@ -624,15 +623,15 @@
         if ethtool_opts:
             conf += 'ETHTOOL_OPTS=%s\n' % pipes.quote(ethtool_opts)
 
-        ipv4, ipv6, mtu = self._getIfaceConfValues(nic)
-        self._createConfFile(conf, nic.name, ipv4, ipv6, mtu, **opts)
+        nic_updated_from_ifcfg = self._update_from_ifcfg(nic)
+        self._createConfFile(conf, nic_updated_from_ifcfg)
 
     @staticmethod
-    def _getIfaceConfValues(iface):
-        ipv4 = copy.deepcopy(iface.ipv4)
-        ipv6 = copy.deepcopy(iface.ipv6)
-        mtu = iface.mtu
+    def _update_from_ifcfg(iface):
         if ifaceUsed(iface.name):
+            ipv4 = copy.deepcopy(iface.ipv4)
+            ipv6 = copy.deepcopy(iface.ipv6)
+            mtu = iface.mtu
             confParams = misc.getIfaceCfg(iface.name)
             if not ipv4.address and ipv4.bootproto != 'dhcp':
                 ipv4.address = confParams.get('IPADDR')
@@ -651,7 +650,12 @@
                 mtu = confParams.get('MTU')
                 if mtu:
                     mtu = int(mtu)
-        return ipv4, ipv6, mtu
+            iface_updated_from_ifcfg = copy.deepcopy(iface)
+            iface_updated_from_ifcfg.ipv4 = ipv4
+            iface_updated_from_ifcfg.ipv6 = ipv6
+            return iface_updated_from_ifcfg
+        else:
+            return iface
 
     def removeNic(self, nic):
         cf = NET_CONF_PREF + nic


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I67db38cb7253bff14f86019a52fef3d736a65c1f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: pass a NetDevice to _createConfFile
......................................................................


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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I67db38cb7253bff14f86019a52fef3d736a65c1f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: pass a NetDevice to _createConfFile
......................................................................


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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I67db38cb7253bff14f86019a52fef3d736a65c1f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Edward Haas has posted comments on this change.

Change subject: ifcfg: pass a NetDevice to _createConfFile
......................................................................


Patch Set 2:

(1 comment)

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

Line 627:         self._createConfFile(conf, nic_updated_from_ifcfg)
Line 628: 
Line 629:     @staticmethod
Line 630:     def _update_from_ifcfg(iface):
Line 631:         if ifaceUsed(iface.name):
Can you please explain this code, I do not understand why this is here at all.

How is addBond and addNic different from add addVlan?
Why do we read the ifcfg file if this device is a slave (of a bond or vlan)?
Assuming it was done to preserve IP settings of a base device on which vlans have been
defined, why this base device has been touched at all?

Seems like touching this opens up a lot of questions :)
Line 632:             ipv4 = copy.deepcopy(iface.ipv4)
Line 633:             ipv6 = copy.deepcopy(iface.ipv6)
Line 634:             mtu = iface.mtu
Line 635:             confParams = misc.getIfaceCfg(iface.name)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I67db38cb7253bff14f86019a52fef3d736a65c1f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: pass a NetDevice to _createConfFile
......................................................................


Patch Set 2:

(1 comment)

History.

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

Line 627:         self._createConfFile(conf, nic_updated_from_ifcfg)
Line 628: 
Line 629:     @staticmethod
Line 630:     def _update_from_ifcfg(iface):
Line 631:         if ifaceUsed(iface.name):
>  Can you please explain this code, I do not understand why this is
> here at a 
For now, I tracked it down to https://gerrit.ovirt.org/7259 and
https://bugzilla.redhat.com/846004

I will read these sources and try to answer the question: do we still need this?

It seems to me that upon adding a VLAN, IP configuration could be preserved on
NICs/bondings by other means, principally in the API, not as (quite) a hack in this one
configurator.
Line 632:             ipv4 = copy.deepcopy(iface.ipv4)
Line 633:             ipv6 = copy.deepcopy(iface.ipv6)
Line 634:             mtu = iface.mtu
Line 635:             confParams = misc.getIfaceCfg(iface.name)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I67db38cb7253bff14f86019a52fef3d736a65c1f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

gerrit-hooks has posted comments on this change.

Change subject: ifcfg: pass a NetDevice to _createConfFile
......................................................................


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-4.0'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I67db38cb7253bff14f86019a52fef3d736a65c1f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Ondřej Svoboda has posted comments on this change.

Change subject: ifcfg: pass a NetDevice to _createConfFile
......................................................................


Patch Set 3: Verified+1

The whole topic (at this time, 4 patches) successfully preserves DNS servers statically
defined on a NIC. First, I created a bridgeless network, then I converted it to bridged
and finally, I rebooted. During all the steps nameservers stayed unchanged even with
NetworkManager running.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I67db38cb7253bff14f86019a52fef3d736a65c1f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No