| Message ID | 20240423115333.2796843-6-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Numan Siddique |
| Headers | show |
| Series | Fix I+P versus recompute differences. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Tue, Apr 23, 2024 at 7:54 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2023-May/405107.html > Suggested-by: Priyankar Jain <priyankar.jain@nutanix.com> > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > controller/binding.c | 12 +++++++++- > tests/ovn.at | 57 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 68 insertions(+), 1 deletion(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 0bef5dc42..c37066cbe 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1317,6 +1317,14 @@ lport_maybe_postpone(const char *port_name, long long int now, > return true; > } > > +static bool > +is_postponed_port(const char *port_name) > +{ > + struct claimed_port *cp = > + (struct claimed_port *) sset_find(&_postponed_ports, port_name); > + return !!cp; > +} This is wrong. '_postponed_ports' is an sset and not shash and sset_find returns 'struct sset_node'. You can use sset_contains i.,e -------------------- static bool is_postponed_port(const char *port_name) { return sset_contains(&_postponed_ports, port_name); } -------------------- With the above addressed: Acked-by: Numan Siddique <numans@ovn.org> Numan > + > /* Returns false if lport is not claimed due to 'sb_readonly'. > * Returns true otherwise. > */ > @@ -1491,7 +1499,8 @@ is_binding_lport_this_chassis(struct binding_lport *b_lport, > { > return (b_lport && b_lport->pb && chassis && > (b_lport->pb->chassis == chassis > - || is_additional_chassis(b_lport->pb, chassis))); > + || is_additional_chassis(b_lport->pb, chassis) > + || is_postponed_port(b_lport->pb->logical_port))); > } > > /* Returns 'true' if the 'lbinding' has binding lports of type LP_CONTAINER, > @@ -1593,6 +1602,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > } > > if (!lbinding_set || !can_bind) { > + remove_related_lport(pb, b_ctx_out); > return release_lport(pb, b_ctx_in->chassis_rec, > !b_ctx_in->ovnsb_idl_txn, > b_ctx_out->tracked_dp_bindings, > diff --git a/tests/ovn.at b/tests/ovn.at > index 74c5bccc0..b0aba2207 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -37838,3 +37838,60 @@ OVN_CLEANUP([hv1]) > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([Deleting vif while controller fight for port claim]) > +ovn_start > + > +ovn-nbctl ls-add ls0 > +ovn-nbctl lsp-add ls0 lsp0 > +ovn-nbctl lsp-add ls0 lsp1 > + > +net_add n1 > +for i in 1 2; do > + sim_add hv$i > + as hv$i > + ovs-vsctl add-br br-phys > + ovn_attach n1 br-phys 192.168.0.$i > +done > + > +check ovn-nbctl --wait=hv sync > +hv1_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv1) > +hv2_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv2) > + > +as hv1 ovs-vsctl -- add-port br-int vif1 -- set Interface vif1 external-ids:iface-id=lsp1 > +as hv1 ovs-vsctl -- add-port br-int vif -- set Interface vif external-ids:iface-id=lsp0 > +wait_for_ports_up > + > +# Delete vif => store flows w/ only vif1, and no vif > +as hv1 ovs-vsctl -- del-port br-int vif > +check ovn-nbctl --wait=hv sync > +DUMP_FLOWS([hv1], [oflows1]) > +sleep_controller hv1 > +as hv2 ovs-vsctl -- add-port br-int vif -- set Interface vif external-ids:iface-id=lsp0 > +as hv1 ovs-vsctl -- add-port br-int vif -- set Interface vif external-ids:iface-id=lsp0 > + > +OVS_WAIT_UNTIL([ > + chassis=$(ovn-sbctl --bare --columns chassis list port_binding lsp0) > + test "$chassis" = $hv2_uuid > +]) > + > +sleep_sb > +wake_up_controller hv1 > +sleep_controller hv2 > + > +as hv1 ovs-vsctl -- del-port br-int vif > +wake_up_sb > +wake_up_controller hv2 > + > +as hv2 ovs-vsctl -- del-port br-int vif > +check ovn-nbctl --wait=hv sync > + > +DUMP_FLOWS([hv1], [oflows2]) > + > +check diff oflows1 oflows2 > +OVN_CLEANUP([hv1],[hv2]) > + > +AT_CLEANUP > +]) > + > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, May 30, 2024 at 11:31 AM Numan Siddique <numans@ovn.org> wrote: > > On Tue, Apr 23, 2024 at 7:54 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2023-May/405107.html > > Suggested-by: Priyankar Jain <priyankar.jain@nutanix.com> > > > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > > controller/binding.c | 12 +++++++++- > > tests/ovn.at | 57 ++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 68 insertions(+), 1 deletion(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 0bef5dc42..c37066cbe 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -1317,6 +1317,14 @@ lport_maybe_postpone(const char *port_name, long long int now, > > return true; > > } > > > > +static bool > > +is_postponed_port(const char *port_name) > > +{ > > + struct claimed_port *cp = > > + (struct claimed_port *) sset_find(&_postponed_ports, port_name); > > + return !!cp; > > +} > > This is wrong. '_postponed_ports' is an sset and not shash and > sset_find returns 'struct sset_node'. > > You can use sset_contains i.,e > -------------------- > static bool > is_postponed_port(const char *port_name) > { > return sset_contains(&_postponed_ports, port_name); > } > -------------------- > > > With the above addressed: > > Acked-by: Numan Siddique <numans@ovn.org> I applied this patch series to main with the above changes. Numan > > Numan > > > > + > > /* Returns false if lport is not claimed due to 'sb_readonly'. > > * Returns true otherwise. > > */ > > @@ -1491,7 +1499,8 @@ is_binding_lport_this_chassis(struct binding_lport *b_lport, > > { > > return (b_lport && b_lport->pb && chassis && > > (b_lport->pb->chassis == chassis > > - || is_additional_chassis(b_lport->pb, chassis))); > > + || is_additional_chassis(b_lport->pb, chassis) > > + || is_postponed_port(b_lport->pb->logical_port))); > > } > > > > /* Returns 'true' if the 'lbinding' has binding lports of type LP_CONTAINER, > > @@ -1593,6 +1602,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > > } > > > > if (!lbinding_set || !can_bind) { > > + remove_related_lport(pb, b_ctx_out); > > return release_lport(pb, b_ctx_in->chassis_rec, > > !b_ctx_in->ovnsb_idl_txn, > > b_ctx_out->tracked_dp_bindings, > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 74c5bccc0..b0aba2207 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -37838,3 +37838,60 @@ OVN_CLEANUP([hv1]) > > > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([Deleting vif while controller fight for port claim]) > > +ovn_start > > + > > +ovn-nbctl ls-add ls0 > > +ovn-nbctl lsp-add ls0 lsp0 > > +ovn-nbctl lsp-add ls0 lsp1 > > + > > +net_add n1 > > +for i in 1 2; do > > + sim_add hv$i > > + as hv$i > > + ovs-vsctl add-br br-phys > > + ovn_attach n1 br-phys 192.168.0.$i > > +done > > + > > +check ovn-nbctl --wait=hv sync > > +hv1_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv1) > > +hv2_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv2) > > + > > +as hv1 ovs-vsctl -- add-port br-int vif1 -- set Interface vif1 external-ids:iface-id=lsp1 > > +as hv1 ovs-vsctl -- add-port br-int vif -- set Interface vif external-ids:iface-id=lsp0 > > +wait_for_ports_up > > + > > +# Delete vif => store flows w/ only vif1, and no vif > > +as hv1 ovs-vsctl -- del-port br-int vif > > +check ovn-nbctl --wait=hv sync > > +DUMP_FLOWS([hv1], [oflows1]) > > +sleep_controller hv1 > > +as hv2 ovs-vsctl -- add-port br-int vif -- set Interface vif external-ids:iface-id=lsp0 > > +as hv1 ovs-vsctl -- add-port br-int vif -- set Interface vif external-ids:iface-id=lsp0 > > + > > +OVS_WAIT_UNTIL([ > > + chassis=$(ovn-sbctl --bare --columns chassis list port_binding lsp0) > > + test "$chassis" = $hv2_uuid > > +]) > > + > > +sleep_sb > > +wake_up_controller hv1 > > +sleep_controller hv2 > > + > > +as hv1 ovs-vsctl -- del-port br-int vif > > +wake_up_sb > > +wake_up_controller hv2 > > + > > +as hv2 ovs-vsctl -- del-port br-int vif > > +check ovn-nbctl --wait=hv sync > > + > > +DUMP_FLOWS([hv1], [oflows2]) > > + > > +check diff oflows1 oflows2 > > +OVN_CLEANUP([hv1],[hv2]) > > + > > +AT_CLEANUP > > +]) > > + > > -- > > 2.31.1 > > > > _______________________________________________ > > 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 0bef5dc42..c37066cbe 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1317,6 +1317,14 @@ lport_maybe_postpone(const char *port_name, long long int now, return true; } +static bool +is_postponed_port(const char *port_name) +{ + struct claimed_port *cp = + (struct claimed_port *) sset_find(&_postponed_ports, port_name); + return !!cp; +} + /* Returns false if lport is not claimed due to 'sb_readonly'. * Returns true otherwise. */ @@ -1491,7 +1499,8 @@ is_binding_lport_this_chassis(struct binding_lport *b_lport, { return (b_lport && b_lport->pb && chassis && (b_lport->pb->chassis == chassis - || is_additional_chassis(b_lport->pb, chassis))); + || is_additional_chassis(b_lport->pb, chassis) + || is_postponed_port(b_lport->pb->logical_port))); } /* Returns 'true' if the 'lbinding' has binding lports of type LP_CONTAINER, @@ -1593,6 +1602,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, } if (!lbinding_set || !can_bind) { + remove_related_lport(pb, b_ctx_out); return release_lport(pb, b_ctx_in->chassis_rec, !b_ctx_in->ovnsb_idl_txn, b_ctx_out->tracked_dp_bindings, diff --git a/tests/ovn.at b/tests/ovn.at index 74c5bccc0..b0aba2207 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -37838,3 +37838,60 @@ OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([Deleting vif while controller fight for port claim]) +ovn_start + +ovn-nbctl ls-add ls0 +ovn-nbctl lsp-add ls0 lsp0 +ovn-nbctl lsp-add ls0 lsp1 + +net_add n1 +for i in 1 2; do + sim_add hv$i + as hv$i + ovs-vsctl add-br br-phys + ovn_attach n1 br-phys 192.168.0.$i +done + +check ovn-nbctl --wait=hv sync +hv1_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv1) +hv2_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv2) + +as hv1 ovs-vsctl -- add-port br-int vif1 -- set Interface vif1 external-ids:iface-id=lsp1 +as hv1 ovs-vsctl -- add-port br-int vif -- set Interface vif external-ids:iface-id=lsp0 +wait_for_ports_up + +# Delete vif => store flows w/ only vif1, and no vif +as hv1 ovs-vsctl -- del-port br-int vif +check ovn-nbctl --wait=hv sync +DUMP_FLOWS([hv1], [oflows1]) +sleep_controller hv1 +as hv2 ovs-vsctl -- add-port br-int vif -- set Interface vif external-ids:iface-id=lsp0 +as hv1 ovs-vsctl -- add-port br-int vif -- set Interface vif external-ids:iface-id=lsp0 + +OVS_WAIT_UNTIL([ + chassis=$(ovn-sbctl --bare --columns chassis list port_binding lsp0) + test "$chassis" = $hv2_uuid +]) + +sleep_sb +wake_up_controller hv1 +sleep_controller hv2 + +as hv1 ovs-vsctl -- del-port br-int vif +wake_up_sb +wake_up_controller hv2 + +as hv2 ovs-vsctl -- del-port br-int vif +check ovn-nbctl --wait=hv sync + +DUMP_FLOWS([hv1], [oflows2]) + +check diff oflows1 oflows2 +OVN_CLEANUP([hv1],[hv2]) + +AT_CLEANUP +]) +
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2023-May/405107.html Suggested-by: Priyankar Jain <priyankar.jain@nutanix.com> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- controller/binding.c | 12 +++++++++- tests/ovn.at | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-)