l2pop to stop using tuples everywhere

[YAMAMOTO Takashi]

l2pop port_info used to be [mac, ip].
recently merged DVR changes made it [mac, device_owner, ip] and broke
existing code which accesses ip as port_info[1].
as it's on-wire messages, it would be better to make it [mac, ip, device_owner]
to maintain compatibility as far as possible.

references:

commit cd35b1904e1da5e3da0a63b37af34d426e72e933
inserted device_owner into fdb entry and broke OVS arp responder which uses port_info[1] as ip.

commit 029eff322d64e7adecbd177b14329eb2fb08cccc
introduced code to use port_info[1] as device_owner while keeping existing code which uses it as ip.

[OpenStack Infra (hudson-openstack)]

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

[Armando Migliaccio (armando-migliaccio)]

During one of the reviews for the ML2 DVR additions [1] we discussed the desire of refactoring this code, to make sure the reduce the dependency of positional arguments. Also, having a better split of the interfaces between agent and plugin would be better; at the moment when looking at the rpc mixin it's very difficult to understand what's a local call and what is a remote one.

[1] - https://review.openstack.org/#/c/102398/48/neutron/plugins/ml2/drivers/l2pop/mech_driver.py

[Carl Baldwin (carl-baldwin)]

+1 to refactoring as I discussed this with Armando during the review. I could offer to refactor this code today or tomorrow when I get a little time.

[YAMAMOTO Takashi (yamamoto)]

i thought tuple was used for reasons. eg. smaller payload.
i think you should ask l2pop folks.
(in that sense, i'm not sure if the addition of device_owner was a good idea.)

[YAMAMOTO Takashi (yamamoto)]

Carl,

thank you.

i'll suspend my fix (https://review.openstack.org/#/c/111966/) for a few days to avoid stepping on your toes.
please ping me if/when you want me abandon or resume it.

[OpenStack Infra (hudson-openstack)]

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

[Armando Migliaccio (armando-migliaccio)]

Whatever the reason tuples where chosen, maintainability and extensibility is seriously affected by their adoption. We should have stopped this practice long time ago. I don't think that smaller payload is a good reason either. Serializing a json object is not that expensive these days. We are in 2014

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Change abandoned by Carl Baldwin (<email address hidden>) on branch: master
Review: https://review.openstack.org/111966
Reason: We've decided that device_owner can safely be removed from the tuple because the need for it went away during DVR development and it was not cleaned up. I have filed a patch to clean this up here:

https://review.openstack.org/112984

It makes this review obsolete. Let me know if you disagree.

[OpenStack Infra (hudson-openstack)]

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

commit c5fafcb30a5b86e87309ad4650f7d05a2ca038dc
Author: Carl Baldwin <email address hidden>
Date: Fri Aug 8 17:31:59 2014 +0000

    Remove unneeded device_owner field from l2pop tuple

    The DVR development added this device_owner to the middle of this
    tuple during early development because it was thought to be needed.
    Over the course of development, it was found to be unnecessary and
    much of the code that read it from this value was removed or
    obsoleted. That job went unfinished and so this commit completes it.
    This essentially restores the code to what it was before and fixes the
    regression that was caused.

    Change-Id: Ia901f925883b53e9880dd25688e16e0ffe402bf4
    Partial-bug: #1352801

[Carl Baldwin (carl-baldwin)]

The partial fix in comment #10 fixes the regression here. I think that downgrades the importance of this bug a bit.

[OpenStack Infra (hudson-openstack)]

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

[Kyle Mestery (mestery)]

Moving medium priority bug out of Juno-RC1.

[Kyle Mestery (mestery)]

Per my own confusion, moving back to Juno-RC1.

[Carl Baldwin (carl-baldwin)]

Kyle,

This is probably okay to move out to kilo.

Carl

On Sep 17, 2014, at 9:14 AM, Kyle Mestery <email address hidden> wrote:

> Per my own confusion, moving back to Juno-RC1.
>
> ** Changed in: neutron
> Milestone: kilo-1 => juno-rc1
>
> --
> You received this bug notification because you are a bug assignee.
> https://bugs.launchpad.net/bugs/1352801
>
> Title:
> l2pop to stop using tuples everywhere
>
> Status in OpenStack Neutron (virtual network service):
> In Progress
>
> Bug description:
> l2pop port_info used to be [mac, ip].
> recently merged DVR changes made it [mac, device_owner, ip] and broke
> existing code which accesses ip as port_info[1].
> as it's on-wire messages, it would be better to make it [mac, ip, device_owner]
> to maintain compatibility as far as possible.
>
> references:
>
> commit cd35b1904e1da5e3da0a63b37af34d426e72e933
> inserted device_owner into fdb entry and broke OVS arp responder which uses port_info[1] as ip.
>
> commit 029eff322d64e7adecbd177b14329eb2fb08cccc
> introduced code to use port_info[1] as device_owner while keeping existing code which uses it as ip.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/neutron/+bug/1352801/+subscriptions

[OpenStack Infra (hudson-openstack)]

Change abandoned by Carl Baldwin (<email address hidden>) on branch: master
Review: https://review.openstack.org/114014
Reason: Will restore when it is relevant

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/112178
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=8f50bc29edf0788dbc6220d56b45334eab9ecdc9
Submitter: Jenkins
Branch: master

commit 8f50bc29edf0788dbc6220d56b45334eab9ecdc9
Author: Carl Baldwin <email address hidden>
Date: Wed Aug 6 01:02:09 2014 +0000

    Refactor l2_pop code to pass mac/ip info more readably

    Previous code used a 2 element array to represent a mac/ip address
    pair. Code assumed that element 0 was mac and 1 was ip. This made
    the code difficult to read and difficult to maintain. An attempt was
    made to insert a third value that failed miserably because of the
    position dependence and other code that assumed not only positions but
    also the number of elements.

    Using a namedtuple seems to be the best way to get better
    maintainability. Named tuples can be compared with regular tuples.
    The json serializer still uses an array to represent it so the on-wire
    representation of the object has not changed. A short snip of code
    was required to restore the namedtuple from the RPC message.

    Change-Id: I7f8c93b0e12ee0179bb23dfbb3a3d814615b1c2e
    Closes-Bug: #1352801