Change in vdsm[master]: netconf: enable multiple gateways for iproute2 configurator

Mark Wu has uploaded a new change for review.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

netconf: enable multiple gateways for iproute2 configurator

Change-Id: I76e1225caffdb2de3073041e541c7c978eefb396
Signed-off-by: Mark Wu 
---
M vdsm/netconf/iproute2.py
1 file changed, 11 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/76/16276/1

diff --git a/vdsm/netconf/iproute2.py b/vdsm/netconf/iproute2.py
index d474e09..795dd9b 100644
--- a/vdsm/netconf/iproute2.py
+++ b/vdsm/netconf/iproute2.py
@@ -45,6 +45,8 @@
             utils.execCmd(['brctl', 'addif', bridge.name, bridge.port.name])
         self.configApplier.setIpConfig(bridge)
         self.configApplier.setIfaceMtu(bridge.name, bridge.mtu)
+        ipaddr, netmask, gateway, bootproto, _ = bridge.getIpConfig()
+        self._addSourceRoute(bridge, ipaddr, netmask, gateway, bootproto)
         self.configApplier.ifup(bridge)

def configureVlan(self, vlan, bridge=None, **opts):
@@ -53,6 +55,8 @@
         if vlan.ip:
             self.configApplier.setIpConfig(vlan)
         self.configApplier.setIfaceMtu(vlan.name, vlan.mtu)
+        ipaddr, netmask, gateway, bootproto, _ = vlan.getIpConfig()
+        self._addSourceRoute(vlan, ipaddr, netmask, gateway, bootproto)
         self.configApplier.ifup(vlan)

def configureBond(self, bond, bridge=None, **opts):
@@ -76,6 +80,8 @@
         if bond.ip:
             self.configApplier.setIpConfig(bond)
         self.configApplier.setIfaceMtu(bond.name, bond.mtu)
+        ipaddr, netmask, gateway, bootproto, _ = bond.getIpConfig()
+        self._addSourceRoute(bond, ipaddr, netmask, gateway, bootproto)
         self.configApplier.ifup(bond)

def editBonding(self, bond, _netinfo):
@@ -91,22 +97,27 @@
         if nic.ip:
             self.configApplier.setIpConfig(nic)
         self.configApplier.setIfaceMtu(nic.name, nic.mtu)
+        ipaddr, netmask, gateway, bootproto, _ = nic.getIpConfig()
+        self._addSourceRoute(nic, ipaddr, netmask, gateway, bootproto)
         self.configApplier.ifup(nic)

def removeBridge(self, bridge):
         self.configApplier.ifdown(bridge)
+        self._removeSourceRoute(bridge)
         utils.execCmd([constants.EXT_BRCTL, 'delbr', bridge.name])
         if bridge.port:
             bridge.port.remove()

def removeVlan(self, vlan):
         self.configApplier.ifdown(vlan)
+        self._removeSourceRoute(vlan)
         ipwrapper.linkDel(['dev', vlan.name])
         vlan.device.remove()

def _ifaceDownAndCleanup(self, iface, _netinfo):
         """Returns True iff the iface is to be removed."""
         self.configApplier.ifdown(iface)
+        self._removeSourceRoute(iface)
         if iface.master is None:
             self.configApplier.removeIpConfig(iface)
         return not _netinfo.ifaceUsers(iface.name)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3014/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3092/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2203/ : SUCCESS

Antoni Segura Puimedon has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 2: Fails

Build Failed

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3071/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3150/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2258/ : FAILURE

Antoni Segura Puimedon has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

Thanks ;-)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 3: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3701/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3617/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2810/ : FAILURE

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 4:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3972/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3889/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3083/ : SUCCESS

Antoni Segura Puimedon has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 4:

(2 comments)

