| Message ID | 1493118328-21311-1-git-send-email-dalvarez@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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 <dalvarez@redhat.com> Thanks a lot for working on this! My understanding is that the metadata API support, added to the DNS support that Numan Siddique is building, will complete OVN's basic OpenStack support, which will be great to have. Before I review this patch in detail, can you help me to understand what other components, in OVN or in Neutron, will be needed to make the overall metadata API feature work? Is it just a matter of plugging a bit into the Neutron networking-ovn driver, or do we need additional infrastructure in OVN beyond this patch? I'm trying to understand the full context here. Thanks, Ben.
On Tue, May 2, 2017 at 11:31 AM, Ben Pfaff <blp@ovn.org> 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 <dalvarez@redhat.com> > > Thanks a lot for working on this! My understanding is that the metadata > API support, added to the DNS support that Numan Siddique is building, > will complete OVN's basic OpenStack support, which will be great to > have. > > Before I review this patch in detail, can you help me to understand what > other components, in OVN or in Neutron, will be needed to make the > overall metadata API feature work? Is it just a matter of plugging a > bit into the Neutron networking-ovn driver, or do we need additional > infrastructure in OVN beyond this patch? I'm trying to understand the > full context here. More complete context is documented here: https://review.openstack.org/#/c/452811/ Latest rendered HTML version would be: http://docs-draft.openstack.org/11/452811/5/check/gate-networking-ovn-docs-ubuntu-xenial/6c6b664//doc/build/html/design/metadata_api.html This patch is the only missing piece from OVN. networking-ovn (the neutron-server driver) will create ports of type=localport. Then we will write an agent that runs on each node along side ovn-controller that will manage OVS ports and corresponding instances of haproxy.
On Tue, May 02, 2017 at 03:57:27PM -0400, Russell Bryant wrote: > On Tue, May 2, 2017 at 11:31 AM, Ben Pfaff <blp@ovn.org> 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 <dalvarez@redhat.com> > > > > Thanks a lot for working on this! My understanding is that the metadata > > API support, added to the DNS support that Numan Siddique is building, > > will complete OVN's basic OpenStack support, which will be great to > > have. > > > > Before I review this patch in detail, can you help me to understand what > > other components, in OVN or in Neutron, will be needed to make the > > overall metadata API feature work? Is it just a matter of plugging a > > bit into the Neutron networking-ovn driver, or do we need additional > > infrastructure in OVN beyond this patch? I'm trying to understand the > > full context here. > > More complete context is documented here: > > https://review.openstack.org/#/c/452811/ > > Latest rendered HTML version would be: > > http://docs-draft.openstack.org/11/452811/5/check/gate-networking-ovn-docs-ubuntu-xenial/6c6b664//doc/build/html/design/metadata_api.html > > This patch is the only missing piece from OVN. networking-ovn (the > neutron-server driver) will create ports of type=localport. Then we > will write an agent that runs on each node along side ovn-controller > that will manage OVS ports and corresponding instances of haproxy. OK, thanks, that context makes this easier to review.
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 <dalvarez@redhat.com> Hi Daniel, thanks again for working on this feature. Russell gave me enough context to review it, so here are some comments. The commit message cites "[0]" but doesn't say what [0] is. I'm a little surprised to see external-ids:ovn-localport-port in both the Port and Interface tables. I think that so far we've applied our special identifiers to one of these tables or the other, since most of the time either one or the other is most appropriate. In this case, I would be inclined to put it just on the Interface. Is there a reason that it should be present in both? 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.) 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. It would be helpful to readers to mention the particular Neutron example in the documentation, so that they can easily learn more about one use case for this feature. Maybe it would be appropriate to reference the OpenStack document that Russell mentioned, as a way to let them learn more. There is some unusual indentation in the test here--I would expect the "ovs-vsctl" and "ovn-nbctl" command names to be indented at the same level: + ovs-vsctl add-port br-int vif${i}1 -- \ + set Interface vif${i}1 external-ids:iface-id=lp${i}1 \ + options:tx_pcap=hv${i}/vif${i}1-tx.pcap \ + options:rxq_pcap=hv${i}/vif${i}1-rx.pcap \ + ofport-request=${i}1 + + ovn-nbctl lsp-add ls1 lp${i}1 + ovn-nbctl lsp-set-addresses lp${i}1 f0:00:00:00:00:${i}1 + ovn-nbctl lsp-set-port-security lp${i}1 f0:00:00:00:00:${i}1 + + ovn-sbctl list port_binding + ovn-sbctl show + ovn-sbctl list chassis + ovs-vsctl list interface + ovs-vsctl list open + ovn-nbctl lsp-get-up lp${i}1 This line in the test looks like it's probably something stray from debugging that should be removed: + ps -ef |grep ovn-controller 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. 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 again!
[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 <blp@ovn.org> 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 <dalvarez@redhat.com> > > > > > > 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.
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 <blp@ovn.org> 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 <blp@ovn.org> 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 <dalvarez@redhat.com>
> > > >
> > > > 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.
>
Thanks. This is a good summary of some of what the patch does. Should it be in the commit message? On Tue, May 09, 2017 at 05:09:43PM +0200, Daniel Alvarez Sanchez wrote: > 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 <blp@ovn.org> 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 <blp@ovn.org> 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 <dalvarez@redhat.com> > > > > > > > > > > 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. > >
On Tue, May 9, 2017 at 9:17 PM, Ben Pfaff <blp@ovn.org> wrote: > Thanks. > > This is a good summary of some of what the patch does. Should it be in > the commit message? > ack I ommited the commands and outputs in the commit message. In order to illustrate the purpose of this patch: - One logical switch sw0 with 2 ports (p1, p2) and 1 localport (lp) - Two hypervisors: HV1 and HV2 - p1 will be in HV1 (OVS port with external-id:iface-id="p1") - p2 will be in HV2 (OVS port with external-id:iface-id="p2") - lp will be in both (OVS port with external-id:iface-id="lp") - p1 should be able to reach p2 and viceversa - lp on HV1 should be able to reach p1 but not p2 - lp on HV2 should be able to reach p2 but not p1 ovn-nbctl ls-add sw0 ovn-nbctl lsp-add sw0 p1 ovn-nbctl lsp-add sw0 p2 ovn-nbctl lsp-add sw0 lp ovn-nbctl lsp-set-addresses p1 "00:00:00:aa:bb:10 10.0.1.10" ovn-nbctl lsp-set-addresses p2 "00:00:00:aa:bb:20 10.0.1.20" ovn-nbctl lsp-set-addresses lp "00:00:00:aa:bb:30 10.0.1.30" ovn-nbctl lsp-set-type lp localport add_phys_port() { name=$1 mac=$2 ip=$3 mask=$4 gw=$5 iface_id=$6 sudo ip netns add $name sudo ovs-vsctl add-port br-int $name -- set interface $name type=internal sudo ip link set $name netns $name sudo ip netns exec $name ip link set $name address $mac sudo ip netns exec $name ip addr add $ip/$mask dev $name sudo ip netns exec $name ip link set $name up sudo ip netns exec $name ip route add default via $gw sudo ovs-vsctl set Interface $name external_ids:iface-id=$iface_id } # Add p1 to HV1, p2 to HV2 and localport to both # HV1 add_phys_port p1 00:00:00:aa:bb:10 10.0.1.10 24 10.0.1.1 p1 add_phys_port lp 00:00:00:aa:bb:30 10.0.1.30 24 10.0.1.1 lp $ sudo ip netns exec p1 ping -c 2 10.0.1.20 PING 10.0.1.20 (10.0.1.20) 56(84) bytes of data. 64 bytes from 10.0.1.20: icmp_seq=1 ttl=64 time=0.738 ms 64 bytes from 10.0.1.20: icmp_seq=2 ttl=64 time=0.502 ms --- 10.0.1.20 ping statistics --- 2 packets transmitted, 2 received, 0% packet loss, time 1001ms rtt min/avg/max/mdev = 0.502/0.620/0.738/0.118 ms $ sudo ip netns exec lp ping -c 2 10.0.1.10 PING 10.0.1.10 (10.0.1.10) 56(84) bytes of data. 64 bytes from 10.0.1.10: icmp_seq=1 ttl=64 time=0.187 ms 64 bytes from 10.0.1.10: icmp_seq=2 ttl=64 time=0.032 ms --- 10.0.1.10 ping statistics --- 2 packets transmitted, 2 received, 0% packet loss, time 999ms rtt min/avg/max/mdev = 0.032/0.109/0.187/0.078 ms $ sudo ip netns exec lp ping -c 2 10.0.1.20 PING 10.0.1.20 (10.0.1.20) 56(84) bytes of data. --- 10.0.1.20 ping statistics --- 2 packets transmitted, 0 received, 100% packet loss, time 1000ms $ sudo ovs-ofctl dump-flows br-int | grep table=32 cookie=0x0, duration=141.939s, table=32, n_packets=2, n_bytes=196, idle_age=123, priority=150,reg14=0x3,reg15=0x2,metadata=0x7 actions=drop cookie=0x0, duration=141.939s, table=32, n_packets=2, n_bytes=196, idle_age=129, priority=100,reg15=0x2,metadata=0x7 actions=load:0x7->NXM_NX_TUN_ID[0..23],set_field:0x2->tun_metadata0,move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30],output:59 # On HV2 add_phys_port p2 00:00:00:aa:bb:20 10.0.1.20 24 10.0.1.1 p2 add_phys_port lp 00:00:00:aa:bb:30 10.0.1.30 24 10.0.1.1 lp $ sudo ip netns exec p2 ping -c 2 10.0.1.10 PING 10.0.1.10 (10.0.1.10) 56(84) bytes of data. 64 bytes from 10.0.1.10: icmp_seq=1 ttl=64 time=0.810 ms 64 bytes from 10.0.1.10: icmp_seq=2 ttl=64 time=0.673 ms --- 10.0.1.10 ping statistics --- 2 packets transmitted, 2 received, 0% packet loss, time 1000ms rtt min/avg/max/mdev = 0.673/0.741/0.810/0.073 ms $ sudo ip netns exec lp ping -c 2 10.0.1.20 PING 10.0.1.20 (10.0.1.20) 56(84) bytes of data. 64 bytes from 10.0.1.20: icmp_seq=1 ttl=64 time=0.357 ms 64 bytes from 10.0.1.20: icmp_seq=2 ttl=64 time=0.062 ms --- 10.0.1.20 ping statistics --- 2 packets transmitted, 2 received, 0% packet loss, time 1000ms rtt min/avg/max/mdev = 0.062/0.209/0.357/0.148 ms $ sudo ip netns exec lp ping -c 2 10.0.1.10 PING 10.0.1.10 (10.0.1.10) 56(84) bytes of data. --- 10.0.1.10 ping statistics --- 2 packets transmitted, 0 received, 100% packet loss, time 999ms $ sudo ovs-ofctl dump-flows br-int | grep table=32 cookie=0x0, duration=24.169s, table=32, n_packets=2, n_bytes=196, idle_age=12, priority=150,reg14=0x3,reg15=0x1,metadata=0x7 actions=drop cookie=0x0, duration=24.169s, table=32, n_packets=2, n_bytes=196, idle_age=14, priority=100,reg15=0x1,metadata=0x7 actions=load:0x7->NXM_NX_TUN_ID[0..23],set_field:0x1->tun_metadata0,move:NXM_NX_REG14[0..14]->NXM_NX_TUN_METADATA0[16..30],output:40 > On Tue, May 09, 2017 at 05:09:43PM +0200, Daniel Alvarez Sanchez wrote: > > 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_me > tadata0,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_me > tadata0,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 <blp@ovn.org> 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 <blp@ovn.org> 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 <dalvarez@redhat.com> > > > > > > > > > > > > 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/3319 > 50.html > > > > > > > > > > > > Thanks!! I picked those and submitted v2 :) > > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/3320 > 94.html > > > > > > > > > > > > > > > Thanks again! > > > > > > > > > > > > > > > > Thanks Ben for your great review. > > > > > > > > You're welcome, I'll try a look at v2 now. > > > >
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index 95e9deb..a0fb778 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -352,6 +352,43 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) hmap_destroy(&consistent_queues); netdev_close(netdev_phy); } +static bool +set_ovsport_external_ids(struct controller_ctx *ctx, + const struct ovsrec_bridge *bridge, + const struct ovsrec_interface *iface_rec, + const char *key, + const char *value) +{ + if (!ctx->ovs_idl_txn || !bridge || !iface_rec || !key) { + return false; + } + + /* Find the port with this interface and add the key/value pair to its + * external_ids. Assume that an interface corresponds only to one port. */ + int i; + for (i = 0; i < bridge->n_ports; i++) { + const struct ovsrec_port *port_rec = bridge->ports[i]; + int j; + + if (!strcmp(port_rec->name, bridge->name)) { + continue; + } + + for (j = 0; j < port_rec->n_interfaces; j++) { + if (port_rec->interfaces[j] == iface_rec) { + struct smap new_ids; + smap_clone(&new_ids, &port_rec->external_ids); + smap_replace(&new_ids, key, value); + ovsrec_port_verify_external_ids(port_rec); + ovsrec_port_set_external_ids(port_rec, &new_ids); + smap_destroy(&new_ids); + return true; + } + } + } + return false; +} + static void consider_local_datapath(struct controller_ctx *ctx, @@ -359,6 +396,7 @@ consider_local_datapath(struct controller_ctx *ctx, const struct lport_index *lports, const struct sbrec_chassis *chassis_rec, const struct sbrec_port_binding *binding_rec, + const struct ovsrec_bridge *bridge, struct hmap *qos_map, struct hmap *local_datapaths, struct shash *lport_to_iface, @@ -368,19 +406,40 @@ consider_local_datapath(struct controller_ctx *ctx, = shash_find_data(lport_to_iface, binding_rec->logical_port); bool our_chassis = false; - if (iface_rec - || (binding_rec->parent_port && binding_rec->parent_port[0] && - sset_contains(local_lports, binding_rec->parent_port))) { - if (binding_rec->parent_port && binding_rec->parent_port[0]) { - /* Add child logical port to the set of all local ports. */ - sset_add(local_lports, binding_rec->logical_port); - } - add_local_datapath(ldatapaths, lports, binding_rec->datapath, - false, local_datapaths); - if (iface_rec && qos_map && ctx->ovs_idl_txn) { - get_qos_params(binding_rec, qos_map); + + if (iface_rec && !strcmp(binding_rec->type, "localport")) { + /* Make sure localport external_id is present, otherwise set it + * for both interface and port. This should only happen the first + * time. We set the value on both the interface and port because it's + * most convenient to check for the value here on the interface. Later, + * we need to read this value in physical.c, and expect to read it from + * the port there.*/ + if (!smap_get(&iface_rec->external_ids, "ovn-localport-port")) { + if (set_ovsport_external_ids(ctx, bridge, iface_rec, + "ovn-localport-port", + binding_rec->logical_port)) { + struct smap new_ids; + smap_clone(&new_ids, &iface_rec->external_ids); + smap_replace(&new_ids, "ovn-localport-port", + binding_rec->logical_port); + ovsrec_interface_verify_external_ids(iface_rec); + ovsrec_interface_set_external_ids(iface_rec, &new_ids); + smap_destroy(&new_ids); + } } - our_chassis = true; + } else if (iface_rec + || (binding_rec->parent_port && binding_rec->parent_port[0] && + sset_contains(local_lports, binding_rec->parent_port))) { + if (binding_rec->parent_port && binding_rec->parent_port[0]) { + /* Add child logical port to the set of all local ports. */ + sset_add(local_lports, binding_rec->logical_port); + } + add_local_datapath(ldatapaths, lports, binding_rec->datapath, + false, local_datapaths); + if (iface_rec && qos_map && ctx->ovs_idl_txn) { + get_qos_params(binding_rec, qos_map); + } + our_chassis = true; } else if (!strcmp(binding_rec->type, "l2gateway")) { const char *chassis_id = smap_get(&binding_rec->options, "l2gateway-chassis"); @@ -468,8 +527,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, consider_local_datapath(ctx, ldatapaths, lports, chassis_rec, binding_rec, sset_is_empty(&egress_ifaces) ? NULL : - &qos_map, local_datapaths, &lport_to_iface, - local_lports); + br_int, &qos_map, local_datapaths, + &lport_to_iface, local_lports); } diff --git a/ovn/controller/ovn-controller.8.xml b/ovn/controller/ovn-controller.8.xml index f9cbbfe..e6d5cd5 100644 --- a/ovn/controller/ovn-controller.8.xml +++ b/ovn/controller/ovn-controller.8.xml @@ -305,6 +305,21 @@ logical patch port that it implements. </p> </dd> + + <dt> + <code>external_ids:ovn-localport-port</code> in the <code>Port</code> + and <code>Interface</code> tables + </dt> + + <dd> + <p> + The presence of this key identifies a port as a + <code>localport</code> so that <code>ovn-controller</code> can + properly set the right flows to allow only local traffic and + drop any packets directed to an external chassis. + </p> + </dd> + </dl> <h1>Runtime Management Commands</h1> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index 0f1aa63..6a745f8 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -59,6 +59,8 @@ physical_register_ovs_idl(struct ovsdb_idl *ovs_idl) static struct simap localvif_to_ofport = SIMAP_INITIALIZER(&localvif_to_ofport); static struct hmap tunnels = HMAP_INITIALIZER(&tunnels); +static struct simap localport_to_ofport = + SIMAP_INITIALIZER(&localport_to_ofport); /* Maps from a chassis to the OpenFlow port number of the tunnel that can be * used to reach that chassis. */ @@ -601,6 +603,27 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, } else { /* Remote port connected by tunnel */ + /* Table 32, priority 150. + * ======================= + * + * Drop traffic originated from a localport to a remote destination. + */ + struct simap_node *localport; + SIMAP_FOR_EACH (localport, &localport_to_ofport) { + int inport; + if ((inport = simap_get(&localport_to_ofport, localport->name))) { + match_init_catchall(&match); + ofpbuf_clear(ofpacts_p); + /* Match localport in_port. */ + match_set_in_port(&match, inport); + /* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */ + match_set_metadata(&match, htonll(dp_key)); + match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key); + ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, 0, + &match, ofpacts_p); + } + } + /* Table 32, priority 100. * ======================= * @@ -753,6 +776,34 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, sset_destroy(&remote_chassis); } +static bool +simap_sync(struct simap *old, const struct simap *new) +{ + struct simap_node *vif_name, *vif_name_next; + bool has_changed = false; + + SIMAP_FOR_EACH_SAFE (vif_name, vif_name_next, old) { + int newport; + if ((newport = simap_get(new, vif_name->name))) { + if (newport != simap_get(old, vif_name->name)) { + simap_put(old, vif_name->name, newport); + has_changed = true; + } + } else { + simap_find_and_delete(old, vif_name->name); + has_changed = true; + } + } + SIMAP_FOR_EACH (vif_name, new) { + if (!simap_get(old, vif_name->name)) { + simap_put(old, vif_name->name, + simap_get(new, vif_name->name)); + has_changed = true; + } + } + return has_changed; +} + void physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, const struct ovsrec_bridge *br_int, @@ -768,6 +819,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, SIMAP_INITIALIZER(&new_localvif_to_ofport); struct simap new_tunnel_to_ofport = SIMAP_INITIALIZER(&new_tunnel_to_ofport); + struct simap new_localport_to_ofport = + SIMAP_INITIALIZER(&new_localport_to_ofport); + for (int i = 0; i < br_int->n_ports; i++) { const struct ovsrec_port *port_rec = br_int->ports[i]; if (!strcmp(port_rec->name, br_int->name)) { @@ -784,6 +838,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, "ovn-localnet-port"); const char *l2gateway = smap_get(&port_rec->external_ids, "ovn-l2gateway-port"); + const char *localport = smap_get(&port_rec->external_ids, + "ovn-localport-port"); for (int j = 0; j < port_rec->n_interfaces; j++) { const struct ovsrec_interface *iface_rec = port_rec->interfaces[j]; @@ -848,6 +904,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, "iface-id"); if (iface_id) { simap_put(&new_localvif_to_ofport, iface_id, ofport); + if (localport) { + simap_put(&new_localport_to_ofport, localport, ofport); + } } } } @@ -864,26 +923,11 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, } /* Capture changed or removed openflow ports. */ - struct simap_node *vif_name, *vif_name_next; - SIMAP_FOR_EACH_SAFE (vif_name, vif_name_next, &localvif_to_ofport) { - int newport; - if ((newport = simap_get(&new_localvif_to_ofport, vif_name->name))) { - if (newport != simap_get(&localvif_to_ofport, vif_name->name)) { - simap_put(&localvif_to_ofport, vif_name->name, newport); - physical_map_changed = true; - } - } else { - simap_find_and_delete(&localvif_to_ofport, vif_name->name); - physical_map_changed = true; - } - } - SIMAP_FOR_EACH (vif_name, &new_localvif_to_ofport) { - if (!simap_get(&localvif_to_ofport, vif_name->name)) { - simap_put(&localvif_to_ofport, vif_name->name, - simap_get(&new_localvif_to_ofport, vif_name->name)); - physical_map_changed = true; - } - } + 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); if (physical_map_changed) { /* Reprocess logical flow table immediately. */ poll_immediate_wake(); @@ -1043,4 +1087,5 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, simap_destroy(&new_localvif_to_ofport); simap_destroy(&new_tunnel_to_ofport); + simap_destroy(&new_localport_to_ofport); } diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml index ab8fd88..c4552c6 100644 --- a/ovn/northd/ovn-northd.8.xml +++ b/ovn/northd/ovn-northd.8.xml @@ -521,8 +521,8 @@ output; </pre> <p> - These flows are omitted for logical ports (other than router ports) - that are down. + These flows are omitted for logical ports (other than router ports or + <code>localport</code> ports) that are down. </p> </li> @@ -548,8 +548,8 @@ nd_na { </pre> <p> - These flows are omitted for logical ports (other than router ports) - that are down. + These flows are omitted for logical ports (other than router ports or + <code>localport</code> ports) that are down. </p> </li> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index d0a5ba2..126c89f 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -3252,9 +3252,11 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, /* * Add ARP/ND reply flows if either the * - port is up or - * - port type is router + * - port type is router or + * - port type is localport */ - if (!lsp_is_up(op->nbsp) && strcmp(op->nbsp->type, "router")) { + if (!lsp_is_up(op->nbsp) && strcmp(op->nbsp->type, "router") && + strcmp(op->nbsp->type, "localport")) { continue; } diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml index d8114f1..2866666 100644 --- a/ovn/ovn-architecture.7.xml +++ b/ovn/ovn-architecture.7.xml @@ -409,6 +409,15 @@ logical patch ports at each such point of connectivity, one on each side. </li> + <li> + <dfn>Localport ports</dfn> represent the points of local + connectivity between logical switches and VIFs. These ports are + present in every chassis (not bound to any particular one) and + traffic from them will never go through a tunnel. A + <code>localport</code> is expected to only generate traffic destined + for a local destination, typically in response to a request it + received. + </li> </ul> </li> </ul> @@ -986,11 +995,12 @@ hypervisor. Each flow's actions implement sending a packet to the port it matches. For unicast logical output ports on remote hypervisors, the actions set the tunnel key to the correct value, then send the - packet on the tunnel port to the correct hypervisor. (When the remote - hypervisor receives the packet, table 0 there will recognize it as a - tunneled packet and pass it along to table 33.) For multicast logical - output ports, the actions send one copy of the packet to each remote - hypervisor, in the same way as for unicast destinations. If a + packet on the tunnel port to the correct hypervisor (unless the packet + comes from a localport, in which case it will be dropped). (When the + remote hypervisor receives the packet, table 0 there will recognize it + as a tunneled packet and pass it along to table 33.) For multicast + logical output ports, the actions send one copy of the packet to each + remote hypervisor, in the same way as for unicast destinations. If a multicast group includes a logical port or ports on the local hypervisor, then its actions also resubmit to table 33. Table 32 also includes a fallback flow that resubmits to table 33 if there is no diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml index 7a1c20e..9b73692 100644 --- a/ovn/ovn-nb.xml +++ b/ovn/ovn-nb.xml @@ -259,6 +259,15 @@ to model direct connectivity to an existing network. </dd> + <dt><code>localport</code></dt> + <dd> + A connection to a local VIF. Traffic that arrives on a + <code>localport</code> is never forwarded over a tunnel to another + chassis. These ports are present on every chassis and have the same + address in all of them. This is used to model connectivity to local + services that run on every hypervisor. + </dd> + <dt><code>l2gateway</code></dt> <dd> A connection to a physical network. diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml index 5542f7e..b4cfd71 100644 --- a/ovn/ovn-sb.xml +++ b/ovn/ovn-sb.xml @@ -311,7 +311,7 @@ transmitted and received with reasonable performance. It is a hint to senders transmitting data to this chassis that they should use checksums to protect OVN metadata. <code>ovn-controller</code> - populates this key with the value defined in + populates this key with the value defined in <ref table="Open_vSwitch" column="external_ids:ovn-encap-csum"/> column of the Open_vSwitch database's <ref table="Open_vSwitch" db="Open_vSwitch"/> table. Other applications should treat this key as @@ -1739,6 +1739,11 @@ tcp.flags = RST; connectivity to the corresponding physical network. </dd> + <dt>localport</dt> + <dd> + Always empty. A localport port is present on every chassis. + </dd> + <dt>l3gateway</dt> <dd> The physical location of the L3 gateway. To successfully identify a @@ -1819,6 +1824,15 @@ tcp.flags = RST; to model direct connectivity to an existing network. </dd> + <dt><code>localport</code></dt> + <dd> + A connection to a local VIF. Traffic that arrives on a + <code>localport</code> is never forwarded over a tunnel to another + chassis. These ports are present on every chassis and have the same + address in all of them. This is used to model connectivity to local + services that run on every hypervisor. + </dd> + <dt><code>l2gateway</code></dt> <dd> An L2 connection to a physical network. The chassis this diff --git a/tests/ovn.at b/tests/ovn.at index 088bbf6..22ba1ac 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -6994,3 +6994,132 @@ OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected]) OVN_CLEANUP([hv1],[hv2]) AT_CLEANUP + +AT_SETUP([ovn -- 2 HVs, 1 lport/HV, localport ports]) +AT_SKIP_IF([test $HAVE_PYTHON = no]) +ovn_start + +ovn-nbctl ls-add ls1 + +# Add localport to the switch +ovn-nbctl lsp-add ls1 lp01 +ovn-nbctl lsp-set-addresses lp01 f0:00:00:00:00:01 +ovn-nbctl lsp-set-type lp01 localport + +net_add n1 + +for i in 1 2; do + sim_add hv$i + as hv$i + ovs-vsctl add-br br-phys + ovn_attach n1 br-phys 192.168.0.$i + ovs-vsctl add-port br-int vif01 -- \ + set Interface vif01 external-ids:iface-id=lp01 \ + options:tx_pcap=hv${i}/vif01-tx.pcap \ + options:rxq_pcap=hv${i}/vif01-rx.pcap \ + ofport-request=${i}0 + + ovs-vsctl add-port br-int vif${i}1 -- \ + set Interface vif${i}1 external-ids:iface-id=lp${i}1 \ + options:tx_pcap=hv${i}/vif${i}1-tx.pcap \ + options:rxq_pcap=hv${i}/vif${i}1-rx.pcap \ + ofport-request=${i}1 + + ovn-nbctl lsp-add ls1 lp${i}1 + ovn-nbctl lsp-set-addresses lp${i}1 f0:00:00:00:00:${i}1 + ovn-nbctl lsp-set-port-security lp${i}1 f0:00:00:00:00:${i}1 + + ovn-sbctl list port_binding + ovn-sbctl show + ovn-sbctl list chassis + ovs-vsctl list interface + ovs-vsctl list open + ovn-nbctl lsp-get-up lp${i}1 + ps -ef |grep ovn-controller + OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lp${i}1` = xup]) +done + +ovn-nbctl --wait=sb sync +ovn-sbctl dump-flows + +ovn_populate_arp + +# Given the name of a logical port, prints the name of the hypervisor +# on which it is located. +vif_to_hv() { + echo hv${1%?} +} +# +# test_packet INPORT DST SRC ETHTYPE EOUT LOUT DEFHV +# +# This shell function causes a packet to be received on INPORT. The packet's +# content has Ethernet destination DST and source SRC (each exactly 12 hex +# digits) and Ethernet type ETHTYPE (4 hex digits). INPORT is specified as +# logical switch port numbers, e.g. 11 for vif11. +# +# EOUT is the end-to-end output port, that is, where the packet will end up +# after possibly bouncing through one or more localnet ports. LOUT is the +# logical output port, which might be a localnet port, as seen by ovn-trace +# (which doesn't know what localnet ports are connected to and therefore can't +# figure out the end-to-end answer). +# +# DEFHV is the default hypervisor from where the packet is going to be sent +# if the source port is a localport. +for i in 1 2; do + for j in 0 1; do + : > $i$j.expected + done +done +test_packet() { + local inport=$1 dst=$2 src=$3 eth=$4 eout=$5 lout=$6 defhv=$7 + echo "$@" + + # First try tracing the packet. + uflow="inport==\"lp$inport\" && eth.dst==$dst && eth.src==$src && eth.type==0x$eth" + if test $lout != drop; then + echo "output(\"$lout\");" + fi > expout + AT_CAPTURE_FILE([trace]) + AT_CHECK([ovn-trace --all ls1 "$uflow" | tee trace | sed '1,/Minimal trace/d'], [0], [expout]) + + # Then actually send a packet, for an end-to-end test. + local packet=$(echo $dst$src | sed 's/://g')${eth} + hv=`vif_to_hv $inport` + # If hypervisor 0 (localport) use the defhv parameter + if test $hv == hv0; then + hv=$defhv + fi + vif=vif$inport + as $hv ovs-appctl netdev-dummy/receive $vif $packet + if test $eout != drop; then + echo $packet >> ${eout#lp}.expected + fi +} + + +# lp11 and lp21 are on different hypervisors +test_packet 11 f0:00:00:00:00:21 f0:00:00:00:00:11 1121 lp21 lp21 +test_packet 21 f0:00:00:00:00:11 f0:00:00:00:00:21 2111 lp11 lp11 + +# Both VIFs should be able to reach the localport on their own HV +test_packet 11 f0:00:00:00:00:01 f0:00:00:00:00:11 1101 lp01 lp01 +test_packet 21 f0:00:00:00:00:01 f0:00:00:00:00:21 2101 lp01 lp01 + +# Packet sent from localport on same hv should reach the vif +test_packet 01 f0:00:00:00:00:11 f0:00:00:00:00:01 0111 lp11 lp11 hv1 +test_packet 01 f0:00:00:00:00:21 f0:00:00:00:00:01 0121 lp21 lp21 hv2 + +# Packet sent from localport on different hv should be dropped +test_packet 01 f0:00:00:00:00:21 f0:00:00:00:00:01 0121 drop lp21 hv1 +test_packet 01 f0:00:00:00:00:11 f0:00:00:00:00:01 0111 drop lp11 hv2 + +# Now check the packets actually received against the ones expected. +for i in 1 2; do + for j in 0 1; do + OVN_CHECK_PACKETS([hv$i/vif$i$j-tx.pcap], [$i$j.expected]) + done +done + +OVN_CLEANUP([hv1],[hv2]) + +AT_CLEANUP
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 <dalvarez@redhat.com> --- ovn/controller/binding.c | 87 ++++++++++++++++++++---- ovn/controller/ovn-controller.8.xml | 15 +++++ ovn/controller/physical.c | 85 ++++++++++++++++++------ ovn/northd/ovn-northd.8.xml | 8 +-- ovn/northd/ovn-northd.c | 6 +- ovn/ovn-architecture.7.xml | 20 ++++-- ovn/ovn-nb.xml | 9 +++ ovn/ovn-sb.xml | 16 ++++- tests/ovn.at | 129 ++++++++++++++++++++++++++++++++++++ 9 files changed, 329 insertions(+), 46 deletions(-)