Re: [ovs-dev] [PATCH] ovn-controller: Inject GARPs to logical switch pipeline to update neighbors
It makes sense to update the test to check for 2 packets. Kindly update it
with this patch.
Thanks
Anil
On Tue, Dec 4, 2018 at 8:26 AM Han Zhou wrote:
> On Mon, Dec 3, 2018 at 9:54 AM Daniel Alvarez wrote:
> >
> > Prior to this patch, GARPs announcing NAT addresses or new VIFs
> > were sent out to localnet ofport through an output action.
> > This can lead to problems since local datapaths won't get those
> > GARPs and ovn-controller won't update MAC_Binding entries (as
> > upstream switch will not send back the GARP to this port hence
> > other logical routers won't update their neighbours).
> >
> > This patch is changing the behavior so that GARPs get injected
> > to OVN pipeline of the external switch. This way, they'll get
> > broadcasted to local pipelines and also sent out to the external
> > network through the localnet port.
> >
> > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047604.html
> > Signed-off-by: Daniel Alvarez
> > ---
> > ovn/controller/pinctrl.c | 62 ++--
> > tests/ovn.at | 100 +++
> > 2 files changed, 125 insertions(+), 37 deletions(-)
> >
> > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> > index 56539a891..3e8af41cc 100644
> > --- a/ovn/controller/pinctrl.c
> > +++ b/ovn/controller/pinctrl.c
> > @@ -2019,8 +2019,8 @@ struct garp_data {
> > ovs_be32 ipv4; /* Ipv4 address of port. */
> > long long int announce_time; /* Next announcement in ms. */
> > int backoff; /* Backoff for the next announcement.
> */
> > -ofp_port_t ofport; /* ofport used to output this GARP. */
> > -int tag; /* VLAN tag of this GARP packet, or -1.
> */
> > +uint32_t dp_key; /* Datapath used to output this GARP.
> */
> > +uint32_t port_key; /* Port to inject the GARP into. */
> > };
> >
> > /* Contains GARPs to be sent. */
> > @@ -2043,37 +2043,24 @@ destroy_send_garps(void)
> > }
> >
> > static void
> > -add_garp(const char *name, ofp_port_t ofport, int tag,
> > - const struct eth_addr ea, ovs_be32 ip)
> > +add_garp(const char *name, const struct eth_addr ea, ovs_be32 ip,
> > + uint32_t dp_key, uint32_t port_key)
> > {
> > struct garp_data *garp = xmalloc(sizeof *garp);
> > garp->ea = ea;
> > garp->ipv4 = ip;
> > garp->announce_time = time_msec() + 1000;
> > garp->backoff = 1;
> > -garp->ofport = ofport;
> > -garp->tag = tag;
> > +garp->dp_key = dp_key;
> > +garp->port_key = port_key;
> > shash_add(_garp_data, name, garp);
> > }
> >
> > /* Add or update a vif for which GARPs need to be announced. */
> > static void
> > send_garp_update(const struct sbrec_port_binding *binding_rec,
> > - struct simap *localnet_ofports,
>
> Since the localnet_ofports is not needed any more, the variable should be
> removed from the function send_garp_run() as well.
>
> > - const struct hmap *local_datapaths,
> > struct shash *nat_addresses)
> > {
> > -/* Find the localnet ofport to send this GARP. */
> > -struct local_datapath *ld
> > -= get_local_datapath(local_datapaths,
> > - binding_rec->datapath->tunnel_key);
> > -if (!ld || !ld->localnet_port) {
> > -return;
> > -}
> > -ofp_port_t ofport = u16_to_ofp(simap_get(localnet_ofports,
> > -
> ld->localnet_port->logical_port));
> > -int tag = ld->localnet_port->n_tag ? *ld->localnet_port->tag : -1;
> > -
> > volatile struct garp_data *garp = NULL;
> > /* Update GARP for NAT IP if it exists. Consider port bindings with
> type
> > * "l3gateway" for logical switch ports attached to gateway routers,
> and
> > @@ -2090,11 +2077,13 @@ send_garp_update(const struct sbrec_port_binding
> *binding_rec,
> >
> laddrs->ipv4_addrs[i].addr_s);
> > garp = shash_find_data(_garp_data, name);
> > if (garp) {
> > -garp->ofport = ofport;
> > -garp->tag = tag;
> > +garp->dp_key = binding_rec->datapath->tunnel_key;
> > +garp->port_key = binding_rec->tunnel_key;
> > } else {
> > -add_garp(name, ofport, tag, laddrs->ea,
> > - laddrs->ipv4_addrs[i].addr);
> > +add_garp(name, laddrs->ea,
> > + laddrs->ipv4_addrs[i].addr,
> > + binding_rec->datapath->tunnel_key,
> > + binding_rec->tunnel_key);
> > }
> > free(name);
> > }
> > @@ -2107,7 +2096,8 @@ send_garp_update(const struct sbrec_port_binding
> *binding_rec,
> > /* Update GARP for vif if it exists. */
> > garp = shash_find_data(_garp_data, binding_rec->logical_port);
> > if
Re: [ovs-dev] [PATCH] ovn-controller: Inject GARPs to logical switch pipeline to update neighbors
On Mon, Dec 3, 2018 at 9:54 AM Daniel Alvarez wrote:
>
> Prior to this patch, GARPs announcing NAT addresses or new VIFs
> were sent out to localnet ofport through an output action.
> This can lead to problems since local datapaths won't get those
> GARPs and ovn-controller won't update MAC_Binding entries (as
> upstream switch will not send back the GARP to this port hence
> other logical routers won't update their neighbours).
>
> This patch is changing the behavior so that GARPs get injected
> to OVN pipeline of the external switch. This way, they'll get
> broadcasted to local pipelines and also sent out to the external
> network through the localnet port.
>
> Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047604.html
> Signed-off-by: Daniel Alvarez
> ---
> ovn/controller/pinctrl.c | 62 ++--
> tests/ovn.at | 100 +++
> 2 files changed, 125 insertions(+), 37 deletions(-)
>
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 56539a891..3e8af41cc 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -2019,8 +2019,8 @@ struct garp_data {
> ovs_be32 ipv4; /* Ipv4 address of port. */
> long long int announce_time; /* Next announcement in ms. */
> int backoff; /* Backoff for the next announcement. */
> -ofp_port_t ofport; /* ofport used to output this GARP. */
> -int tag; /* VLAN tag of this GARP packet, or -1.
*/
> +uint32_t dp_key; /* Datapath used to output this GARP. */
> +uint32_t port_key; /* Port to inject the GARP into. */
> };
>
> /* Contains GARPs to be sent. */
> @@ -2043,37 +2043,24 @@ destroy_send_garps(void)
> }
>
> static void
> -add_garp(const char *name, ofp_port_t ofport, int tag,
> - const struct eth_addr ea, ovs_be32 ip)
> +add_garp(const char *name, const struct eth_addr ea, ovs_be32 ip,
> + uint32_t dp_key, uint32_t port_key)
> {
> struct garp_data *garp = xmalloc(sizeof *garp);
> garp->ea = ea;
> garp->ipv4 = ip;
> garp->announce_time = time_msec() + 1000;
> garp->backoff = 1;
> -garp->ofport = ofport;
> -garp->tag = tag;
> +garp->dp_key = dp_key;
> +garp->port_key = port_key;
> shash_add(_garp_data, name, garp);
> }
>
> /* Add or update a vif for which GARPs need to be announced. */
> static void
> send_garp_update(const struct sbrec_port_binding *binding_rec,
> - struct simap *localnet_ofports,
Since the localnet_ofports is not needed any more, the variable should be
removed from the function send_garp_run() as well.
> - const struct hmap *local_datapaths,
> struct shash *nat_addresses)
> {
> -/* Find the localnet ofport to send this GARP. */
> -struct local_datapath *ld
> -= get_local_datapath(local_datapaths,
> - binding_rec->datapath->tunnel_key);
> -if (!ld || !ld->localnet_port) {
> -return;
> -}
> -ofp_port_t ofport = u16_to_ofp(simap_get(localnet_ofports,
> -
ld->localnet_port->logical_port));
> -int tag = ld->localnet_port->n_tag ? *ld->localnet_port->tag : -1;
> -
> volatile struct garp_data *garp = NULL;
> /* Update GARP for NAT IP if it exists. Consider port bindings with
type
> * "l3gateway" for logical switch ports attached to gateway routers,
and
> @@ -2090,11 +2077,13 @@ send_garp_update(const struct sbrec_port_binding
*binding_rec,
>
laddrs->ipv4_addrs[i].addr_s);
> garp = shash_find_data(_garp_data, name);
> if (garp) {
> -garp->ofport = ofport;
> -garp->tag = tag;
> +garp->dp_key = binding_rec->datapath->tunnel_key;
> +garp->port_key = binding_rec->tunnel_key;
> } else {
> -add_garp(name, ofport, tag, laddrs->ea,
> - laddrs->ipv4_addrs[i].addr);
> +add_garp(name, laddrs->ea,
> + laddrs->ipv4_addrs[i].addr,
> + binding_rec->datapath->tunnel_key,
> + binding_rec->tunnel_key);
> }
> free(name);
> }
> @@ -2107,7 +2096,8 @@ send_garp_update(const struct sbrec_port_binding
*binding_rec,
> /* Update GARP for vif if it exists. */
> garp = shash_find_data(_garp_data, binding_rec->logical_port);
> if (garp) {
> -garp->ofport = ofport;
> +garp->dp_key = binding_rec->datapath->tunnel_key;
> +garp->port_key = binding_rec->tunnel_key;
> return;
> }
>
> @@ -2120,8 +2110,9 @@ send_garp_update(const struct sbrec_port_binding
*binding_rec,
> continue;
> }
>
> -add_garp(binding_rec->logical_port, ofport, tag,
> -
Re: [ovs-dev] [PATCH] ovn-controller: Inject GARPs to logical switch pipeline to update neighbors
This patch is making test "ovn -- vlan traffic for external network
with distributed router gateway port" fail at [0].
The reason seems to be that now we're sending GARPs so n_packets is
not 0 anymore for the flow that the test is checking:
./ovn.at:8816: as hv1 ovs-ofctl dump-flows br-int table=65 | grep
"priority=100,reg15=0x1,metadata=0x2" \
| grep actions=clone | grep -v n_packets=0 | wc -l
cookie=0x0, duration=6.454s, table=65, n_packets=2, n_bytes=84,
idle_age=3, priority=100,reg15=0x1,metadata=0x2
actions=clone(ct_clear,load:0->NXM_NX_REG11[],load:0->NXM_NX_REG12[],load:0->NXM_NX_REG13[],load:0x2->NXM_NX_REG11[],load:0x7->NXM_NX_REG12[],load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[],load:0->NXM_NX_REG10[],load:0->NXM_NX_REG15[],load:0->NXM_NX_REG0[],load:0->NXM_NX_REG1[],load:0->NXM_NX_REG2[],load:0->NXM_NX_REG3[],load:0->NXM_NX_REG4[],load:0->NXM_NX_REG5[],load:0->NXM_NX_REG6[],load:0->NXM_NX_REG7[],load:0->NXM_NX_REG8[],load:0->NXM_NX_REG9[],load:0->NXM_OF_IN_PORT[],resubmit(,8))
As you can see n_packets is 2 at this point. I'm waiting for inputs
here from authors of the patch [1] (Numan, Anil and Guru) or other
folks. This is the only test failing and if we would expect 2 packets
instead of comment out that particular line, the rest of the test
passes.
Thanks a lot!
Daniel
[0] https://github.com/openvswitch/ovs/blob/master/tests/ovn.at#L8816
[1]
https://github.com/openvswitch/ovs/commit/85706c34d53d4810f54bec1de662392a3c06a996
On Mon, Dec 3, 2018 at 6:54 PM Daniel Alvarez wrote:
>
> Prior to this patch, GARPs announcing NAT addresses or new VIFs
> were sent out to localnet ofport through an output action.
> This can lead to problems since local datapaths won't get those
> GARPs and ovn-controller won't update MAC_Binding entries (as
> upstream switch will not send back the GARP to this port hence
> other logical routers won't update their neighbours).
>
> This patch is changing the behavior so that GARPs get injected
> to OVN pipeline of the external switch. This way, they'll get
> broadcasted to local pipelines and also sent out to the external
> network through the localnet port.
>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047604.html
> Signed-off-by: Daniel Alvarez
> ---
> ovn/controller/pinctrl.c | 62 ++--
> tests/ovn.at | 100 +++
> 2 files changed, 125 insertions(+), 37 deletions(-)
>
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 56539a891..3e8af41cc 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -2019,8 +2019,8 @@ struct garp_data {
> ovs_be32 ipv4; /* Ipv4 address of port. */
> long long int announce_time; /* Next announcement in ms. */
> int backoff; /* Backoff for the next announcement. */
> -ofp_port_t ofport; /* ofport used to output this GARP. */
> -int tag; /* VLAN tag of this GARP packet, or -1. */
> +uint32_t dp_key; /* Datapath used to output this GARP. */
> +uint32_t port_key; /* Port to inject the GARP into. */
> };
>
> /* Contains GARPs to be sent. */
> @@ -2043,37 +2043,24 @@ destroy_send_garps(void)
> }
>
> static void
> -add_garp(const char *name, ofp_port_t ofport, int tag,
> - const struct eth_addr ea, ovs_be32 ip)
> +add_garp(const char *name, const struct eth_addr ea, ovs_be32 ip,
> + uint32_t dp_key, uint32_t port_key)
> {
> struct garp_data *garp = xmalloc(sizeof *garp);
> garp->ea = ea;
> garp->ipv4 = ip;
> garp->announce_time = time_msec() + 1000;
> garp->backoff = 1;
> -garp->ofport = ofport;
> -garp->tag = tag;
> +garp->dp_key = dp_key;
> +garp->port_key = port_key;
> shash_add(_garp_data, name, garp);
> }
>
> /* Add or update a vif for which GARPs need to be announced. */
> static void
> send_garp_update(const struct sbrec_port_binding *binding_rec,
> - struct simap *localnet_ofports,
> - const struct hmap *local_datapaths,
> struct shash *nat_addresses)
> {
> -/* Find the localnet ofport to send this GARP. */
> -struct local_datapath *ld
> -= get_local_datapath(local_datapaths,
> - binding_rec->datapath->tunnel_key);
> -if (!ld || !ld->localnet_port) {
> -return;
> -}
> -ofp_port_t ofport = u16_to_ofp(simap_get(localnet_ofports,
> -
> ld->localnet_port->logical_port));
> -int tag = ld->localnet_port->n_tag ? *ld->localnet_port->tag : -1;
> -
> volatile struct garp_data *garp = NULL;
> /* Update GARP for NAT IP if it exists. Consider port bindings with type
> * "l3gateway" for logical switch ports attached to gateway routers, and
> @@ -2090,11 +2077,13 @@ send_garp_update(const struct sbrec_port_binding
> *binding_rec,
>
[ovs-dev] [PATCH] ovn-controller: Inject GARPs to logical switch pipeline to update neighbors
Prior to this patch, GARPs announcing NAT addresses or new VIFs
were sent out to localnet ofport through an output action.
This can lead to problems since local datapaths won't get those
GARPs and ovn-controller won't update MAC_Binding entries (as
upstream switch will not send back the GARP to this port hence
other logical routers won't update their neighbours).
This patch is changing the behavior so that GARPs get injected
to OVN pipeline of the external switch. This way, they'll get
broadcasted to local pipelines and also sent out to the external
network through the localnet port.
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047604.html
Signed-off-by: Daniel Alvarez
---
ovn/controller/pinctrl.c | 62 ++--
tests/ovn.at | 100 +++
2 files changed, 125 insertions(+), 37 deletions(-)
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 56539a891..3e8af41cc 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -2019,8 +2019,8 @@ struct garp_data {
ovs_be32 ipv4; /* Ipv4 address of port. */
long long int announce_time; /* Next announcement in ms. */
int backoff; /* Backoff for the next announcement. */
-ofp_port_t ofport; /* ofport used to output this GARP. */
-int tag; /* VLAN tag of this GARP packet, or -1. */
+uint32_t dp_key; /* Datapath used to output this GARP. */
+uint32_t port_key; /* Port to inject the GARP into. */
};
/* Contains GARPs to be sent. */
@@ -2043,37 +2043,24 @@ destroy_send_garps(void)
}
static void
-add_garp(const char *name, ofp_port_t ofport, int tag,
- const struct eth_addr ea, ovs_be32 ip)
+add_garp(const char *name, const struct eth_addr ea, ovs_be32 ip,
+ uint32_t dp_key, uint32_t port_key)
{
struct garp_data *garp = xmalloc(sizeof *garp);
garp->ea = ea;
garp->ipv4 = ip;
garp->announce_time = time_msec() + 1000;
garp->backoff = 1;
-garp->ofport = ofport;
-garp->tag = tag;
+garp->dp_key = dp_key;
+garp->port_key = port_key;
shash_add(_garp_data, name, garp);
}
/* Add or update a vif for which GARPs need to be announced. */
static void
send_garp_update(const struct sbrec_port_binding *binding_rec,
- struct simap *localnet_ofports,
- const struct hmap *local_datapaths,
struct shash *nat_addresses)
{
-/* Find the localnet ofport to send this GARP. */
-struct local_datapath *ld
-= get_local_datapath(local_datapaths,
- binding_rec->datapath->tunnel_key);
-if (!ld || !ld->localnet_port) {
-return;
-}
-ofp_port_t ofport = u16_to_ofp(simap_get(localnet_ofports,
- ld->localnet_port->logical_port));
-int tag = ld->localnet_port->n_tag ? *ld->localnet_port->tag : -1;
-
volatile struct garp_data *garp = NULL;
/* Update GARP for NAT IP if it exists. Consider port bindings with type
* "l3gateway" for logical switch ports attached to gateway routers, and
@@ -2090,11 +2077,13 @@ send_garp_update(const struct sbrec_port_binding
*binding_rec,
laddrs->ipv4_addrs[i].addr_s);
garp = shash_find_data(_garp_data, name);
if (garp) {
-garp->ofport = ofport;
-garp->tag = tag;
+garp->dp_key = binding_rec->datapath->tunnel_key;
+garp->port_key = binding_rec->tunnel_key;
} else {
-add_garp(name, ofport, tag, laddrs->ea,
- laddrs->ipv4_addrs[i].addr);
+add_garp(name, laddrs->ea,
+ laddrs->ipv4_addrs[i].addr,
+ binding_rec->datapath->tunnel_key,
+ binding_rec->tunnel_key);
}
free(name);
}
@@ -2107,7 +2096,8 @@ send_garp_update(const struct sbrec_port_binding
*binding_rec,
/* Update GARP for vif if it exists. */
garp = shash_find_data(_garp_data, binding_rec->logical_port);
if (garp) {
-garp->ofport = ofport;
+garp->dp_key = binding_rec->datapath->tunnel_key;
+garp->port_key = binding_rec->tunnel_key;
return;
}
@@ -2120,8 +2110,9 @@ send_garp_update(const struct sbrec_port_binding
*binding_rec,
continue;
}
-add_garp(binding_rec->logical_port, ofport, tag,
- laddrs.ea, laddrs.ipv4_addrs[0].addr);
+add_garp(binding_rec->logical_port,
+ laddrs.ea, laddrs.ipv4_addrs[0].addr,
+ binding_rec->datapath->tunnel_key, binding_rec->tunnel_key);
destroy_lport_addresses();
break;
@@ -2150,16
