Message ID | c3026ef41e7089711aee745796ddf36994d86ff1.1616778834.git.lorenzo.bianconi@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,v2] controller: do not claim localports | expand |
LGTM Acked-by: Mark Michelson <mmichels@redhat.com> On 3/26/21 1:15 PM, Lorenzo Bianconi wrote: > Localports should not be binded to any specific controller but should be > available to each hv however in the current codebase consider_iface_claim > routine is claiming the localport for a specific chassis. > Fix the issue skipping localports in consider_iface_claim routine. > > https://bugzilla.redhat.com/show_bug.cgi?id=1937662 > > Reviewed-by: Mark D. Gray <mark.d.gray@redhat.com> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > Changes since v1: > - move lport_lookup before local_binding_find > - fix commit message > --- > controller/binding.c | 10 ++++++++-- > tests/ovn.at | 3 +++ > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 4e6c75696..7bb26f819 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1799,6 +1799,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > update_local_lports(iface_id, b_ctx_out); > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id); > > + const struct sbrec_port_binding *pb = > + lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, iface_id); > + if (pb && get_lport_type(pb) == LP_LOCALPORT) { > + /* nothing to do for localports. */ > + return true; > + } > + > struct local_binding *lbinding = > local_binding_find(b_ctx_out->local_bindings, iface_id); > > @@ -1810,8 +1817,7 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > } > > if (!lbinding->pb || strcmp(lbinding->name, lbinding->pb->logical_port)) { > - lbinding->pb = lport_lookup_by_name( > - b_ctx_in->sbrec_port_binding_by_name, lbinding->name); > + lbinding->pb = pb; > if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) { > lbinding->pb = NULL; > } > diff --git a/tests/ovn.at b/tests/ovn.at > index 391a8bcd9..d36e2483f 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -11567,6 +11567,9 @@ test_packet() { > fi > } > > +# Check the localport is not claimed by the hvs > +chassis_lp=$(fetch_column port_binding chassis logical_port=lp01) > +AT_CHECK([$(fetch_column port_binding chassis logical_port=lp01)], [0]) > > # lp11 and lp21 are on different hypervisors > test_packet 11 f0:00:00:00:00:21 f0:00:00:00:00:11 1121 lp21 lp21 >
On 26/03/2021 17:15, Lorenzo Bianconi wrote: > Localports should not be binded to any specific controller but should be > available to each hv however in the current codebase consider_iface_claim > routine is claiming the localport for a specific chassis. > Fix the issue skipping localports in consider_iface_claim routine. > > https://bugzilla.redhat.com/show_bug.cgi?id=1937662 > > Reviewed-by: Mark D. Gray <mark.d.gray@redhat.com> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > Changes since v1: > - move lport_lookup before local_binding_find > - fix commit message > --- > controller/binding.c | 10 ++++++++-- > tests/ovn.at | 3 +++ > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 4e6c75696..7bb26f819 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1799,6 +1799,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > update_local_lports(iface_id, b_ctx_out); > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id); > > + const struct sbrec_port_binding *pb = > + lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, iface_id); > + if (pb && get_lport_type(pb) == LP_LOCALPORT) { > + /* nothing to do for localports. */ > + return true; > + } > + > struct local_binding *lbinding = > local_binding_find(b_ctx_out->local_bindings, iface_id); > > @@ -1810,8 +1817,7 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > } > > if (!lbinding->pb || strcmp(lbinding->name, lbinding->pb->logical_port)) { > - lbinding->pb = lport_lookup_by_name( > - b_ctx_in->sbrec_port_binding_by_name, lbinding->name); > + lbinding->pb = pb; 'pb' could be 'NULL' here but I don't think that matters? > if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) { > lbinding->pb = NULL; > } > diff --git a/tests/ovn.at b/tests/ovn.at > index 391a8bcd9..d36e2483f 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -11567,6 +11567,9 @@ test_packet() { > fi > } > > +# Check the localport is not claimed by the hvs > +chassis_lp=$(fetch_column port_binding chassis logical_port=lp01) > +AT_CHECK([$(fetch_column port_binding chassis logical_port=lp01)], [0]) > > # lp11 and lp21 are on different hypervisors > test_packet 11 f0:00:00:00:00:21 f0:00:00:00:00:11 1121 lp21 lp21 > Tested it and it seems to work as expected. Thanks Lorenzo!! Acked-by: Mark D. Gray <mark.d.gray@redhat.com>
On Tue, Mar 30, 2021 at 6:12 AM Mark Gray <mark.d.gray@redhat.com> wrote: > > On 26/03/2021 17:15, Lorenzo Bianconi wrote: > > Localports should not be binded to any specific controller but should be > > available to each hv however in the current codebase consider_iface_claim > > routine is claiming the localport for a specific chassis. > > Fix the issue skipping localports in consider_iface_claim routine. > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1937662 > > > > Reviewed-by: Mark D. Gray <mark.d.gray@redhat.com> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Hi Lorenzo, This patch needs a rebase. Can you please submit v3 resolving the conflicts. Numan > > --- > > Changes since v1: > > - move lport_lookup before local_binding_find > > - fix commit message > > --- > > controller/binding.c | 10 ++++++++-- > > tests/ovn.at | 3 +++ > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 4e6c75696..7bb26f819 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -1799,6 +1799,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > > update_local_lports(iface_id, b_ctx_out); > > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id); > > > > + const struct sbrec_port_binding *pb = > > + lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, iface_id); > > + if (pb && get_lport_type(pb) == LP_LOCALPORT) { > > + /* nothing to do for localports. */ > > + return true; > > + } > > + > > struct local_binding *lbinding = > > local_binding_find(b_ctx_out->local_bindings, iface_id); > > > > @@ -1810,8 +1817,7 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > > } > > > > if (!lbinding->pb || strcmp(lbinding->name, lbinding->pb->logical_port)) { > > - lbinding->pb = lport_lookup_by_name( > > - b_ctx_in->sbrec_port_binding_by_name, lbinding->name); > > + lbinding->pb = pb; > > 'pb' could be 'NULL' here but I don't think that matters? > > if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) { > > lbinding->pb = NULL; > > } > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 391a8bcd9..d36e2483f 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -11567,6 +11567,9 @@ test_packet() { > > fi > > } > > > > +# Check the localport is not claimed by the hvs > > +chassis_lp=$(fetch_column port_binding chassis logical_port=lp01) > > +AT_CHECK([$(fetch_column port_binding chassis logical_port=lp01)], [0]) > > > > # lp11 and lp21 are on different hypervisors > > test_packet 11 f0:00:00:00:00:21 f0:00:00:00:00:11 1121 lp21 lp21 > > > > Tested it and it seems to work as expected. Thanks Lorenzo!! > > Acked-by: Mark D. Gray <mark.d.gray@redhat.com> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On Tue, Mar 30, 2021 at 6:12 AM Mark Gray <mark.d.gray@redhat.com> wrote: > > > > On 26/03/2021 17:15, Lorenzo Bianconi wrote: > > > Localports should not be binded to any specific controller but should be > > > available to each hv however in the current codebase consider_iface_claim > > > routine is claiming the localport for a specific chassis. > > > Fix the issue skipping localports in consider_iface_claim routine. > > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1937662 > > > > > > Reviewed-by: Mark D. Gray <mark.d.gray@redhat.com> > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > Hi Lorenzo, > > This patch needs a rebase. Can you please submit v3 resolving the conflicts. ack Numan, I will post v3 soon. Regards, Lorenzo > > Numan > > > > --- > > > Changes since v1: > > > - move lport_lookup before local_binding_find > > > - fix commit message > > > --- > > > controller/binding.c | 10 ++++++++-- > > > tests/ovn.at | 3 +++ > > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index 4e6c75696..7bb26f819 100644 > > > --- a/controller/binding.c > > > +++ b/controller/binding.c > > > @@ -1799,6 +1799,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > > > update_local_lports(iface_id, b_ctx_out); > > > smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id); > > > > > > + const struct sbrec_port_binding *pb = > > > + lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, iface_id); > > > + if (pb && get_lport_type(pb) == LP_LOCALPORT) { > > > + /* nothing to do for localports. */ > > > + return true; > > > + } > > > + > > > struct local_binding *lbinding = > > > local_binding_find(b_ctx_out->local_bindings, iface_id); > > > > > > @@ -1810,8 +1817,7 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > > > } > > > > > > if (!lbinding->pb || strcmp(lbinding->name, lbinding->pb->logical_port)) { > > > - lbinding->pb = lport_lookup_by_name( > > > - b_ctx_in->sbrec_port_binding_by_name, lbinding->name); > > > + lbinding->pb = pb; > > > > 'pb' could be 'NULL' here but I don't think that matters? > > > if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) { > > > lbinding->pb = NULL; > > > } > > > diff --git a/tests/ovn.at b/tests/ovn.at > > > index 391a8bcd9..d36e2483f 100644 > > > --- a/tests/ovn.at > > > +++ b/tests/ovn.at > > > @@ -11567,6 +11567,9 @@ test_packet() { > > > fi > > > } > > > > > > +# Check the localport is not claimed by the hvs > > > +chassis_lp=$(fetch_column port_binding chassis logical_port=lp01) > > > +AT_CHECK([$(fetch_column port_binding chassis logical_port=lp01)], [0]) > > > > > > # lp11 and lp21 are on different hypervisors > > > test_packet 11 f0:00:00:00:00:21 f0:00:00:00:00:11 1121 lp21 lp21 > > > > > > > Tested it and it seems to work as expected. Thanks Lorenzo!! > > > > Acked-by: Mark D. Gray <mark.d.gray@redhat.com> > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
diff --git a/controller/binding.c b/controller/binding.c index 4e6c75696..7bb26f819 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1799,6 +1799,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, update_local_lports(iface_id, b_ctx_out); smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id); + const struct sbrec_port_binding *pb = + lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, iface_id); + if (pb && get_lport_type(pb) == LP_LOCALPORT) { + /* nothing to do for localports. */ + return true; + } + struct local_binding *lbinding = local_binding_find(b_ctx_out->local_bindings, iface_id); @@ -1810,8 +1817,7 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, } if (!lbinding->pb || strcmp(lbinding->name, lbinding->pb->logical_port)) { - lbinding->pb = lport_lookup_by_name( - b_ctx_in->sbrec_port_binding_by_name, lbinding->name); + lbinding->pb = pb; if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) { lbinding->pb = NULL; } diff --git a/tests/ovn.at b/tests/ovn.at index 391a8bcd9..d36e2483f 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -11567,6 +11567,9 @@ test_packet() { fi } +# Check the localport is not claimed by the hvs +chassis_lp=$(fetch_column port_binding chassis logical_port=lp01) +AT_CHECK([$(fetch_column port_binding chassis logical_port=lp01)], [0]) # lp11 and lp21 are on different hypervisors test_packet 11 f0:00:00:00:00:21 f0:00:00:00:00:11 1121 lp21 lp21