From: Ondrej Lichtner olichtne@redhat.com
The 'interface-name' value in device specific dictionaries was moved into the general 'connection' dict. The old attributes are still supported by NM (for compatibility) but since we don't use older NM versions anymore I'm moving these to their proper place.
Signed-off-by: Ondrej Lichtner olichtne@redhat.com --- lnst/Slave/NmConfigDevice.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/lnst/Slave/NmConfigDevice.py b/lnst/Slave/NmConfigDevice.py index 8f9170e..29c2c75 100644 --- a/lnst/Slave/NmConfigDevice.py +++ b/lnst/Slave/NmConfigDevice.py @@ -494,6 +494,7 @@ class NmConfigDeviceBond(NmConfigDeviceGeneric):
s_bond_con = dbus.Dictionary({ 'type': 'bond', + 'interface-name': config["name"], 'autoconnect': dbus.Boolean(False), 'uuid': config["master_uuid"], 'id': config["name"]+"_con"}) @@ -502,12 +503,9 @@ class NmConfigDeviceBond(NmConfigDeviceGeneric):
if options: s_bond = dbus.Dictionary({ - 'interface-name': config["name"], 'options': options}) else: - s_bond = dbus.Dictionary({ - 'interface-name': config["name"]}) - + s_bond = dbus.Dictionary({})
connection = dbus.Dictionary({ 'bond': s_bond, @@ -591,11 +589,11 @@ class NmConfigDeviceBridge(NmConfigDeviceGeneric): s_bridge_con = dbus.Dictionary({ 'type': 'bridge', 'autoconnect': dbus.Boolean(False), + 'interface-name': config["name"], 'uuid': config["master_uuid"], 'id': config["name"]+"_con"})
s_bridge = dbus.Dictionary({ - 'interface-name': config["name"], 'stp': dbus.Boolean(False)})
connection = dbus.Dictionary({ @@ -715,11 +713,11 @@ class NmConfigDeviceVlan(NmConfigDeviceGeneric): s_vlan_con = dbus.Dictionary({ 'type': 'vlan', 'autoconnect': dbus.Boolean(False), + 'interface-name': dev_name, 'uuid': str(uuid.uuid4()), 'id': dev_name+"_con"})
s_vlan = dbus.Dictionary({ - 'interface-name': dev_name, 'parent': realdev, 'id': dbus.UInt32(vlan_tci)}, signature="sv")
@@ -796,13 +794,13 @@ class NmConfigDeviceTeam(NmConfigDeviceGeneric): s_team_con = dbus.Dictionary({ 'type': 'team', 'autoconnect': dbus.Boolean(False), + 'interface-name': config["name"], 'uuid': config["master_uuid"], 'id': config["name"]+"_con"})
teamd_config = get_option(config, "teamd_config")
s_team = dbus.Dictionary({ - 'interface-name': config["name"], 'config': teamd_config})
connection = dbus.Dictionary({
From: Ondrej Lichtner olichtne@redhat.com
Due to how the Ctl-Slave communication currently works, returning None from a Slave Method is not possible - the main Slave loop doesn't resend this to the Ctl, resulting in the Ctl waiting forever. This is due to how background and foreground commands work - background command immediately returns "passed" (meaning started) whereas a forground command at first returns None, the command then runs in a forked process and then returns the command results - meanwhile the Ctl is waiting because it didn't get the "return None" message.
So returning None is fine as long as a different result message is generated and returned later, currently that only happens in the run_command method and the others are now fixed by this commit.
Signed-off-by: Ondrej Lichtner olichtne@redhat.com --- lnst/Slave/NetTestSlave.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lnst/Slave/NetTestSlave.py b/lnst/Slave/NetTestSlave.py index da1e2f4..f6976db 100644 --- a/lnst/Slave/NetTestSlave.py +++ b/lnst/Slave/NetTestSlave.py @@ -147,7 +147,7 @@ class SlaveMethods: if device: return device.get_if_data() else: - return None + return {}
def get_devices_by_devname(self, devname): name_scan = self._if_manager.get_devices() @@ -190,14 +190,14 @@ class SlaveMethods: def get_if_data(self, if_id): dev = self._if_manager.get_mapped_device(if_id) if dev is None: - return None + return {} return dev.get_if_data()
def link_stats(self, if_id): dev = self._if_manager.get_mapped_device(if_id) if dev is None: logging.error("Device with id '%s' not found." % if_id) - return None + return {} return dev.link_stats()
def set_addresses(self, if_id, ips): @@ -670,7 +670,7 @@ class SlaveMethods: self._net_namespaces[netns] = {"pid": pid, "pipe": read_pipe} self._server_handler.add_netns(netns, read_pipe) - return None + return False elif pid == 0: self._slave_server.set_netns_sighandlers() #create new network namespace
From: Ondrej Lichtner olichtne@redhat.com
Sometimes when configuration of the interface failed during it's creation (and the device wasn't created, e.g. missing team{d, ctl}). The slave would register this as a "configured" Device (because it had a {Net, Nm}ConfigDevice object created, and would attempt to deconfigure it, this return an exception again and either crash the slave or leave it in an inconsistent state (trying to deconfigure the device again and again).
This makes sure that deconfiguration is skipped in case LNST didn't receive a netlink notification that the device was created and registered by the kernel.
Signed-off-by: Ondrej Lichtner olichtne@redhat.com --- lnst/Slave/InterfaceManager.py | 13 +++++++------ lnst/Slave/NetTestSlave.py | 5 +++-- 2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/lnst/Slave/InterfaceManager.py b/lnst/Slave/InterfaceManager.py index 949817c..dc390a9 100644 --- a/lnst/Slave/InterfaceManager.py +++ b/lnst/Slave/InterfaceManager.py @@ -566,12 +566,13 @@ class Device(object): m_dev.clear_configuration()
if self._conf != None: - self.address_cleanup() - self.down() - self.deconfigure() - self._clear_tc_qdisc() - self._clear_tc_filters() - self.destroy() + if self._if_index is not None: + self.address_cleanup() + self.down() + self.deconfigure() + self._clear_tc_qdisc() + self._clear_tc_filters() + self.destroy() self._conf = None self._conf_dict = None
diff --git a/lnst/Slave/NetTestSlave.py b/lnst/Slave/NetTestSlave.py index f6976db..2d0b361 100644 --- a/lnst/Slave/NetTestSlave.py +++ b/lnst/Slave/NetTestSlave.py @@ -383,7 +383,7 @@ class SlaveMethods:
def deconfigure_interface(self, if_id): device = self._if_manager.get_mapped_device(if_id) - if device is not None: + if device is not None and device.get_if_index() is not None: device.clear_configuration() else: logging.error("No device with id '%s' to deconfigure." % if_id) @@ -533,7 +533,8 @@ class SlaveMethods: for if_id, dev in devs.iteritems(): peer = dev.get_peer() if peer == None: - dev.clear_configuration() + if dev.get_if_index() is not None: + dev.clear_configuration() else: peer_if_index = peer.get_if_index() peer_id = self._if_manager.get_id_by_if_index(peer_if_index)
Thu, Sep 22, 2016 at 10:25:24PM CEST, olichtne@redhat.com wrote:
From: Ondrej Lichtner olichtne@redhat.com
Sometimes when configuration of the interface failed during it's creation (and the device wasn't created, e.g. missing team{d, ctl}). The slave would register this as a "configured" Device (because it had a {Net, Nm}ConfigDevice object created, and would attempt to deconfigure it, this return an exception again and either crash the slave or leave it in an inconsistent state (trying to deconfigure the device again and again).
This makes sure that deconfiguration is skipped in case LNST didn't receive a netlink notification that the device was created and registered by the kernel.
Signed-off-by: Ondrej Lichtner olichtne@redhat.com
lnst/Slave/InterfaceManager.py | 13 +++++++------ lnst/Slave/NetTestSlave.py | 5 +++-- 2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/lnst/Slave/InterfaceManager.py b/lnst/Slave/InterfaceManager.py index 949817c..dc390a9 100644 --- a/lnst/Slave/InterfaceManager.py +++ b/lnst/Slave/InterfaceManager.py @@ -566,12 +566,13 @@ class Device(object): m_dev.clear_configuration()
if self._conf != None:
self.address_cleanup()
self.down()
self.deconfigure()
self._clear_tc_qdisc()
self._clear_tc_filters()
self.destroy()
if self._if_index is not None:
self.address_cleanup()
self.down()
self.deconfigure()
self._clear_tc_qdisc()
self._clear_tc_filters()
self.destroy() self._conf = None self._conf_dict = None
diff --git a/lnst/Slave/NetTestSlave.py b/lnst/Slave/NetTestSlave.py index f6976db..2d0b361 100644 --- a/lnst/Slave/NetTestSlave.py +++ b/lnst/Slave/NetTestSlave.py @@ -383,7 +383,7 @@ class SlaveMethods:
def deconfigure_interface(self, if_id): device = self._if_manager.get_mapped_device(if_id)
if device is not None:
if device is not None and device.get_if_index() is not None:
^^^^ Is this check necessary? You already do the check in the hunk above.
device.clear_configuration() else: logging.error("No device with id '%s' to deconfigure." % if_id)
@@ -533,7 +533,8 @@ class SlaveMethods: for if_id, dev in devs.iteritems(): peer = dev.get_peer() if peer == None:
dev.clear_configuration()
if dev.get_if_index() is not None:
dev.clear_configuration() else: peer_if_index = peer.get_if_index() peer_id = self._if_manager.get_id_by_if_index(peer_if_index)
-- 2.10.0 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org
On Fri, Sep 23, 2016 at 09:17:13AM +0200, Jan Tluka wrote:
Thu, Sep 22, 2016 at 10:25:24PM CEST, olichtne@redhat.com wrote:
From: Ondrej Lichtner olichtne@redhat.com
Sometimes when configuration of the interface failed during it's creation (and the device wasn't created, e.g. missing team{d, ctl}). The slave would register this as a "configured" Device (because it had a {Net, Nm}ConfigDevice object created, and would attempt to deconfigure it, this return an exception again and either crash the slave or leave it in an inconsistent state (trying to deconfigure the device again and again).
This makes sure that deconfiguration is skipped in case LNST didn't receive a netlink notification that the device was created and registered by the kernel.
Signed-off-by: Ondrej Lichtner olichtne@redhat.com
lnst/Slave/InterfaceManager.py | 13 +++++++------ lnst/Slave/NetTestSlave.py | 5 +++-- 2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/lnst/Slave/InterfaceManager.py b/lnst/Slave/InterfaceManager.py index 949817c..dc390a9 100644 --- a/lnst/Slave/InterfaceManager.py +++ b/lnst/Slave/InterfaceManager.py @@ -566,12 +566,13 @@ class Device(object): m_dev.clear_configuration()
if self._conf != None:
self.address_cleanup()
self.down()
self.deconfigure()
self._clear_tc_qdisc()
self._clear_tc_filters()
self.destroy()
if self._if_index is not None:
self.address_cleanup()
self.down()
self.deconfigure()
self._clear_tc_qdisc()
self._clear_tc_filters()
self.destroy() self._conf = None self._conf_dict = None
diff --git a/lnst/Slave/NetTestSlave.py b/lnst/Slave/NetTestSlave.py index f6976db..2d0b361 100644 --- a/lnst/Slave/NetTestSlave.py +++ b/lnst/Slave/NetTestSlave.py @@ -383,7 +383,7 @@ class SlaveMethods:
def deconfigure_interface(self, if_id): device = self._if_manager.get_mapped_device(if_id)
if device is not None:
if device is not None and device.get_if_index() is not None:
^^^^ Is this check necessary? You
already do the check in the hunk above.
You're right, it's not necessary, and I also just noticed that the Device class has an attribute _configured that is set to True if configuration succeeds so it can probably be used more reliably than waiting for a netlink notification (wrt. race conditions).
The _configured flag is already used by the deconfigure() method, the problems were caused by the other methods, so I'll send a v2 of the patch that extends the use of the flag to all the other methods.
Thanks for catching this.
-Ondrej
On Thu, Sep 22, 2016 at 10:25:22PM +0200, olichtne@redhat.com wrote:
From: Ondrej Lichtner olichtne@redhat.com
The 'interface-name' value in device specific dictionaries was moved into the general 'connection' dict. The old attributes are still supported by NM (for compatibility) but since we don't use older NM versions anymore I'm moving these to their proper place.
Signed-off-by: Ondrej Lichtner olichtne@redhat.com
lnst/Slave/NmConfigDevice.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/lnst/Slave/NmConfigDevice.py b/lnst/Slave/NmConfigDevice.py index 8f9170e..29c2c75 100644 --- a/lnst/Slave/NmConfigDevice.py +++ b/lnst/Slave/NmConfigDevice.py @@ -494,6 +494,7 @@ class NmConfigDeviceBond(NmConfigDeviceGeneric):
s_bond_con = dbus.Dictionary({ 'type': 'bond',
'interface-name': config["name"], 'autoconnect': dbus.Boolean(False), 'uuid': config["master_uuid"], 'id': config["name"]+"_con"})
@@ -502,12 +503,9 @@ class NmConfigDeviceBond(NmConfigDeviceGeneric):
if options: s_bond = dbus.Dictionary({
'interface-name': config["name"], 'options': options}) else:
s_bond = dbus.Dictionary({
'interface-name': config["name"]})
s_bond = dbus.Dictionary({}) connection = dbus.Dictionary({ 'bond': s_bond,
@@ -591,11 +589,11 @@ class NmConfigDeviceBridge(NmConfigDeviceGeneric): s_bridge_con = dbus.Dictionary({ 'type': 'bridge', 'autoconnect': dbus.Boolean(False),
'interface-name': config["name"], 'uuid': config["master_uuid"], 'id': config["name"]+"_con"}) s_bridge = dbus.Dictionary({
'interface-name': config["name"], 'stp': dbus.Boolean(False)}) connection = dbus.Dictionary({
@@ -715,11 +713,11 @@ class NmConfigDeviceVlan(NmConfigDeviceGeneric): s_vlan_con = dbus.Dictionary({ 'type': 'vlan', 'autoconnect': dbus.Boolean(False),
'interface-name': dev_name, 'uuid': str(uuid.uuid4()), 'id': dev_name+"_con"}) s_vlan = dbus.Dictionary({
'interface-name': dev_name, 'parent': realdev, 'id': dbus.UInt32(vlan_tci)}, signature="sv")
@@ -796,13 +794,13 @@ class NmConfigDeviceTeam(NmConfigDeviceGeneric): s_team_con = dbus.Dictionary({ 'type': 'team', 'autoconnect': dbus.Boolean(False),
'interface-name': config["name"], 'uuid': config["master_uuid"], 'id': config["name"]+"_con"}) teamd_config = get_option(config, "teamd_config") s_team = dbus.Dictionary({
'interface-name': config["name"], 'config': teamd_config}) connection = dbus.Dictionary({
-- 2.10.0
scratch patchset, I sent a v2 that adds a new patch and fixes 3/3 based on suggestion from jtluka
-Ondrej
lnst-developers@lists.fedorahosted.org