....................................................
File vdsm/netconf/iproute2.py
Line 87: 
Line 88:     def _ifaceDownAndCleanup(self, iface, _netinfo):
Line 89:         """Returns True iff the iface is to be removed."""
Line 90:         self.configApplier.ifdown(iface)
Line 91:         self._removeSourceRoute(iface)
@Assaf: Does it impact the functionality if this is done after the ifdown?
Line 92:         if iface.master is None:
Line 93:             self.configApplier.removeIpConfig(iface)
Line 94:         return not _netinfo.ifaceUsers(iface.name)
Line 95:

....................................................
File vdsm/sourceRoute.py
Line 150:         # its source
Line 151:         rules += [rule for rule in allRules if rule.source == network]
Line 152: 
Line 153:         return rules
Line 154: 
Since now this behavior is in the static part, probaby it should special case for configurator behavior to maintain what the ifcfg part doesn't need to do.
Line 155:     def remove(self):
Line 156:         logging.info("Removing gateway - device: %s" % self.device)
Line 157: 
Line 158:         rules = self._getRules(self.device)

Mark Wu has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 4: Code-Review-1

(1 comment)

it misses the interface tracking when removing interface. Then the the dynamic source route can't be removed.

....................................................
File vdsm/netconf/iproute2.py
Line 87: 
Line 88:     def _ifaceDownAndCleanup(self, iface, _netinfo):
Line 89:         """Returns True iff the iface is to be removed."""
Line 90:         self.configApplier.ifdown(iface)
Line 91:         self._removeSourceRoute(iface)
I think it has the same flow as dynamic source route with ifcfg.
Line 92:         if iface.master is None:
Line 93:             self.configApplier.removeIpConfig(iface)
Line 94:         return not _netinfo.ifaceUsers(iface.name)
Line 95:

Mark Wu has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 4:

(1 comment)

....................................................
File vdsm/sourceRoute.py
Line 150:         # its source
Line 151:         rules += [rule for rule in allRules if rule.source == network]
Line 152: 
Line 153:         return rules
Line 154: 
Yes, it makes sense.  I will fix it in next patch. Thanks!
Line 155:     def remove(self):
Line 156:         logging.info("Removing gateway - device: %s" % self.device)
Line 157: 
Line 158:         rules = self._getRules(self.device)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 5:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4011/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3122/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3929/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 6:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4068/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3173/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3989/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 7:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4171/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3276/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4092/ : SUCCESS

Itamar Heim has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 7:

ping?

Itamar Heim has abandoned this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Abandoned

no comment for 30 days post last ping. abandoning. please re-open if/when relevant and/or ping reviewers directly / promote discussion about the patch in the mailing list.

[Dan Kenigsberg]

Dan Kenigsberg has restored this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Restored

We're slow, but it's needed.

Assaf Muller has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 7: Code-Review-1

A few updates:
* Once we rebase this patch we can theoretically remove the To-Do item, as netinfo.getBootProtocol has since been enhanced to support both the ifcfg and unified persistence models
* It's clear from the changes made in this patch that the current design for the SourceRoute classes doesn't *actually* support multiple configurators... Passing a 'byIproute2' flag doesn't count :) A new design needs to be thought up that properly supports multi-configurators
* There's verification work to be done

Assaf Muller has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 7:

(1 comment)

....................................................
File vdsm/netconf/iproute2.py
Line 153:         if ipConfig.ipaddr:
Line 154:             try:
Line 155:                 self.removeIpConfig(iface)
Line 156:                 ipwrapper.addrAdd(iface.name, ipConfig.ipaddr,
Line 157:                                   ipConfig.netmask)
Need to test if 'dev' is a required parameter in this context.
Line 158:                 if ipConfig.gateway and ipConfig.defaultRoute:
Line 159:                     ipwrapper.routeAdd(['default', 'via', ipConfig.gateway])
Line 160:             except ipwrapper.IPRoute2Error as e:
Line 161:                 if 'File exists' in e.message[0]:

Itamar Heim has posted comments on this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Patch Set 7:

ping

Itamar Heim has abandoned this change.

Change subject: netconf: enable multiple gateways for iproute2 configurator
......................................................................

Abandoned

no reply - abandoning - please restore if still relevant