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(&b_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(&b_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(&b_ctx->related_lports->lport_names, > > + pb->logical_port); > > +if (sset_find_and_delete(&b_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(&rp->lport_names); > > +sset_init(&rp->lport_ids); > > +} > > + > > +void > > +related_lports_destroy(struct related_lports *rp) > > +{ > > +sset_destroy(&rp->lport_names); > > +sset_destroy(&rp->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
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(&b_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(&b_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(&b_ctx->related_lports->lport_names, > + pb->logical_port); > +if (sset_find_and_delete(&b_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(&rp->lport_names); > +sset_init(&rp->lport_ids); > +} > + > +void > +related_lports_destroy(struct related_lports *rp) > +{ > +sset_destroy(&rp->lport_names); > +sset_destroy(&rp->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(&b_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(&b_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(&b_ctx->related_lports->lport_names, + pb->logical_port); +if (sset_find_and_delete(&b_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(&rp->lport_names); +sset_init(&rp->lport_ids); +} + +void +related_lports_destroy(struct related_lports *rp) +{ +sset_destroy(&rp->lport_names); +sset_destroy(&rp->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);