LinuxInterfaceDriver plug method in derived class already check for device existence

Bug #1348703 reported by Rossella Sblendido
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
neutron
Fix Released
Low
Sergey Belous

Bug Description

LinuxInterfaceDriver plug in derived classes already check if the device exists. There's no need to duplicate the check in the code that is calling those methods. See l3_agent.py in internal_network_added for example:

        if not ip_lib.device_exists(interface_name,
                                    root_helper=self.root_helper,
                                    namespace=ri.ns_name):
            self.driver.plug(network_id, port_id, interface_name, mac_address,
                             namespace=ri.ns_name,
                             prefix=INTERNAL_DEV_PREFIX)

the check "if not ip_lib.device_exists" is a duplicate and it's expensive.

Changed in neutron:
status: New → Confirmed
tags: added: low-hanging-fruit
Veena (mveenasl)
Changed in neutron:
assignee: nobody → veena (mveenasl)
Akihiro Motoki (amotoki)
Changed in neutron:
importance: Undecided → Low
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

Fix proposed to branch: master
Review: https://review.openstack.org/113897

Changed in neutron:
status: Confirmed → In Progress
summary: - LinuxInterfaceDriver plug and unplug methods in derived class already
- check for device existence
+ LinuxInterfaceDriver plug and methods in derived class already check for
+ device existence
summary: - LinuxInterfaceDriver plug and methods in derived class already check for
+ LinuxInterfaceDriver plug method in derived class already check for
device existence
description: updated
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to neutron (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/116588

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron (master)

Change abandoned by Salvatore Orlando (<email address hidden>) on branch: master
Review: https://review.openstack.org/113897
Reason: This patch has been inactive long enough that I think it's safe to abandon.
The author can resurrect it if needed.

Changed in neutron:
status: In Progress → New
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by Salvatore Orlando (<email address hidden>) on branch: master
Review: https://review.openstack.org/116588
Reason: This patch has been inactive long enough that I think it's safe to abandon.
The author can resurrect it if needed.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

Fix proposed to branch: master
Review: https://review.openstack.org/147554

Changed in neutron:
assignee: Veena (mveenasl) → Sergey Belous (sbelous)
status: New → In Progress
Revision history for this message
Salvatore Orlando (salvatore-orlando) wrote :

The fact that plug verifies that device exists is a characteristic of all the implementations of LinuxInterfaceDriver, where plug is actually abstract.

I agree that there is an unnecessary duplicate check here.
I am ok with declaring that verifying that an interface exists before plugging is the responsability of the driver. This should be however documented, since in theory one could use his own custom driver (interface_driver is a configuration parameter).

Moreover since LinuxInterfaceDriver can use ip_lib perhaps the check should be done in the base class thus also removing some repeated logic in the various drivers.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron (master)

Reviewed: https://review.openstack.org/147554
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=6b4d006344e38dcbbc0048b17ca41af16e13e5a2
Submitter: Jenkins
Branch: master

commit 6b4d006344e38dcbbc0048b17ca41af16e13e5a2
Author: Sergey Belous <email address hidden>
Date: Thu Jan 15 18:19:51 2015 +0300

    Refactor checks for device existence

    The code calling driver.plug() shouldn't check for the device existence,
    it's a duplicate and it's an expensive call.
    Move check for device existence to base LinuxInterfaceDriver.plug()
    to remove code duplication. Make plug_new() abstract instead.

    Change-Id: Id118a64012ad10b197ba681ce5f1b2742eb135b4
    Closes-Bug:1348703

Changed in neutron:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (neutron-pecan)

Fix proposed to branch: neutron-pecan
Review: https://review.openstack.org/185072

Thierry Carrez (ttx)
Changed in neutron:
milestone: none → liberty-1
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in neutron:
milestone: liberty-1 → 7.0.0
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.