| Message ID | 20220627085202.1187277-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev,v2] ovn-controller: fixed ovn-installed not always properly added. | 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 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 <xsimonar@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 <dceara@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. > */
On 6/28/22 14:35, Dumitru Ceara wrote: > 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 <xsimonar@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 <dceara@redhat.com> > Hi Xavier, Unfortunately it seems this patch needs a rebase. Would it be possible for you to do that and also address the comments I had earlier in a new revision? Thanks, Dumitru
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) +{ + 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. + */ 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", [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. */
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 <xsimonar@redhat.com> --- controller/binding.c | 35 ++++++++++++++++++++++++++++++++-- controller/binding.h | 6 ++++++ controller/if-status.c | 43 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 80 insertions(+), 4 deletions(-)