Re: [ovs-dev] [PATCH v4] OVN localport type support

2017-05-26 Thread Daniel Alvarez Sanchez
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 Pfaff  wrote:

> 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

2017-05-25 Thread Ben Pfaff
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

2017-05-23 Thread Daniel Alvarez Sanchez
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 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

2017-05-23 Thread Miguel Angel Ajo Pelayo
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 Sanchez  wrote:

> 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

2017-05-18 Thread Ben Pfaff
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?)

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH v4] OVN localport type support

2017-05-10 Thread Daniel Alvarez
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.