Re: [ovs-dev] [PATCH v4] OVN localport type support
Hi Ben, Thanks for proposing this optimization. I've tried it and it works. These flows are inserted in table 32 when I have a logical switch with two logical ports and one localport: cookie=0x0, duration=528.253s, table=32, n_packets=416, n_bytes=17584, idle_age=1, priority=150,reg14=0x3,metadata=0x9 actions=resubmit(,33) Also, if I try to ping a port in a different chassis from a localport, the ARP request gets dropped in table 34: cookie=0x0, duration=568.319s, table=34, n_packets=448, n_bytes=18816, idle_age=1, priority=100,reg10=0/0x1,reg14=0x3,reg15=0x3,metadata=0x9 actions=drop If I insert an static entry in the ARP table for that remote port, then the packet gets forwarded to table 33 but dropped there due to no hits. I just submitted a version 5 of the patch including the links to OpenStack documentation (the patch got already merged) and a minor indentation issue. Thanks once again! Daniel On Fri, May 26, 2017 at 6:46 AM, Ben Pfaffwrote: > On Tue, May 23, 2017 at 03:13:08PM +0200, Daniel Alvarez Sanchez wrote: > > On Tue, May 23, 2017 at 10:01 AM, Miguel Angel Ajo Pelayo < > > majop...@redhat.com> wrote: > > > > > If we forsee use cases with several local ports by logical > switch/chassis > > > could one option be to allocate a bit in REG10 to mark local ports, > > > and then have a single rule that matches reg10 to drop > output/forwarding > > > of packets? > > > > > > I like the idea... let's see what others say about this, I don't know > how > > strict we want to be consuming > > bits from registers. > > Thanks Miguel for the suggestion :) > > I don't think we need a bit from a register here. Here's my suggested > incremental followed by a full patch. It passes the test, at least. > Will you take a look? > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > index c98b3053130c..2172dd849893 100644 > --- a/ovn/controller/physical.c > +++ b/ovn/controller/physical.c > @@ -293,8 +293,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, >const struct sbrec_port_binding *binding, >const struct sbrec_chassis *chassis, >struct ofpbuf *ofpacts_p, > - struct hmap *flow_table, > - const struct sset *local_lports) > + struct hmap *flow_table) > { > uint32_t dp_key = binding->datapath->tunnel_key; > uint32_t port_key = binding->tunnel_key; > @@ -602,32 +601,6 @@ 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. > - */ > -const char *localport; > -SSET_FOR_EACH (localport, local_lports) { > -/* Iterate over all local logical ports and insert a drop > - * rule with higher priority for every localport in this > - * datapath. */ > -const struct sbrec_port_binding *pb = lport_lookup_by_name( > -lports, localport); > -if (pb && pb->datapath->tunnel_key == dp_key && > -!strcmp(pb->type, "localport")) { > -match_init_catchall(); > -ofpbuf_clear(ofpacts_p); > -/* Match localport logical in_port. */ > -match_set_reg(, MFF_LOG_INPORT - MFF_REG0, > - pb->tunnel_key); > -/* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */ > -match_set_metadata(, htonll(dp_key)); > -match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, > port_key); > -ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, 0, > -, ofpacts_p); > -} > -} > /* Table 32, priority 100. > * === > * > @@ -919,7 +892,7 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) { > consider_port_binding(mff_ovn_geneve, ct_zones, lports, >local_datapaths, binding, chassis, > - , flow_table, local_lports); > + , flow_table); > } > > /* Handle output to multicast groups, in tables 32 and 33. */ > @@ -1016,15 +989,40 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > */ > struct match match; > match_init_catchall(); > -ofpbuf_clear(); > match_set_reg_masked(, MFF_LOG_FLAGS - MFF_REG0, > MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN); > > /* Resubmit to table 33. */ > +ofpbuf_clear(); > put_resubmit(OFTABLE_LOCAL_OUTPUT, ); > ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, 0, >
Re: [ovs-dev] [PATCH v4] OVN localport type support
On Tue, May 23, 2017 at 03:13:08PM +0200, Daniel Alvarez Sanchez wrote: > On Tue, May 23, 2017 at 10:01 AM, Miguel Angel Ajo Pelayo < > majop...@redhat.com> wrote: > > > If we forsee use cases with several local ports by logical switch/chassis > > could one option be to allocate a bit in REG10 to mark local ports, > > and then have a single rule that matches reg10 to drop output/forwarding > > of packets? > > > > I like the idea... let's see what others say about this, I don't know how > strict we want to be consuming > bits from registers. > Thanks Miguel for the suggestion :) I don't think we need a bit from a register here. Here's my suggested incremental followed by a full patch. It passes the test, at least. Will you take a look? diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index c98b3053130c..2172dd849893 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -293,8 +293,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, const struct sbrec_port_binding *binding, const struct sbrec_chassis *chassis, struct ofpbuf *ofpacts_p, - struct hmap *flow_table, - const struct sset *local_lports) + struct hmap *flow_table) { uint32_t dp_key = binding->datapath->tunnel_key; uint32_t port_key = binding->tunnel_key; @@ -602,32 +601,6 @@ 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. - */ -const char *localport; -SSET_FOR_EACH (localport, local_lports) { -/* Iterate over all local logical ports and insert a drop - * rule with higher priority for every localport in this - * datapath. */ -const struct sbrec_port_binding *pb = lport_lookup_by_name( -lports, localport); -if (pb && pb->datapath->tunnel_key == dp_key && -!strcmp(pb->type, "localport")) { -match_init_catchall(); -ofpbuf_clear(ofpacts_p); -/* Match localport logical in_port. */ -match_set_reg(, MFF_LOG_INPORT - MFF_REG0, - pb->tunnel_key); -/* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */ -match_set_metadata(, htonll(dp_key)); -match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, port_key); -ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, 0, -, ofpacts_p); -} -} /* Table 32, priority 100. * === * @@ -919,7 +892,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) { consider_port_binding(mff_ovn_geneve, ct_zones, lports, local_datapaths, binding, chassis, - , flow_table, local_lports); + , flow_table); } /* Handle output to multicast groups, in tables 32 and 33. */ @@ -1016,15 +989,40 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, */ struct match match; match_init_catchall(); -ofpbuf_clear(); match_set_reg_masked(, MFF_LOG_FLAGS - MFF_REG0, MLF_RCV_FROM_VXLAN, MLF_RCV_FROM_VXLAN); /* Resubmit to table 33. */ +ofpbuf_clear(); put_resubmit(OFTABLE_LOCAL_OUTPUT, ); ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, 0, , ); +/* Table 32, priority 150. + * === + * + * Handles packets received from ports of type "localport". These ports + * are present on every hypervisor. Traffic that originates at one should + * never go over a tunnel to a remote hypervisor, so resubmit them to table + * 33 for local delivery. */ +match_init_catchall(); +ofpbuf_clear(); +put_resubmit(OFTABLE_LOCAL_OUTPUT, ); +const char *localport; +SSET_FOR_EACH (localport, local_lports) { +/* Iterate over all local logical ports and insert a drop + * rule with higher priority for every localport in this + * datapath. */ +const struct sbrec_port_binding *pb = lport_lookup_by_name( +lports, localport); +if (pb && !strcmp(pb->type, "localport")) { +match_set_reg(, MFF_LOG_INPORT - MFF_REG0, pb->tunnel_key); +match_set_metadata(, htonll(pb->datapath->tunnel_key)); +ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, 0, +, ); +} +} + /* Table 32, Priority 0. *
Re: [ovs-dev] [PATCH v4] OVN localport type support
On Tue, May 23, 2017 at 10:01 AM, Miguel Angel Ajo Pelayo < majop...@redhat.com> wrote: > If we forsee use cases with several local ports by logical switch/chassis > could one option be to allocate a bit in REG10 to mark local ports, > and then have a single rule that matches reg10 to drop output/forwarding > of packets? > > I like the idea... let's see what others say about this, I don't know how strict we want to be consuming bits from registers. Thanks Miguel for the suggestion :) > Best, > Miguel Ángel Ajo > > > > On Fri, May 19, 2017 at 4:26 PM, Daniel Alvarez Sanchez < > dalva...@redhat.com> wrote: > >> Thanks a lot Ben for your review! >> >> On Fri, May 19, 2017 at 12:16 AM, Ben Pfaffwrote: >> >> > On Wed, May 10, 2017 at 08:35:45AM +, 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]. >> > > >> > > An example 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 in HV1 (OVS port with external-id:iface-id="p1") >> > > - p2 in HV2 (OVS port with external-id:iface-id="p2") >> > > - lp in both hypevisors (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 >> > > >> > > Explicit drop rules are inserted in table 32 with priority 150 >> > > in order to prevent traffic originated at a localport to go over >> > > a tunnel. >> > > >> > > [0] https://review.openstack.org/#/c/452811/ >> > > >> > > Signed-off-by: Daniel Alvarez >> > >> > Thanks for working on this! >> > >> > This seems reasonable, but I'm torn about one aspect of it. I'm not >> > sure whether my concern is a kind of premature optimization or not, so >> > let me just describe it and we can discuss. >> > >> > This adds code that iterates over every local lport (suppose there are N >> > of them), which is nested inside a function that is executed for every >> > port relevant to the local hypervisor (suppose there are M of them). >> > That's O(N*M) time. But the inner loop is only doing something useful >> > for localport logical ports, and normally there would only be 1 or so of >> > those; at any rate a constant number. So in theory this could run in >> > O(M) time. >> > >> > I see at least two ways to fix the problem, if it's a problem, but I >> > don't know whether it's worth fixing. Daniel? Russell? (Others?) >> > >> > Yes, I also thought about this but I don't know either if it's a problem >> or not. >> If we want to impose at most one logical localport per datapath then we >> would have O(M) time (mimic localnet ports) but that's something I didn't >> want to do unless you think it makes more sense. >> >> >> > Thanks, >> > >> > Ben. >> > >> ___ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] OVN localport type support
If we forsee use cases with several local ports by logical switch/chassis could one option be to allocate a bit in REG10 to mark local ports, and then have a single rule that matches reg10 to drop output/forwarding of packets? Best, Miguel Ángel Ajo On Fri, May 19, 2017 at 4:26 PM, Daniel Alvarez Sanchezwrote: > Thanks a lot Ben for your review! > > On Fri, May 19, 2017 at 12:16 AM, Ben Pfaff wrote: > > > On Wed, May 10, 2017 at 08:35:45AM +, 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]. > > > > > > An example 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 in HV1 (OVS port with external-id:iface-id="p1") > > > - p2 in HV2 (OVS port with external-id:iface-id="p2") > > > - lp in both hypevisors (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 > > > > > > Explicit drop rules are inserted in table 32 with priority 150 > > > in order to prevent traffic originated at a localport to go over > > > a tunnel. > > > > > > [0] https://review.openstack.org/#/c/452811/ > > > > > > Signed-off-by: Daniel Alvarez > > > > Thanks for working on this! > > > > This seems reasonable, but I'm torn about one aspect of it. I'm not > > sure whether my concern is a kind of premature optimization or not, so > > let me just describe it and we can discuss. > > > > This adds code that iterates over every local lport (suppose there are N > > of them), which is nested inside a function that is executed for every > > port relevant to the local hypervisor (suppose there are M of them). > > That's O(N*M) time. But the inner loop is only doing something useful > > for localport logical ports, and normally there would only be 1 or so of > > those; at any rate a constant number. So in theory this could run in > > O(M) time. > > > > I see at least two ways to fix the problem, if it's a problem, but I > > don't know whether it's worth fixing. Daniel? Russell? (Others?) > > > > Yes, I also thought about this but I don't know either if it's a problem > or not. > If we want to impose at most one logical localport per datapath then we > would have O(M) time (mimic localnet ports) but that's something I didn't > want to do unless you think it makes more sense. > > > > Thanks, > > > > Ben. > > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] OVN localport type support
On Wed, May 10, 2017 at 08:35:45AM +, 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]. > > An example 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 in HV1 (OVS port with external-id:iface-id="p1") > - p2 in HV2 (OVS port with external-id:iface-id="p2") > - lp in both hypevisors (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 > > Explicit drop rules are inserted in table 32 with priority 150 > in order to prevent traffic originated at a localport to go over > a tunnel. > > [0] https://review.openstack.org/#/c/452811/ > > Signed-off-by: Daniel AlvarezThanks for working on this! This seems reasonable, but I'm torn about one aspect of it. I'm not sure whether my concern is a kind of premature optimization or not, so let me just describe it and we can discuss. This adds code that iterates over every local lport (suppose there are N of them), which is nested inside a function that is executed for every port relevant to the local hypervisor (suppose there are M of them). That's O(N*M) time. But the inner loop is only doing something useful for localport logical ports, and normally there would only be 1 or so of those; at any rate a constant number. So in theory this could run in O(M) time. I see at least two ways to fix the problem, if it's a problem, but I don't know whether it's worth fixing. Daniel? Russell? (Others?) Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4] OVN localport type support
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]. An example 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 in HV1 (OVS port with external-id:iface-id="p1") - p2 in HV2 (OVS port with external-id:iface-id="p2") - lp in both hypevisors (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 Explicit drop rules are inserted in table 32 with priority 150 in order to prevent traffic originated at a localport to go over a tunnel. [0] https://review.openstack.org/#/c/452811/ Signed-off-by: Daniel Alvarez--- ovn/controller/binding.c| 3 +- ovn/controller/ovn-controller.c | 2 +- ovn/controller/physical.c | 34 ++- ovn/controller/physical.h | 4 +- ovn/northd/ovn-northd.8.xml | 8 +-- ovn/northd/ovn-northd.c | 6 +- ovn/ovn-architecture.7.xml | 25 ++-- ovn/ovn-nb.xml | 9 +++ ovn/ovn-sb.xml | 14 + tests/ovn.at| 122 10 files changed, 210 insertions(+), 17 deletions(-) diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index 95e9deb..83a7543 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -380,7 +380,8 @@ consider_local_datapath(struct controller_ctx *ctx, if (iface_rec && qos_map && ctx->ovs_idl_txn) { get_qos_params(binding_rec, qos_map); } -our_chassis = true; + if(strcmp(binding_rec->type, "localport")) +our_chassis = true; } else if (!strcmp(binding_rec->type, "l2gateway")) { const char *chassis_id = smap_get(_rec->options, "l2gateway-chassis"); diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index f22551d..0f4dd35 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -655,7 +655,7 @@ main(int argc, char *argv[]) physical_run(, mff_ovn_geneve, br_int, chassis, _zones, , - _table, _datapaths); + _table, _datapaths, _lports); ofctrl_put(_table, _ct_zones, get_nb_cfg(ctx.ovnsb_idl)); diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index 457fc45..c98b305 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -293,7 +293,8 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, const struct sbrec_port_binding *binding, const struct sbrec_chassis *chassis, struct ofpbuf *ofpacts_p, - struct hmap *flow_table) + struct hmap *flow_table, + const struct sset *local_lports) { uint32_t dp_key = binding->datapath->tunnel_key; uint32_t port_key = binding->tunnel_key; @@ -601,6 +602,32 @@ 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. + */ +const char *localport; +SSET_FOR_EACH (localport, local_lports) { +/* Iterate over all local logical ports and insert a drop + * rule with higher priority for every localport in this + * datapath. */ +const struct sbrec_port_binding *pb = lport_lookup_by_name( +lports, localport); +if (pb && pb->datapath->tunnel_key == dp_key && +!strcmp(pb->type, "localport")) { +match_init_catchall(); +ofpbuf_clear(ofpacts_p); +/* Match localport logical in_port. */ +match_set_reg(, MFF_LOG_INPORT - MFF_REG0, + pb->tunnel_key); +/* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */ +match_set_metadata(, htonll(dp_key)); +match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, port_key); +ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, 0, +, ofpacts_p); +} +} /* Table 32, priority 100.