On 6/27/22 10:52, Xavier Simonart wrote:
> OVN checks whether ovn-installed is already present (in OVS) before updating
> it.
> This might cause ovn-installed related issues in the following case:
> - (1) ovn-installed is present
> - (2) we claim the interface
> - (3) we update ovs, removing ovn-installed and start installing flows
> - (4) (next loop), after flows installed, we check if ovn-installed is absent,
> and if yes, we update OVS with ovn-installed.
> However, in step4, if OVS is still busy from step 3, ovn-installed is read as
> present; hence OVN controller does not update it and move to INSTALLED state.
>
> Note that this does not happen with writing port up in SBDB because Port
> status
> changes will hit I-P.
>
> This patch will require updating the state machine description in the
> "controller: avoid recomputes triggered by SBDB Port_Binding updates." patch
> when it's merged, as it changes the if-status state-machine.
>
> Signed-off-by: Xavier Simonart <xsimo...@redhat.com>
> ---
Hi Xavier,
I only have a couple tiny style related remarks below. I think those
can be addressed at apply time if maintainers agree. Therefore:
Acked-by: Dumitru Ceara <dce...@redhat.com>
Thanks,
Dumitru
> controller/binding.c | 35 ++++++++++++++++++++++++++++++++--
> controller/binding.h | 6 ++++++
> controller/if-status.c | 43 ++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 80 insertions(+), 4 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 2279570f9..157c381cf 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -643,6 +643,19 @@ local_binding_get_lport_ofport(const struct shash
> *local_bindings,
> u16_to_ofp(lbinding->iface->ofport[0]) : 0;
> }
>
> +bool
> +local_binding_is_ovn_installed(struct shash *local_bindings,
> + const char *pb_name)
> +{
> + struct local_binding *lbinding =
> + local_binding_find(local_bindings, pb_name);
> + if (lbinding && lbinding->iface) {
> + return smap_get_bool(&lbinding->iface->external_ids,
> + OVN_INSTALLED_EXT_ID, false);
> + }
> + return false;
> +}
> +
> bool
> local_binding_is_up(struct shash *local_bindings, const char *pb_name)
> {
> @@ -715,6 +728,22 @@ local_binding_set_up(struct shash *local_bindings, const
> char *pb_name,
> }
> }
>
> +void
> +local_binding_remove_ovn_installed(struct shash *local_bindings,
> + const char *pb_name, bool ovs_readonly)
> +{
Nit: we could avoid the shash lookup if we return if ovs_readonly ==
true here.
> + struct local_binding *lbinding =
> + local_binding_find(local_bindings, pb_name);
> +
> + if (!ovs_readonly && lbinding && lbinding->iface
> + && smap_get_bool(&lbinding->iface->external_ids,
> + OVN_INSTALLED_EXT_ID, false)) {
> + VLOG_INFO("Removing lport %s ovn-installed in OVS", pb_name);
> + ovsrec_interface_update_external_ids_delkey(lbinding->iface,
> + OVN_INSTALLED_EXT_ID);
> + }
> +}
> +
> void
> local_binding_set_down(struct shash *local_bindings, const char *pb_name,
> const struct sbrec_chassis *chassis_rec,
> @@ -1297,9 +1326,11 @@ consider_vif_lport_(const struct sbrec_port_binding
> *pb,
> const char *requested_chassis_option = smap_get(
> &pb->options, "requested-chassis");
> VLOG_INFO_RL(&rl,
> - "Not claiming lport %s, chassis %s requested-chassis %s",
> + "Not claiming lport %s, chassis %s requested-chassis %s "
> + "pb->chassis %s",
> pb->logical_port, b_ctx_in->chassis_rec->name,
> - requested_chassis_option ? requested_chassis_option : "[]");
> + requested_chassis_option ? requested_chassis_option : "[]",
> + pb->chassis ? pb->chassis->name: "");
> }
> }
>
> diff --git a/controller/binding.h b/controller/binding.h
> index 1fed06674..445bdd9f2 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -152,6 +152,12 @@ ofp_port_t local_binding_get_lport_ofport(const struct
> shash *local_bindings,
> const char *pb_name);
>
> bool local_binding_is_up(struct shash *local_bindings, const char *pb_name);
> +bool local_binding_is_ovn_installed(struct shash *local_bindings,
> + const char *pb_name);
> +void local_binding_remove_ovn_installed(struct shash *local_bindings,
> + const char *pb_name,
> + bool ovs_readonly);
> +
> bool local_binding_is_down(struct shash *local_bindings, const char
> *pb_name);
> void local_binding_set_up(struct shash *local_bindings, const char *pb_name,
> const struct sbrec_chassis *chassis_rec,
> diff --git a/controller/if-status.c b/controller/if-status.c
> index ad61844d8..fe544c6ec 100644
> --- a/controller/if-status.c
> +++ b/controller/if-status.c
> @@ -57,6 +57,9 @@ enum if_state {
> OIF_INSTALL_FLOWS, /* Already claimed interface for which flows are still
> * being installed.
> */
> + OIF_REMOVE_OVN_INST,/* Interface with flows successfully installed in
> OVS,
> + * but with ovn-installed still in OVSDB.
> + */
Nit: indentation and missing space after ','.
> OIF_MARK_UP, /* Interface with flows successfully installed in OVS
> * but not yet marked "up" in the binding module (in
> * SB and OVS databases).
> @@ -73,6 +76,7 @@ enum if_state {
> static const char *if_state_names[] = {
> [OIF_CLAIMED] = "CLAIMED",
> [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS",
> + [OIF_REMOVE_OVN_INST] = "REMOVE_OVN_INST",
I know one often prefers the misaligned '=' over re-indenting all the
existing assignments but because we only have 6 lines here I think I'd
re-align all of them. But it's personal preference, it's fine to leave
it as-is too.
> [OIF_MARK_UP] = "MARK_UP",
> [OIF_MARK_DOWN] = "MARK_DOWN",
> [OIF_INSTALLED] = "INSTALLED",
> @@ -169,6 +173,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
> const char *iface_id)
> switch (iface->state) {
> case OIF_CLAIMED:
> case OIF_INSTALL_FLOWS:
> + case OIF_REMOVE_OVN_INST:
> case OIF_MARK_UP:
> /* Nothing to do here. */
> break;
> @@ -199,6 +204,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr,
> const char *iface_id)
> /* Not yet fully installed interfaces can be safely deleted. */
> ovs_iface_destroy(mgr, iface);
> break;
> + case OIF_REMOVE_OVN_INST:
> case OIF_MARK_UP:
> case OIF_INSTALLED:
> /* Properly mark interfaces "down" if their flows were already
> @@ -230,6 +236,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr,
> const char *iface_id)
> /* Not yet fully installed interfaces can be safely deleted. */
> ovs_iface_destroy(mgr, iface);
> break;
> + case OIF_REMOVE_OVN_INST:
> case OIF_MARK_UP:
> case OIF_INSTALLED:
> /* Properly mark interfaces "down" if their flows were already
> @@ -257,6 +264,17 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> struct shash *bindings = &binding_data->bindings;
> struct hmapx_node *node;
>
> + /* Move all interfaces that have been confirmed without ovn-installed,
> + * from OIF_REMOVE_OVN_INST to OIF_MARK_UP.
> + */
> + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_REMOVE_OVN_INST]) {
> + struct ovs_iface *iface = node->data;
> +
> + if (!local_binding_is_ovn_installed(bindings, iface->id)) {
> + ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> + }
> + }
> +
> /* Move all interfaces that have been confirmed "up" by the binding
> module,
> * from OIF_MARK_UP to OIF_INSTALLED.
> */
> @@ -325,7 +343,19 @@ if_status_mgr_run(struct if_status_mgr *mgr,
> iface->install_seqno)) {
> continue;
> }
> - ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> + /* Wait for ovn-installed to be absent before moving to MARK_UP
> state.
> + * Most of the times ovn-installed is already absent and hence we
> will
> + * not have to wait.
> + * If there is no binding_data, we can't determine if ovn-installed
> is
> + * present or not; hence also go to the OIF_REMOVE_OVN_INST state.
> + */
> + if (!binding_data ||
> + local_binding_is_ovn_installed(&binding_data->bindings,
> + iface->id)) {
> + ovs_iface_set_state(mgr, iface, OIF_REMOVE_OVN_INST);
> + } else {
> + ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> + }
> }
> ofctrl_acked_seqnos_destroy(acked_seqnos);
>
> @@ -412,7 +442,16 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr,
> sb_readonly, ovs_readonly);
> }
>
> - /* Notifiy the binding module to set "up" all bindings that have had
> + /* Notify the binding module to remove "ovn-installed" for all bindings
> + * in the OIF_REMOVE_OVN_INST state.
> + */
> + HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_REMOVE_OVN_INST]) {
> + struct ovs_iface *iface = node->data;
> +
> + local_binding_remove_ovn_installed(bindings, iface->id,
> ovs_readonly);
> + }
> +
> + /* Notify the binding module to set "up" all bindings that have had
> * their flows installed but are not yet marked "up" in the binding
> * module.
> */
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
