Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Fix port group I-P when they contain non-vif ports.
On Thu, Jul 1, 2021 at 10:18 AM Dumitru Ceara wrote: > > On 7/1/21 6:39 PM, Han Zhou wrote: > > On Thu, Jul 1, 2021 at 6:05 AM Dumitru Ceara wrote: > >> > >> On 7/1/21 2:53 PM, Numan Siddique wrote: > Thanks Dumitru! > Acked-by: Han Zhou > > Not sure if Numan would like to take a second look as well, so let's > > wait > for one or two days before merging. > >>> Thanks Dumitru and Han. > >>> > >>> I looked at it and LGTM. I applied the patch to the main branch. > >>> > >>> Numan > >>> > >> > >> Thanks! > >> > > > > Do we need this in branch-21.06 and 21.03? > > > > Right, that would be great, thanks! > Ok, I rebased and pushed to 21.06 and 21.03. Thanks, Han ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Fix port group I-P when they contain non-vif ports.
On 7/1/21 6:39 PM, Han Zhou wrote: > On Thu, Jul 1, 2021 at 6:05 AM Dumitru Ceara wrote: >> >> On 7/1/21 2:53 PM, Numan Siddique wrote: Thanks Dumitru! Acked-by: Han Zhou Not sure if Numan would like to take a second look as well, so let's > wait for one or two days before merging. >>> Thanks Dumitru and Han. >>> >>> I looked at it and LGTM. I applied the patch to the main branch. >>> >>> Numan >>> >> >> Thanks! >> > > Do we need this in branch-21.06 and 21.03? > Right, that would be great, thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Fix port group I-P when they contain non-vif ports.
On Thu, Jul 1, 2021 at 6:05 AM Dumitru Ceara wrote: > > On 7/1/21 2:53 PM, Numan Siddique wrote: > >> Thanks Dumitru! > >> Acked-by: Han Zhou > >> > >> Not sure if Numan would like to take a second look as well, so let's wait > >> for one or two days before merging. > > Thanks Dumitru and Han. > > > > I looked at it and LGTM. I applied the patch to the main branch. > > > > Numan > > > > Thanks! > Do we need this in branch-21.06 and 21.03? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Fix port group I-P when they contain non-vif ports.
On 7/1/21 2:53 PM, Numan Siddique wrote: >> Thanks Dumitru! >> Acked-by: Han Zhou >> >> Not sure if Numan would like to take a second look as well, so let's wait >> for one or two days before merging. > Thanks Dumitru and Han. > > I looked at it and LGTM. I applied the patch to the main branch. > > Numan > Thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Fix port group I-P when they contain non-vif ports.
On Wed, Jun 30, 2021 at 7:04 PM Han Zhou wrote:
>
> On Wed, Jun 30, 2021 at 7:01 AM Dumitru Ceara wrote:
> >
> > It's valid that port_groups contain non-vif ports, they can actually
> > contain any type of logical_switch_port.
> >
> > Also, there's no need to allocate a new sset containing the local ports'
> > names every time the I-P engine processes a change. We were already
> > maintaining a set of "local_lport_ids". These correspond to port
> > bindings that are relevant locally (including non-vif ports). Extend
> > it to include the locally relevant lport names too and rename the
> > structure an its helper functions to related_lport*.
> >
> > Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163
> > Reported-by: Antonio Ojea
> > Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow
> explosion problem.")
> > Signed-off-by: Dumitru Ceara
> > ---
> > v2:
> > - Addressed Numan's and Han's comments:
> > - add struct related_lports
> > - add test case.
> > ---
> > controller/binding.c| 79 ++---
> > controller/binding.h| 31 ---
> > controller/lflow.c | 2 +-
> > controller/lflow.h | 2 +-
> > controller/ovn-controller.c | 48 +-
> > tests/ovn.at| 44 +
> > 6 files changed, 120 insertions(+), 86 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 7fde0fdbb..594babc98 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -531,38 +531,41 @@ remove_local_lports(const char *iface_id, struct
> binding_ctx_out *b_ctx)
> > }
> > }
> >
> > -/* Add a port binding ID (of the form "dp-key"_"port-key") to the set of
> local
> > - * lport IDs. Also track if the set has changed.
> > +/* Add a port binding to the set of locally relevant lports.
> > + * Also track if the set has changed.
> > */
> > static void
> > -update_local_lport_ids(const struct sbrec_port_binding *pb,
> > - struct binding_ctx_out *b_ctx)
> > +update_related_lport(const struct sbrec_port_binding *pb,
> > + struct binding_ctx_out *b_ctx)
> > {
> > char buf[16];
> > get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key,
> > buf, sizeof(buf));
> > -if (sset_add(b_ctx->local_lport_ids, buf) != NULL) {
> > -b_ctx->local_lport_ids_changed = true;
> > +if (sset_add(_ctx->related_lports->lport_ids, buf) != NULL) {
> > +b_ctx->related_lports_changed = true;
> >
> > if (b_ctx->tracked_dp_bindings) {
> > /* Add the 'pb' to the tracked_datapaths. */
> > tracked_binding_datapath_lport_add(pb,
> b_ctx->tracked_dp_bindings);
> > }
> > }
> > +sset_add(_ctx->related_lports->lport_names, pb->logical_port);
> > }
> >
> > -/* Remove a port binding id from the set of local lport IDs. Also track
> if
> > - * the set has changed.
> > +/* Remove a port binding id from the set of locally relevant lports.
> > + * Also track if the set has changed.
> > */
> > static void
> > -remove_local_lport_ids(const struct sbrec_port_binding *pb,
> > - struct binding_ctx_out *b_ctx)
> > +remove_related_lport(const struct sbrec_port_binding *pb,
> > + struct binding_ctx_out *b_ctx)
> > {
> > char buf[16];
> > get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key,
> > buf, sizeof(buf));
> > -if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) {
> > -b_ctx->local_lport_ids_changed = true;
> > +sset_find_and_delete(_ctx->related_lports->lport_names,
> > + pb->logical_port);
> > +if (sset_find_and_delete(_ctx->related_lports->lport_ids, buf)) {
> > +b_ctx->related_lports_changed = true;
> >
> > if (b_ctx->tracked_dp_bindings) {
> > /* Add the 'pb' to the tracked_datapaths. */
> > @@ -678,6 +681,20 @@ static struct binding_lport
> *binding_lport_check_and_cleanup(
> >
> > static char *get_lport_type_str(enum en_lport_type lport_type);
> >
> > +void
> > +related_lports_init(struct related_lports *rp)
> > +{
> > +sset_init(>lport_names);
> > +sset_init(>lport_ids);
> > +}
> > +
> > +void
> > +related_lports_destroy(struct related_lports *rp)
> > +{
> > +sset_destroy(>lport_names);
> > +sset_destroy(>lport_ids);
> > +}
> > +
> > void
> > local_binding_data_init(struct local_binding_data *lbinding_data)
> > {
> > @@ -1172,7 +1189,7 @@ release_binding_lport(const struct sbrec_chassis
> *chassis_rec,
> >struct binding_ctx_out *b_ctx_out)
> > {
> > if (is_binding_lport_this_chassis(b_lport, chassis_rec)) {
> > -remove_local_lport_ids(b_lport->pb, b_ctx_out);
> > +remove_related_lport(b_lport->pb, b_ctx_out);
> > if (!release_lport(b_lport->pb,
Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Fix port group I-P when they contain non-vif ports.
On Wed, Jun 30, 2021 at 7:01 AM Dumitru Ceara wrote:
>
> It's valid that port_groups contain non-vif ports, they can actually
> contain any type of logical_switch_port.
>
> Also, there's no need to allocate a new sset containing the local ports'
> names every time the I-P engine processes a change. We were already
> maintaining a set of "local_lport_ids". These correspond to port
> bindings that are relevant locally (including non-vif ports). Extend
> it to include the locally relevant lport names too and rename the
> structure an its helper functions to related_lport*.
>
> Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163
> Reported-by: Antonio Ojea
> Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow
explosion problem.")
> Signed-off-by: Dumitru Ceara
> ---
> v2:
> - Addressed Numan's and Han's comments:
> - add struct related_lports
> - add test case.
> ---
> controller/binding.c| 79 ++---
> controller/binding.h| 31 ---
> controller/lflow.c | 2 +-
> controller/lflow.h | 2 +-
> controller/ovn-controller.c | 48 +-
> tests/ovn.at| 44 +
> 6 files changed, 120 insertions(+), 86 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 7fde0fdbb..594babc98 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -531,38 +531,41 @@ remove_local_lports(const char *iface_id, struct
binding_ctx_out *b_ctx)
> }
> }
>
> -/* Add a port binding ID (of the form "dp-key"_"port-key") to the set of
local
> - * lport IDs. Also track if the set has changed.
> +/* Add a port binding to the set of locally relevant lports.
> + * Also track if the set has changed.
> */
> static void
> -update_local_lport_ids(const struct sbrec_port_binding *pb,
> - struct binding_ctx_out *b_ctx)
> +update_related_lport(const struct sbrec_port_binding *pb,
> + struct binding_ctx_out *b_ctx)
> {
> char buf[16];
> get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key,
> buf, sizeof(buf));
> -if (sset_add(b_ctx->local_lport_ids, buf) != NULL) {
> -b_ctx->local_lport_ids_changed = true;
> +if (sset_add(_ctx->related_lports->lport_ids, buf) != NULL) {
> +b_ctx->related_lports_changed = true;
>
> if (b_ctx->tracked_dp_bindings) {
> /* Add the 'pb' to the tracked_datapaths. */
> tracked_binding_datapath_lport_add(pb,
b_ctx->tracked_dp_bindings);
> }
> }
> +sset_add(_ctx->related_lports->lport_names, pb->logical_port);
> }
>
> -/* Remove a port binding id from the set of local lport IDs. Also track
if
> - * the set has changed.
> +/* Remove a port binding id from the set of locally relevant lports.
> + * Also track if the set has changed.
> */
> static void
> -remove_local_lport_ids(const struct sbrec_port_binding *pb,
> - struct binding_ctx_out *b_ctx)
> +remove_related_lport(const struct sbrec_port_binding *pb,
> + struct binding_ctx_out *b_ctx)
> {
> char buf[16];
> get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key,
> buf, sizeof(buf));
> -if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) {
> -b_ctx->local_lport_ids_changed = true;
> +sset_find_and_delete(_ctx->related_lports->lport_names,
> + pb->logical_port);
> +if (sset_find_and_delete(_ctx->related_lports->lport_ids, buf)) {
> +b_ctx->related_lports_changed = true;
>
> if (b_ctx->tracked_dp_bindings) {
> /* Add the 'pb' to the tracked_datapaths. */
> @@ -678,6 +681,20 @@ static struct binding_lport
*binding_lport_check_and_cleanup(
>
> static char *get_lport_type_str(enum en_lport_type lport_type);
>
> +void
> +related_lports_init(struct related_lports *rp)
> +{
> +sset_init(>lport_names);
> +sset_init(>lport_ids);
> +}
> +
> +void
> +related_lports_destroy(struct related_lports *rp)
> +{
> +sset_destroy(>lport_names);
> +sset_destroy(>lport_ids);
> +}
> +
> void
> local_binding_data_init(struct local_binding_data *lbinding_data)
> {
> @@ -1172,7 +1189,7 @@ release_binding_lport(const struct sbrec_chassis
*chassis_rec,
>struct binding_ctx_out *b_ctx_out)
> {
> if (is_binding_lport_this_chassis(b_lport, chassis_rec)) {
> -remove_local_lport_ids(b_lport->pb, b_ctx_out);
> +remove_related_lport(b_lport->pb, b_ctx_out);
> if (!release_lport(b_lport->pb, sb_readonly,
> b_ctx_out->tracked_dp_bindings,
> b_ctx_out->if_mgr)) {
> @@ -1214,7 +1231,7 @@ consider_vif_lport_(const struct sbrec_port_binding
*pb,
> pb->datapath, false,
>
[ovs-dev] [PATCH ovn v2] ovn-controller: Fix port group I-P when they contain non-vif ports.
It's valid that port_groups contain non-vif ports, they can actually
contain any type of logical_switch_port.
Also, there's no need to allocate a new sset containing the local ports'
names every time the I-P engine processes a change. We were already
maintaining a set of "local_lport_ids". These correspond to port
bindings that are relevant locally (including non-vif ports). Extend
it to include the locally relevant lport names too and rename the
structure an its helper functions to related_lport*.
Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163
Reported-by: Antonio Ojea
Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion
problem.")
Signed-off-by: Dumitru Ceara
---
v2:
- Addressed Numan's and Han's comments:
- add struct related_lports
- add test case.
---
controller/binding.c| 79 ++---
controller/binding.h| 31 ---
controller/lflow.c | 2 +-
controller/lflow.h | 2 +-
controller/ovn-controller.c | 48 +-
tests/ovn.at| 44 +
6 files changed, 120 insertions(+), 86 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index 7fde0fdbb..594babc98 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -531,38 +531,41 @@ remove_local_lports(const char *iface_id, struct
binding_ctx_out *b_ctx)
}
}
-/* Add a port binding ID (of the form "dp-key"_"port-key") to the set of local
- * lport IDs. Also track if the set has changed.
+/* Add a port binding to the set of locally relevant lports.
+ * Also track if the set has changed.
*/
static void
-update_local_lport_ids(const struct sbrec_port_binding *pb,
- struct binding_ctx_out *b_ctx)
+update_related_lport(const struct sbrec_port_binding *pb,
+ struct binding_ctx_out *b_ctx)
{
char buf[16];
get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key,
buf, sizeof(buf));
-if (sset_add(b_ctx->local_lport_ids, buf) != NULL) {
-b_ctx->local_lport_ids_changed = true;
+if (sset_add(_ctx->related_lports->lport_ids, buf) != NULL) {
+b_ctx->related_lports_changed = true;
if (b_ctx->tracked_dp_bindings) {
/* Add the 'pb' to the tracked_datapaths. */
tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings);
}
}
+sset_add(_ctx->related_lports->lport_names, pb->logical_port);
}
-/* Remove a port binding id from the set of local lport IDs. Also track if
- * the set has changed.
+/* Remove a port binding id from the set of locally relevant lports.
+ * Also track if the set has changed.
*/
static void
-remove_local_lport_ids(const struct sbrec_port_binding *pb,
- struct binding_ctx_out *b_ctx)
+remove_related_lport(const struct sbrec_port_binding *pb,
+ struct binding_ctx_out *b_ctx)
{
char buf[16];
get_unique_lport_key(pb->datapath->tunnel_key, pb->tunnel_key,
buf, sizeof(buf));
-if (sset_find_and_delete(b_ctx->local_lport_ids, buf)) {
-b_ctx->local_lport_ids_changed = true;
+sset_find_and_delete(_ctx->related_lports->lport_names,
+ pb->logical_port);
+if (sset_find_and_delete(_ctx->related_lports->lport_ids, buf)) {
+b_ctx->related_lports_changed = true;
if (b_ctx->tracked_dp_bindings) {
/* Add the 'pb' to the tracked_datapaths. */
@@ -678,6 +681,20 @@ static struct binding_lport
*binding_lport_check_and_cleanup(
static char *get_lport_type_str(enum en_lport_type lport_type);
+void
+related_lports_init(struct related_lports *rp)
+{
+sset_init(>lport_names);
+sset_init(>lport_ids);
+}
+
+void
+related_lports_destroy(struct related_lports *rp)
+{
+sset_destroy(>lport_names);
+sset_destroy(>lport_ids);
+}
+
void
local_binding_data_init(struct local_binding_data *lbinding_data)
{
@@ -1172,7 +1189,7 @@ release_binding_lport(const struct sbrec_chassis
*chassis_rec,
struct binding_ctx_out *b_ctx_out)
{
if (is_binding_lport_this_chassis(b_lport, chassis_rec)) {
-remove_local_lport_ids(b_lport->pb, b_ctx_out);
+remove_related_lport(b_lport->pb, b_ctx_out);
if (!release_lport(b_lport->pb, sb_readonly,
b_ctx_out->tracked_dp_bindings,
b_ctx_out->if_mgr)) {
@@ -1214,7 +1231,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
pb->datapath, false,
b_ctx_out->local_datapaths,
b_ctx_out->tracked_dp_bindings);
-update_local_lport_ids(pb, b_ctx_out);
+update_related_lport(pb, b_ctx_out);
update_local_lports(pb->logical_port, b_ctx_out);
if
