Hi,
I've submitted a new patch v3 where I removed the external-id
"ovn-localport"
from the Interface which I used to identify a port as localport in
physical.c.
Instead, I have passed another parameter "local_lports" to physical_run.
When
inserting flows in table 32, I'm inserting higher priority drop flows for
every
local localport.
In order to illustrate this:
LS1 switch1 with 3 ports: 2 ports and 1 localport
$ ovn-nbctl show switch1
switch c2a81271-b77f-4019-b877-6428c8b647d7 (switch1)
port p2
addresses: ["00:00:00:01:01:11 20.0.0.11"]
port lp1
type: localport
addresses: ["00:00:00:01:01:05 20.0.0.5"]
port p1
addresses: ["00:00:00:01:01:10 20.0.0.10"]
Two HVs: hv1 and hv2.
p1 (tunnel_key = 2) is in hv1
p2 (tunnel_key = 4) is in hv2
lp1 (tunnel_key = 1) is in both hv1 and hv2
(When i say that a port is in a certain hv i'm saying that there's an OVS
port
with external-id:iface-id=<port>")
Table 32 in hv2 looks like:
cookie=0x0, duration=2077.204s, table=32, n_packets=2, n_bytes=196,
idle_age=114, priority=150,reg14=0x1,reg15=0x2,metadata=0x5 actions=drop
cookie=0x0, duration=2077.214s, table=32, n_packets=0, n_bytes=0,
idle_age=2077, priority=100,reg15=0x2,metadata=0x5
actions=load:0x5->NXM_NX_TUN_ID[0..23],set_field:0x2->tun_metadata0,move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30],output:34
Which means that if a packet is directed to p1 (reg15=0x2) send it through
the tunnel unless it comes from lp1 (reg14=0x1) when it'll be dropped.
Table 32 in hv1 looks like:
cookie=0x0, duration=15.193s, table=32, n_packets=0, n_bytes=0,
idle_age=15, priority=150,reg14=0x1,reg15=0x4,metadata=0x5 actions=drop
cookie=0x0, duration=15.193s, table=32, n_packets=0, n_bytes=0,
idle_age=15, priority=100,reg15=0x4,metadata=0x5
actions=load:0x5->NXM_NX_TUN_ID[0..23],set_field:0x4->tun_metadata0,move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30],output:28
Which means that if a packet is directed to p1 (reg15=0x4) send it through
the tunnel unless it comes from lp1 (reg14=0x1) when it'll be dropped.
On Fri, May 5, 2017 at 5:51 PM, Ben Pfaff <[email protected]> wrote:
> [oops, adding back the list]
>
> On Fri, May 05, 2017 at 08:51:01AM -0700, Ben Pfaff wrote:
> > On Fri, May 05, 2017 at 02:58:45PM +0200, Daniel Alvarez Sanchez wrote:
> > > Thanks a lot Ben for taking the time to review the patch and submit
> > > the 3 patch series.
> > >
> > > On Wed, May 3, 2017 at 11:54 PM, Ben Pfaff <[email protected]> wrote:
> > >
> > > > On Tue, Apr 25, 2017 at 11:05:28AM +0000, Daniel Alvarez wrote:
> > > > > This patch introduces a new type of OVN ports called "localport".
> > > > > These ports will be present in every hypervisor and may have the
> > > > > same IP/MAC addresses. They are not bound to any chassis and
> traffic
> > > > > to these ports will never go through a tunnel.
> > > > >
> > > > > Its main use case is the OpenStack metadata API support which
> relies
> > > > > on a local agent running on every hypervisor and serving metadata
> to
> > > > > VM's locally. This service is described in detail at [0].
> > > > >
> > > > > Signed-off-by: Daniel Alvarez <[email protected]>
> > > >
> > > > In consider_local_datapath(), the ovn-localport-port logic only
> fires if
> > > > ovn-localport-port is completely unset. Usually, it's better to make
> > > > sure that everything that ovn-controller sets happens every time,
> > > > because this fixes up damage if any occurs. So, for example, in this
> > > > case, we would tend to always set ovn-localport-port. (Sometimes,
> this
> > > > can be expensive, and so we come up with ways to avoid it, but I
> don't
> > > > have a reason to believe that it is expensive in this case.)
> > > >
> > > Ack. However, now that I've made previous change (only setting it on
> > > the Interface, I think we can keep it like this because that
> > > "ovn-localport-port logic" is simply setting the external-id if
> > > unset. Or you would just prefer to set it anyways? There's no more
> > > logic apart from this now that I removed setting it also in the Port
> > > table.
> >
> > I'm not sure I understand. I'll see when I read the next version, no
> > problem.
> >
> > > > The documentation for external-ids:ovn-localport-port should say
> what
> > > > the key's value is.
> > > >
> > > > Actually, the purpose of external-ids:ovn-localport-port is not
> entirely
> > > > clear to me. The other documented keys in this category have the
> > > > following uses:
> > > >
> > > > external-ids:ovn-localnet-port
> > > > external-ids:ovn-l2gateway-port
> > > > external-ids:ovn-l3gateway-port
> > > >
> > > > ovn-controller creates these ports itself and it adds
> these
> > > > keys to them to indicate that it owns them; otherwise
> that
> > > > would be ambiguous and therefore it would be risky for
> > > > ovn-controller to later delete them.
> > > >
> > > > external-ids:ovn-logical-patch-port
> > > >
> > > > ovn-controller doesn't use these anymore; the
> documentation
> > > > is obsolete and should be removed.
> > > >
> > > > But it looks to me that ovn-controller doesn't create this new kind
> of
> > > > port (and doesn't delete it either). It seems that, so far,
> > > > ovn-controller only uses this key to internally communicate with two
> > > > pieces of itself, which doesn't seem like a great approach to me.
> > > >
> > > I see your point. Right now, I'm setting this key in binding.c so that,
> > > when in physical.c we're iterating over ports to set the flows, I can
> > > have the corresponding ofport for every localport and, for each,
> > > output flow insert a drop if the traffic comes from a localport and
> > > tries to go through a tunnel.
> > >
> > > Other thing I could do at consider_port_binding in physical.c at [0]
> is:
> > >
> > > 1. SBREC_PORT_BINDING_FOR_EACH(entry):
> > > 2. Insert a drop flow if entry->type == "localport" matching
> > > by logical inport
> > >
> > > What do you think about this? I think current implementation is more
> > > efficient
> > > since we don't have to iterate over all sbrec_port_binding entries
> every
> > > time
> > > for every flow we want to insert in table 32 but we'd get rid of the
> > > ovn-localport-port external_id.
> >
> > Usually I'd suggest a shared data structure. The "binding" code already
> > constructs data structures to keep track of logical ports, for example.
> >
> > > > Also, I noticed that this patch re-set physical_map_changed based on
> > > > update_ofports(), although it should have honored an existing 'true'
> > > > value there.
> > > >
> > >
> > > I might be missing something but what I just wanted to set
> > > physical_map_changed to true if an update on 'localport_to_ofport'
> > > happened. Otherwise, leave it as it was before. What am I missing?
> > >
> > > /* Capture changed or removed openflow ports. */
> > > physical_map_changed |= update_ofports(&localvif_to_ofport,
> > >
> > > &new_localvif_to_ofport);
> > > physical_map_changed |= update_ofports(&localport_to_ofport,
> > >
> > > &new_localport_to_ofport);
> >
> > Sure, that would be fine, but the patch actually used = instead of |=:
> > + physical_map_changed = simap_sync(&localvif_to_ofport,
> > + &new_localvif_to_ofport);
> > + /* Do the same for all localports. */
> > + physical_map_changed |= simap_sync(&localport_to_ofport,
> > + &new_localport_to_ofport);
> >
> > > Finally, this patch reuses some code from physical_run() that was more
> > > > complicated than necessary. Not your fault--it was natural to reuse
> > > > it--but I decided to rewrite and simplify it. So, I sent this out
> as a
> > > > 3-patch series with that simplification as the first 2 patches.
> Would
> > > > you mind picking it up from that series? It's posted starting here:
> > > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/331950.html
> > > >
> > > > Thanks!! I picked those and submitted v2 :)
> > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/332094.html
> > >
> > >
> > > Thanks again!
> > > >
> > >
> > > Thanks Ben for your great review.
> >
> > You're welcome, I'll try a look at v2 now.
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
