[ovs-dev] [PATCH] ovn-northd: Only peer router ports to other router ports.

[nickcooper-zhangtonghao]

> On Jul 28, 2016, at 5:01 AM, Ben Pfaff <blp at ovn.org> wrote:
> 
> On Mon, Jul 25, 2016 at 09:19:00AM -0700, nickcooper-zhangtonghao wrote:
>> Deletion of a logical router port, which may be an other
>> router peer, result in a WARN, because the "ports" variable
>> still contains the logical port deleted. This port exists
>> but should not have been initialized fully.
>> 
>> nbsp == NULL, nbrp == NULL
>> 
>> It is necessary to check 'nbsp'.
>> 
>> A router port's "peer", if set, must point to another router port, but the
>> code as written also accepted switch ports.  This caused problems when
>> switch ports were actually specified.
>> 
>> Reported-by: Gurucharan Shetty <guru at ovn.org>
>> Reported-at: http://openvswitch.org/pipermail/dev/2016-July/075524.html
>> Signed-off-by: Ben Pfaff <blp at ovn.org>
>> Acked-by: Gurucharan Shetty <guru at ovn.org>
>> Signed-off-by: nickcooper-zhangtonghao <nickcooper-zhangtonghao at opencloud.tech>
>> ---
>> ovn/northd/ovn-northd.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> index a3d1672..efc915c 100644
>> --- a/ovn/northd/ovn-northd.c
>> +++ b/ovn/northd/ovn-northd.c
>> @@ -746,9 +746,15 @@ join_logical_ports(struct northd_context *ctx,
>>         } else if (op->nbrp && op->nbrp->peer) {
>>             struct ovn_port *peer = ovn_port_find(ports, op->nbrp->peer);
>>             if (peer) {
>> +                /* Deletion of a logical router port, which may be an other
>> +                 * router peer, result in a WARN, because the "ports" variable
>> +                 * still contains the logical port deleted. This port exists 
>> +                 * but should not have been initialized fully.
>> +                 *
>> +                 * nbsp == NULL, nbrp == NULL */
>>                 if (peer->nbrp) {
>>                     op->peer = peer;
>> -                } else {
>> +                } else if (peer->nbsp) {
>>                     /* An ovn_port for a switch port of type "router" does have
>>                      * a router port as its peer (see the case above for
>>                      * "router" ports), but this is set via options:router-port
> 
> I don't understand this yet.  I don't see how an ovn_port's nbsp and
> nbrp can both be NULL.  I only see two calls to ovn_port_create(), and
> each of them supplies either nbsp or nbrp as nonnull.
> 
> Can you explain further?
> 
> Thanks,
> 
> Ben.


Run the commands below, you will get a WARN info in the ovn-northd.log

ovn-nbctl lr-add lr0
ovn-nbctl lr-add lr1
ovn-nbctl lrp-add lr0 lr0-p0 00:11:22:33:44:55 192.168.10.10/24 peer=lr1-p0
ovn-nbctl lrp-add lr1 lr1-p0 00:11:22:33:44:66 192.168.20.10/24 peer=lr0-p0
ovn-nbctl lrp-del lr1-p0

WARN INFO:
“Bad configuration: The peer of router port lr0-p0 is a switch port.”


Here’s is why:
In the ‘join_logical_ports’ function,  firstly, we get the ‘ovn_port’s from 
SB database and save them to “ports” variable. The ovn_port’s nbsp and nbrp both
are NULL via ovn_port_create.

Next, in the HMAP_FOR_EACH  loop, we initialize the ‘ovn_port’s except which 
were deleted on the NB database. The ovn_port which is deleted in NB database 
is still in SB database. so, still in “ports” variable.

Then, when connected to their peer, the ovn_port deleted may be found as a peer.
But the ovn_port's nbsp and nbsp both are NULL.

Thanks,
zhangtonghao