On Tue, May 11, 2021 at 3:31 PM Mark Michelson <[email protected]> wrote: > > Hi Dumitru, > > The change looks good to me. It's very easy to understand the state > machine, and it's easy to follow how the code is structured. Yay! I > would have liked to see some more test cases, but given the discussion > you had on v1 of this patch, I can understand why they're not present in > this version. > > Acked-by: Mark Michelson <[email protected]>
Thanks Dumitru for v2 and Mark for reviews. I applied this patch to the main branch. Numan > > On 5/6/21 11:35 AM, Dumitru Ceara wrote: > > The initial implementation of the notification mechanism that indicates > > if OVS flows corresponding to a logical switch port have been installed > > is not resilient enough. It didn't cover the case when transactions to > > the local OVS DB or to the Southbound fail. > > > > In order to deal with that, factor out the code that tracks the state > > of the interfaces to a new module, 'if-status', and implement it with > > a state machine that will retry setting Port_Binding.UP and > > OVS.Interface.external_ids:ovn-installed in the Southbound and local > > OVS databases. > > > > Having a separate module makes sense because tracking the interface > > state doesn't really depend on the rest of the binding module, except > > for interacting with the Port_Binding and Interface IDL records. For > > this we add some additional APIs to binding.c. > > > > Reported-at: https://bugzilla.redhat.com/1952846 > > Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS > > flows are installed.") > > Signed-off-by: Dumitru Ceara <[email protected]> > > --- > > V2: > > - Address Numan's comments: > > - Update Port_Binding.up whenever an OVS interface is released. > > - Add a new test. > > - Factor out functions that set Port_Binding.up in binding.c > > - Remove useless hmapx.h include from binding.c. > > - Fix some indentation. > > --- > > controller/automake.mk | 2 + > > controller/binding.c | 338 ++++++++++++----------------- > > controller/binding.h | 13 +- > > controller/if-status.c | 415 ++++++++++++++++++++++++++++++++++++ > > controller/if-status.h | 37 ++++ > > controller/ovn-controller.c | 24 ++- > > tests/ovn.at | 8 + > > 7 files changed, 625 insertions(+), 212 deletions(-) > > create mode 100644 controller/if-status.c > > create mode 100644 controller/if-status.h > > > > diff --git a/controller/automake.mk b/controller/automake.mk > > index e664f1980..2f6c50890 100644 > > --- a/controller/automake.mk > > +++ b/controller/automake.mk > > @@ -10,6 +10,8 @@ controller_ovn_controller_SOURCES = \ > > controller/encaps.h \ > > controller/ha-chassis.c \ > > controller/ha-chassis.h \ > > + controller/if-status.c \ > > + controller/if-status.h \ > > controller/ip-mcast.c \ > > controller/ip-mcast.h \ > > controller/lflow.c \ > > diff --git a/controller/binding.c b/controller/binding.c > > index 451f00e34..31f3a210f 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -16,9 +16,9 @@ > > #include <config.h> > > #include "binding.h" > > #include "ha-chassis.h" > > +#include "if-status.h" > > #include "lflow.h" > > #include "lport.h" > > -#include "ofctrl-seqno.h" > > #include "patch.h" > > > > #include "lib/bitmap.h" > > @@ -41,32 +41,6 @@ VLOG_DEFINE_THIS_MODULE(binding); > > */ > > #define OVN_INSTALLED_EXT_ID "ovn-installed" > > > > -/* Set of OVS interface IDs that have been released in the most recent > > - * processing iterations. This gets updated in release_lport() and is > > - * periodically emptied in binding_seqno_run(). > > - */ > > -static struct sset binding_iface_released_set = > > - SSET_INITIALIZER(&binding_iface_released_set); > > - > > -/* Set of OVS interface IDs that have been bound in the most recent > > - * processing iterations. This gets updated in release_lport() and is > > - * periodically emptied in binding_seqno_run(). > > - */ > > -static struct sset binding_iface_bound_set = > > - SSET_INITIALIZER(&binding_iface_bound_set); > > - > > -static void > > -binding_iface_released_add(const char *iface_id) > > -{ > > - sset_add(&binding_iface_released_set, iface_id); > > -} > > - > > -static void > > -binding_iface_bound_add(const char *iface_id) > > -{ > > - sset_add(&binding_iface_bound_set, iface_id); > > -} > > - > > #define OVN_QOS_TYPE "linux-htb" > > > > struct qos_queue { > > @@ -672,7 +646,8 @@ static void local_binding_destroy(struct local_binding > > *, > > struct shash *binding_lports); > > static void local_binding_delete(struct local_binding *, > > struct shash *local_bindings, > > - struct shash *binding_lports); > > + struct shash *binding_lports, > > + struct if_status_mgr *if_mgr); > > static struct binding_lport *local_binding_add_lport( > > struct shash *binding_lports, > > struct local_binding *, > > @@ -692,6 +667,8 @@ static void binding_lport_delete(struct shash > > *binding_lports, > > struct binding_lport *); > > static void binding_lport_add(struct shash *binding_lports, > > struct binding_lport *); > > +static void binding_lport_set_up(struct binding_lport *, bool sb_readonly); > > +static void binding_lport_set_down(struct binding_lport *, bool > > sb_readonly); > > static struct binding_lport *binding_lport_find( > > struct shash *binding_lports, const char *lport_name); > > static const struct sbrec_port_binding *binding_lport_get_parent_pb( > > @@ -739,6 +716,93 @@ local_binding_get_primary_pb(struct shash > > *local_bindings, const char *pb_name) > > return b_lport ? b_lport->pb : NULL; > > } > > > > +bool > > +local_binding_is_up(struct shash *local_bindings, const char *pb_name) > > +{ > > + struct local_binding *lbinding = > > + local_binding_find(local_bindings, pb_name); > > + struct binding_lport *b_lport = > > local_binding_get_primary_lport(lbinding); > > + if (lbinding && b_lport && lbinding->iface) { > > + if (b_lport->pb->n_up && !b_lport->pb->up[0]) { > > + return false; > > + } > > + return smap_get_bool(&lbinding->iface->external_ids, > > + OVN_INSTALLED_EXT_ID, false); > > + } > > + return false; > > +} > > + > > +bool > > +local_binding_is_down(struct shash *local_bindings, const char *pb_name) > > +{ > > + struct local_binding *lbinding = > > + local_binding_find(local_bindings, pb_name); > > + > > + struct binding_lport *b_lport = > > local_binding_get_primary_lport(lbinding); > > + > > + if (!lbinding) { > > + return true; > > + } > > + > > + if (lbinding->iface && smap_get_bool(&lbinding->iface->external_ids, > > + OVN_INSTALLED_EXT_ID, false)) { > > + return false; > > + } > > + > > + if (b_lport && b_lport->pb->n_up && b_lport->pb->up[0]) { > > + return false; > > + } > > + > > + return true; > > +} > > + > > +void > > +local_binding_set_up(struct shash *local_bindings, const char *pb_name, > > + bool sb_readonly, bool ovs_readonly) > > +{ > > + struct local_binding *lbinding = > > + local_binding_find(local_bindings, pb_name); > > + struct binding_lport *b_lport = > > local_binding_get_primary_lport(lbinding); > > + > > + if (!ovs_readonly && lbinding && lbinding->iface > > + && !smap_get_bool(&lbinding->iface->external_ids, > > + OVN_INSTALLED_EXT_ID, false)) { > > + ovsrec_interface_update_external_ids_setkey(lbinding->iface, > > + OVN_INSTALLED_EXT_ID, > > + "true"); > > + } > > + > > + if (!sb_readonly && lbinding && b_lport && b_lport->pb->n_up) { > > + binding_lport_set_up(b_lport, sb_readonly); > > + LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > > + binding_lport_set_up(b_lport, sb_readonly); > > + } > > + } > > +} > > + > > +void > > +local_binding_set_down(struct shash *local_bindings, const char *pb_name, > > + bool sb_readonly, bool ovs_readonly) > > +{ > > + struct local_binding *lbinding = > > + local_binding_find(local_bindings, pb_name); > > + struct binding_lport *b_lport = > > local_binding_get_primary_lport(lbinding); > > + > > + if (!ovs_readonly && lbinding && lbinding->iface > > + && smap_get_bool(&lbinding->iface->external_ids, > > + OVN_INSTALLED_EXT_ID, false)) { > > + ovsrec_interface_update_external_ids_delkey(lbinding->iface, > > + OVN_INSTALLED_EXT_ID); > > + } > > + > > + if (!sb_readonly && b_lport && b_lport->pb->n_up) { > > + binding_lport_set_down(b_lport, sb_readonly); > > + LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > > + binding_lport_set_down(b_lport, sb_readonly); > > + } > > + } > > +} > > + > > void > > binding_dump_local_bindings(struct local_binding_data *lbinding_data, > > struct ds *out_data) > > @@ -959,7 +1023,7 @@ static void > > claimed_lport_set_up(const struct sbrec_port_binding *pb, > > const struct sbrec_port_binding *parent_pb, > > const struct sbrec_chassis *chassis_rec, > > - bool notify_up) > > + bool notify_up, struct if_status_mgr *if_mgr) > > { > > if (!notify_up) { > > bool up = true; > > @@ -970,7 +1034,7 @@ claimed_lport_set_up(const struct sbrec_port_binding > > *pb, > > } > > > > if (pb->chassis != chassis_rec || (pb->n_up && !pb->up[0])) { > > - binding_iface_bound_add(pb->logical_port); > > + if_status_mgr_claim_iface(if_mgr, pb->logical_port); > > } > > } > > > > @@ -983,10 +1047,11 @@ claim_lport(const struct sbrec_port_binding *pb, > > const struct sbrec_chassis *chassis_rec, > > const struct ovsrec_interface *iface_rec, > > bool sb_readonly, bool notify_up, > > - struct hmap *tracked_datapaths) > > + struct hmap *tracked_datapaths, > > + struct if_status_mgr *if_mgr) > > { > > if (!sb_readonly) { > > - claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up); > > + claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up, > > if_mgr); > > } > > > > if (pb->chassis != chassis_rec) { > > @@ -1034,7 +1099,7 @@ claim_lport(const struct sbrec_port_binding *pb, > > */ > > static bool > > release_lport(const struct sbrec_port_binding *pb, bool sb_readonly, > > - struct hmap *tracked_datapaths) > > + struct hmap *tracked_datapaths, struct if_status_mgr *if_mgr) > > { > > if (pb->encap) { > > if (sb_readonly) { > > @@ -1057,12 +1122,8 @@ release_lport(const struct sbrec_port_binding *pb, > > bool sb_readonly, > > sbrec_port_binding_set_virtual_parent(pb, NULL); > > } > > > > - if (pb->n_up) { > > - bool up = false; > > - sbrec_port_binding_set_up(pb, &up, 1); > > - } > > update_lport_tracking(pb, tracked_datapaths); > > - binding_iface_released_add(pb->logical_port); > > + if_status_mgr_release_iface(if_mgr, pb->logical_port); > > VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); > > return true; > > } > > @@ -1113,9 +1174,11 @@ release_binding_lport(const struct sbrec_chassis > > *chassis_rec, > > if (is_binding_lport_this_chassis(b_lport, chassis_rec)) { > > remove_local_lport_ids(b_lport->pb, b_ctx_out); > > if (!release_lport(b_lport->pb, sb_readonly, > > - b_ctx_out->tracked_dp_bindings)) { > > + b_ctx_out->tracked_dp_bindings, > > + b_ctx_out->if_mgr)) { > > return false; > > } > > + binding_lport_set_down(b_lport, sb_readonly); > > } > > > > return true; > > @@ -1140,7 +1203,8 @@ consider_vif_lport_(const struct sbrec_port_binding > > *pb, > > if (!claim_lport(pb, parent_pb, b_ctx_in->chassis_rec, > > b_lport->lbinding->iface, > > !b_ctx_in->ovnsb_idl_txn, > > - !parent_pb, b_ctx_out->tracked_dp_bindings)){ > > + !parent_pb, b_ctx_out->tracked_dp_bindings, > > + b_ctx_out->if_mgr)){ > > return false; > > } > > > > @@ -1171,7 +1235,8 @@ consider_vif_lport_(const struct sbrec_port_binding > > *pb, > > /* Release the lport if there is no lbinding. */ > > if (!lbinding_set || !can_bind) { > > return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > - b_ctx_out->tracked_dp_bindings); > > + b_ctx_out->tracked_dp_bindings, > > + b_ctx_out->if_mgr); > > } > > } > > > > @@ -1269,7 +1334,8 @@ consider_container_lport(const struct > > sbrec_port_binding *pb, > > if (is_binding_lport_this_chassis(container_b_lport, > > b_ctx_in->chassis_rec)) { > > return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > - b_ctx_out->tracked_dp_bindings); > > + b_ctx_out->tracked_dp_bindings, > > + b_ctx_out->if_mgr); > > } > > > > return true; > > @@ -1367,10 +1433,12 @@ consider_nonvif_lport_(const struct > > sbrec_port_binding *pb, > > update_local_lport_ids(pb, b_ctx_out); > > return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL, > > !b_ctx_in->ovnsb_idl_txn, false, > > - b_ctx_out->tracked_dp_bindings); > > + b_ctx_out->tracked_dp_bindings, > > + b_ctx_out->if_mgr); > > } else if (pb->chassis == b_ctx_in->chassis_rec) { > > return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > - b_ctx_out->tracked_dp_bindings); > > + b_ctx_out->tracked_dp_bindings, > > + b_ctx_out->if_mgr); > > } > > > > return true; > > @@ -1978,7 +2046,8 @@ consider_iface_release(const struct ovsrec_interface > > *iface_rec, > > /* Check if the lbinding has children of type PB_CONTAINER. > > * If so, don't delete the local_binding. */ > > if (lbinding && !is_lbinding_container_parent(lbinding)) { > > - local_binding_delete(lbinding, local_bindings, binding_lports); > > + local_binding_delete(lbinding, local_bindings, binding_lports, > > + b_ctx_out->if_mgr); > > } > > > > remove_local_lports(iface_id, b_ctx_out); > > @@ -2533,160 +2602,6 @@ delete_done: > > return handled; > > } > > > > -/* Registered ofctrl seqno type for port_binding flow installation. */ > > -static size_t binding_seq_type_pb_cfg; > > - > > -/* Binding specific seqno to be acked by ofctrl when flows for new > > interfaces > > - * have been installed. > > - */ > > -static uint32_t binding_iface_seqno = 0; > > - > > -/* Map indexed by iface-id containing the sequence numbers that when acked > > - * indicate that the OVS flows for the iface-id have been installed. > > - */ > > -static struct simap binding_iface_seqno_map = > > - SIMAP_INITIALIZER(&binding_iface_seqno_map); > > - > > -void > > -binding_init(void) > > -{ > > - binding_seq_type_pb_cfg = ofctrl_seqno_add_type(); > > -} > > - > > -/* Processes new release/bind operations OVN ports. For newly bound ports > > - * it creates ofctrl seqno update requests that will be acked when > > - * corresponding OVS flows have been installed. > > - * > > - * NOTE: Should be called only when valid SB and OVS transactions are > > - * available. > > - */ > > -void > > -binding_seqno_run(struct local_binding_data *lbinding_data) > > -{ > > - const char *iface_id; > > - const char *iface_id_next; > > - struct shash *local_bindings = &lbinding_data->bindings; > > - SSET_FOR_EACH_SAFE (iface_id, iface_id_next, > > &binding_iface_released_set) { > > - struct shash_node *lb_node = shash_find(local_bindings, iface_id); > > - > > - /* If the local binding still exists (i.e., the OVS interface is > > - * still configured locally) then remove the external id and remove > > - * it from the in-flight seqno map. > > - */ > > - if (lb_node) { > > - struct local_binding *lb = lb_node->data; > > - > > - if (lb->iface && smap_get(&lb->iface->external_ids, > > - OVN_INSTALLED_EXT_ID)) { > > - ovsrec_interface_update_external_ids_delkey( > > - lb->iface, OVN_INSTALLED_EXT_ID); > > - } > > - } > > - simap_find_and_delete(&binding_iface_seqno_map, iface_id); > > - sset_delete(&binding_iface_released_set, > > - SSET_NODE_FROM_NAME(iface_id)); > > - } > > - > > - bool new_ifaces = false; > > - uint32_t new_seqno = binding_iface_seqno + 1; > > - > > - SSET_FOR_EACH_SAFE (iface_id, iface_id_next, &binding_iface_bound_set) > > { > > - struct shash_node *lb_node = shash_find(local_bindings, iface_id); > > - > > - struct local_binding *lb = lb_node ? lb_node->data : NULL; > > - > > - /* Make sure the binding is still complete, i.e., both SB > > port_binding > > - * and OVS interface still exist. > > - * > > - * If so, then this is a newly bound interface, make sure we reset > > the > > - * Port_Binding 'up' field and the OVS Interface 'external-id'. > > - */ > > - struct binding_lport *b_lport = > > local_binding_get_primary_lport(lb); > > - if (lb && b_lport && lb->iface > > - && !simap_contains(&binding_iface_seqno_map, lb->name)) { > > - new_ifaces = true; > > - > > - if (smap_get(&lb->iface->external_ids, OVN_INSTALLED_EXT_ID)) { > > - ovsrec_interface_update_external_ids_delkey( > > - lb->iface, OVN_INSTALLED_EXT_ID); > > - } > > - if (b_lport->pb->n_up) { > > - bool up = false; > > - sbrec_port_binding_set_up(b_lport->pb, &up, 1); > > - } > > - simap_put(&binding_iface_seqno_map, lb->name, new_seqno); > > - } > > - sset_delete(&binding_iface_bound_set, > > SSET_NODE_FROM_NAME(iface_id)); > > - } > > - > > - /* Request a seqno update when the flows for new interfaces have been > > - * installed in OVS. > > - */ > > - if (new_ifaces) { > > - binding_iface_seqno = new_seqno; > > - ofctrl_seqno_update_create(binding_seq_type_pb_cfg, new_seqno); > > - } > > -} > > - > > -/* Processes ofctrl seqno ACKs for new bindings. Sets the > > - * 'OVN_INSTALLED_EXT_ID' external-id in the OVS interface and the > > - * Port_Binding.up field for all ports for which OVS flows have been > > - * installed. > > - * > > - * NOTE: Should be called only when valid SB and OVS transactions are > > - * available. > > - */ > > -void > > -binding_seqno_install(struct local_binding_data *lbinding_data) > > -{ > > - struct ofctrl_acked_seqnos *acked_seqnos = > > - ofctrl_acked_seqnos_get(binding_seq_type_pb_cfg); > > - struct simap_node *node; > > - struct simap_node *node_next; > > - struct shash *local_bindings = &lbinding_data->bindings; > > - > > - SIMAP_FOR_EACH_SAFE (node, node_next, &binding_iface_seqno_map) { > > - struct shash_node *lb_node = shash_find(local_bindings, > > node->name); > > - > > - if (!lb_node) { > > - goto del_seqno; > > - } > > - > > - struct local_binding *lb = lb_node->data; > > - struct binding_lport *b_lport = > > local_binding_get_primary_lport(lb); > > - if (!b_lport || !lb->iface) { > > - goto del_seqno; > > - } > > - > > - if (!ofctrl_acked_seqnos_contains(acked_seqnos, node->data)) { > > - continue; > > - } > > - > > - ovsrec_interface_update_external_ids_setkey(lb->iface, > > - OVN_INSTALLED_EXT_ID, > > - "true"); > > - if (b_lport->pb->n_up) { > > - bool up = true; > > - > > - sbrec_port_binding_set_up(b_lport->pb, &up, 1); > > - LIST_FOR_EACH (b_lport, list_node, &lb->binding_lports) { > > - sbrec_port_binding_set_up(b_lport->pb, &up, 1); > > - } > > - } > > - > > -del_seqno: > > - simap_delete(&binding_iface_seqno_map, node); > > - } > > - > > - ofctrl_acked_seqnos_destroy(acked_seqnos); > > -} > > - > > -void > > -binding_seqno_flush(void) > > -{ > > - simap_clear(&binding_iface_seqno_map); > > -} > > - > > /* Static functions for local_lbindind and binding_lport. */ > > static struct local_binding * > > local_binding_create(const char *name, const struct ovsrec_interface > > *iface) > > @@ -2728,9 +2643,11 @@ local_binding_destroy(struct local_binding *lbinding, > > static void > > local_binding_delete(struct local_binding *lbinding, > > struct shash *local_bindings, > > - struct shash *binding_lports) > > + struct shash *binding_lports, > > + struct if_status_mgr *if_mgr) > > { > > shash_find_and_delete(local_bindings, lbinding->name); > > + if_status_mgr_delete_iface(if_mgr, lbinding->name); > > local_binding_destroy(lbinding, binding_lports); > > } > > > > @@ -2897,6 +2814,27 @@ binding_lport_delete(struct shash *binding_lports, > > binding_lport_destroy(b_lport); > > } > > > > +static void > > +binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly) > > +{ > > + if (sb_readonly || !b_lport || !b_lport->pb->n_up || > > b_lport->pb->up[0]) { > > + return; > > + } > > + > > + bool up = true; > > + sbrec_port_binding_set_up(b_lport->pb, &up, 1); > > +} > > + > > +static void > > +binding_lport_set_down(struct binding_lport *b_lport, bool sb_readonly) > > +{ > > + if (sb_readonly || !b_lport || !b_lport->pb->n_up || > > !b_lport->pb->up[0]) { > > + return; > > + } > > + > > + bool up = false; > > + sbrec_port_binding_set_up(b_lport->pb, &up, 1); > > +} > > > > static const struct sbrec_port_binding * > > binding_lport_get_parent_pb(struct binding_lport *b_lport) > > diff --git a/controller/binding.h b/controller/binding.h > > index 4fc9ef207..7a6495320 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -37,6 +37,7 @@ struct sbrec_port_binding_table; > > struct sset; > > struct sbrec_port_binding; > > struct ds; > > +struct if_status_mgr; > > > > struct binding_ctx_in { > > struct ovsdb_idl_txn *ovnsb_idl_txn; > > @@ -85,6 +86,8 @@ struct binding_ctx_out { > > * binding_handle_port_binding_changes) fills in for > > * the changed datapaths and port bindings. */ > > struct hmap *tracked_dp_bindings; > > + > > + struct if_status_mgr *if_mgr; > > }; > > > > struct local_binding_data { > > @@ -97,6 +100,12 @@ void local_binding_data_destroy(struct > > local_binding_data *); > > > > const struct sbrec_port_binding *local_binding_get_primary_pb( > > 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_down(struct shash *local_bindings, const char > > *pb_name); > > +void local_binding_set_up(struct shash *local_bindings, const char > > *pb_name, > > + bool sb_readonly, bool ovs_readonly); > > +void local_binding_set_down(struct shash *local_bindings, const char > > *pb_name, > > + bool sb_readonly, bool ovs_readonly); > > > > /* Represents a tracked binding logical port. */ > > struct tracked_binding_lport { > > @@ -123,9 +132,5 @@ bool binding_handle_port_binding_changes(struct > > binding_ctx_in *, > > struct binding_ctx_out *); > > void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); > > > > -void binding_init(void); > > -void binding_seqno_run(struct local_binding_data *lbinding_data); > > -void binding_seqno_install(struct local_binding_data *lbinding_data); > > -void binding_seqno_flush(void); > > void binding_dump_local_bindings(struct local_binding_data *, struct ds > > *); > > #endif /* controller/binding.h */ > > diff --git a/controller/if-status.c b/controller/if-status.c > > new file mode 100644 > > index 000000000..8d8c8d436 > > --- /dev/null > > +++ b/controller/if-status.c > > @@ -0,0 +1,415 @@ > > +/* Copyright (c) 2021, Red Hat, Inc. > > + * > > + * Licensed under the Apache License, Version 2.0 (the "License"); > > + * you may not use this file except in compliance with the License. > > + * You may obtain a copy of the License at: > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > + * > > + * Unless required by applicable law or agreed to in writing, software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > > + * See the License for the specific language governing permissions and > > + * limitations under the License. > > + */ > > + > > +#include <config.h> > > + > > +#include "binding.h" > > +#include "if-status.h" > > +#include "ofctrl-seqno.h" > > + > > +#include "lib/hmapx.h" > > +#include "lib/util.h" > > +#include "openvswitch/vlog.h" > > + > > +VLOG_DEFINE_THIS_MODULE(if_status); > > + > > +/* This module implements an interface manager that maintains the state of > > + * the interfaces wrt. their flows being completely installed in OVS and > > + * their corresponding bindings being marked up/down. > > + * > > + * A state machine is maintained for each interface. > > + * > > + * Transitions are triggered between states by three types of events: > > + * A. Events received from the binding module: > > + * - interface is claimed: if_status_mgr_claim_iface() > > + * - interface is released: if_status_mgr_release_iface() > > + * - interface is deleted: if_status_mgr_delete_iface() > > + * > > + * B. At every iteration, based on SB/OVS updates, handled in > > + * if_status_mgr_update(): > > + * - an interface binding has been marked "up" both in the Southbound and > > OVS > > + * databases. > > + * - an interface binding has been marked "down" both in the Southbound > > and OVS > > + * databases. > > + * - new interface has been claimed. > > + * > > + * C. At every iteration, based on ofctrl_seqno updates, handled in > > + * if_status_mgr_run(): > > + * - the flows for a previously claimed interface have been installed in > > OVS. > > + */ > > + > > +enum if_state { > > + OIF_CLAIMED, /* Newly claimed interface. */ > > + OIF_INSTALL_FLOWS, /* Already claimed interface for which flows are > > still > > + * being installed. > > + */ > > + OIF_MARK_UP, /* Interface with flows successfully installed in > > OVS > > + * but not yet marked "up" in the binding module (in > > + * SB and OVS databases). > > + */ > > + OIF_MARK_DOWN, /* Released interface but not yet marked "down" in > > the > > + * binding module (in SB and/or OVS databases). > > + */ > > + OIF_INSTALLED, /* Interface flows programmed in OVS and binding > > marked > > + * "up" in the binding module. > > + */ > > + OIF_MAX, > > +}; > > + > > +static const char *if_state_names[] = { > > + [OIF_CLAIMED] = "CLAIMED", > > + [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS", > > + [OIF_MARK_UP] = "MARK_UP", > > + [OIF_MARK_DOWN] = "MARK_DOWN", > > + [OIF_INSTALLED] = "INSTALLED", > > +}; > > + > > +struct ovs_iface { > > + char *id; /* Extracted from OVS external_ids.iface_id. */ > > + enum if_state state; /* State of the interface in the state > > machine. */ > > + uint32_t install_seqno; /* Seqno at which this interface is expected to > > + * be fully programmed in OVS. Only used in > > state > > + * OIF_INSTALL_FLOWS. > > + */ > > +}; > > + > > +/* State machine manager for all local OVS interfaces. */ > > +struct if_status_mgr { > > + /* All local interfaces, mapping from 'iface-id' to 'struct > > ovs_iface'. */ > > + struct shash ifaces; > > + > > + /* All local interfaces, stored per state. */ > > + struct hmapx ifaces_per_state[OIF_MAX]; > > + > > + /* Registered ofctrl seqno type for port_binding flow installation. */ > > + size_t iface_seq_type_pb_cfg; > > + > > + /* Interface specific seqno to be acked by ofctrl when flows for new > > + * interfaces have been installed. > > + */ > > + uint32_t iface_seqno; > > +}; > > + > > +static struct ovs_iface *ovs_iface_create(struct if_status_mgr *, > > + const char *iface_id, > > + enum if_state ); > > +static void ovs_iface_destroy(struct if_status_mgr *, struct ovs_iface *); > > +static void ovs_iface_set_state(struct if_status_mgr *, struct ovs_iface *, > > + enum if_state); > > + > > +static void if_status_mgr_update_bindings( > > + struct if_status_mgr *mgr, struct local_binding_data *binding_data, > > + bool sb_readonly, bool ovs_readonly); > > + > > +struct if_status_mgr * > > +if_status_mgr_create(void) > > +{ > > + struct if_status_mgr *mgr = xzalloc(sizeof *mgr); > > + > > + mgr->iface_seq_type_pb_cfg = ofctrl_seqno_add_type(); > > + for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) { > > + hmapx_init(&mgr->ifaces_per_state[i]); > > + } > > + shash_init(&mgr->ifaces); > > + return mgr; > > +} > > + > > +void > > +if_status_mgr_clear(struct if_status_mgr *mgr) > > +{ > > + struct shash_node *node_next; > > + struct shash_node *node; > > + > > + SHASH_FOR_EACH_SAFE (node, node_next, &mgr->ifaces) { > > + ovs_iface_destroy(mgr, node->data); > > + } > > + ovs_assert(shash_is_empty(&mgr->ifaces)); > > + > > + for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) { > > + ovs_assert(hmapx_is_empty(&mgr->ifaces_per_state[i])); > > + } > > +} > > + > > +void > > +if_status_mgr_destroy(struct if_status_mgr *mgr) > > +{ > > + if_status_mgr_clear(mgr); > > + shash_destroy(&mgr->ifaces); > > + for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) { > > + hmapx_destroy(&mgr->ifaces_per_state[i]); > > + } > > + free(mgr); > > +} > > + > > +void > > +if_status_mgr_claim_iface(struct if_status_mgr *mgr, const char *iface_id) > > +{ > > + struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id); > > + > > + if (!iface) { > > + iface = ovs_iface_create(mgr, iface_id, OIF_CLAIMED); > > + } > > + > > + switch (iface->state) { > > + case OIF_CLAIMED: > > + case OIF_INSTALL_FLOWS: > > + case OIF_MARK_UP: > > + /* Nothing to do here. */ > > + break; > > + case OIF_INSTALLED: > > + case OIF_MARK_DOWN: > > + ovs_iface_set_state(mgr, iface, OIF_CLAIMED); > > + break; > > + case OIF_MAX: > > + OVS_NOT_REACHED(); > > + break; > > + } > > +} > > + > > +void > > +if_status_mgr_release_iface(struct if_status_mgr *mgr, const char > > *iface_id) > > +{ > > + struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id); > > + > > + if (!iface) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > + VLOG_WARN_RL(&rl, "Trying to release unknown interface %s", > > iface_id); > > + return; > > + } > > + > > + switch (iface->state) { > > + case OIF_CLAIMED: > > + case OIF_INSTALL_FLOWS: > > + /* Not yet fully installed interfaces can be safely deleted. */ > > + ovs_iface_destroy(mgr, iface); > > + break; > > + case OIF_MARK_UP: > > + case OIF_INSTALLED: > > + /* Properly mark interfaces "down" if their flows were already > > + * programmed in OVS. > > + */ > > + ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN); > > + break; > > + case OIF_MARK_DOWN: > > + /* Nothing to do here. */ > > + break; > > + case OIF_MAX: > > + OVS_NOT_REACHED(); > > + break; > > + } > > +} > > + > > +void > > +if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) > > +{ > > + struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id); > > + > > + if (!iface) { > > + return; > > + } > > + > > + switch (iface->state) { > > + case OIF_CLAIMED: > > + case OIF_INSTALL_FLOWS: > > + /* Not yet fully installed interfaces can be safely deleted. */ > > + ovs_iface_destroy(mgr, iface); > > + break; > > + case OIF_MARK_UP: > > + case OIF_INSTALLED: > > + /* Properly mark interfaces "down" if their flows were already > > + * programmed in OVS. > > + */ > > + ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN); > > + break; > > + case OIF_MARK_DOWN: > > + /* Nothing to do here. */ > > + break; > > + case OIF_MAX: > > + OVS_NOT_REACHED(); > > + break; > > + } > > +} > > + > > +void > > +if_status_mgr_update(struct if_status_mgr *mgr, > > + struct local_binding_data *binding_data) > > +{ > > + if (!binding_data) { > > + return; > > + } > > + > > + struct shash *bindings = &binding_data->bindings; > > + struct hmapx_node *node_next; > > + struct hmapx_node *node; > > + > > + /* Move all interfaces that have been confirmed "up" by the binding > > module, > > + * from OIF_MARK_UP to OIF_INSTALLED. > > + */ > > + HMAPX_FOR_EACH_SAFE (node, node_next, > > + &mgr->ifaces_per_state[OIF_MARK_UP]) { > > + struct ovs_iface *iface = node->data; > > + > > + if (local_binding_is_up(bindings, iface->id)) { > > + ovs_iface_set_state(mgr, iface, OIF_INSTALLED); > > + } > > + } > > + > > + /* Cleanup all interfaces that have been confirmed "down" by the > > binding > > + * module. > > + */ > > + HMAPX_FOR_EACH_SAFE (node, node_next, > > + &mgr->ifaces_per_state[OIF_MARK_DOWN]) { > > + struct ovs_iface *iface = node->data; > > + > > + if (local_binding_is_down(bindings, iface->id)) { > > + ovs_iface_destroy(mgr, iface); > > + } > > + } > > + > > + /* Register for a notification about flows being installed in OVS for > > all > > + * newly claimed interfaces. > > + * > > + * Move them from OIF_CLAIMED to OIF_INSTALL_FLOWS. > > + */ > > + bool new_ifaces = false; > > + HMAPX_FOR_EACH_SAFE (node, node_next, > > + &mgr->ifaces_per_state[OIF_CLAIMED]) { > > + struct ovs_iface *iface = node->data; > > + > > + ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS); > > + iface->install_seqno = mgr->iface_seqno + 1; > > + new_ifaces = true; > > + } > > + > > + /* Request a seqno update when the flows for new interfaces have been > > + * installed in OVS. > > + */ > > + if (new_ifaces) { > > + mgr->iface_seqno++; > > + ofctrl_seqno_update_create(mgr->iface_seq_type_pb_cfg, > > + mgr->iface_seqno); > > + VLOG_DBG("Seqno requested: %"PRIu32, mgr->iface_seqno); > > + } > > +} > > + > > +void > > +if_status_mgr_run(struct if_status_mgr *mgr, > > + struct local_binding_data *binding_data, > > + bool sb_readonly, bool ovs_readonly) > > +{ > > + struct ofctrl_acked_seqnos *acked_seqnos = > > + ofctrl_acked_seqnos_get(mgr->iface_seq_type_pb_cfg); > > + struct hmapx_node *node_next; > > + struct hmapx_node *node; > > + > > + /* Move interfaces from state OIF_INSTALL_FLOWS to OIF_MARK_UP if a > > + * notification has been received aabout their flows being installed > > + * in OVS. > > + */ > > + HMAPX_FOR_EACH_SAFE (node, node_next, > > + &mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) { > > + struct ovs_iface *iface = node->data; > > + > > + if (!ofctrl_acked_seqnos_contains(acked_seqnos, > > + iface->install_seqno)) { > > + continue; > > + } > > + ovs_iface_set_state(mgr, iface, OIF_MARK_UP); > > + } > > + ofctrl_acked_seqnos_destroy(acked_seqnos); > > + > > + /* Update binding states. */ > > + if_status_mgr_update_bindings(mgr, binding_data, sb_readonly, > > + ovs_readonly); > > +} > > + > > +static struct ovs_iface * > > +ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id, > > + enum if_state state) > > +{ > > + struct ovs_iface *iface = xzalloc(sizeof *iface); > > + > > + VLOG_DBG("Interface %s create.", iface->id); > > + iface->id = xstrdup(iface_id); > > + shash_add(&mgr->ifaces, iface_id, iface); > > + ovs_iface_set_state(mgr, iface, state); > > + return iface; > > +} > > + > > +static void > > +ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface) > > +{ > > + VLOG_DBG("Interface %s destroy: state %s", iface->id, > > + if_state_names[iface->state]); > > + hmapx_find_and_delete(&mgr->ifaces_per_state[iface->state], iface); > > + shash_find_and_delete(&mgr->ifaces, iface->id); > > + free(iface->id); > > + free(iface); > > +} > > + > > +static void > > +ovs_iface_set_state(struct if_status_mgr *mgr, struct ovs_iface *iface, > > + enum if_state state) > > +{ > > + VLOG_DBG("Interface %s set state: old %s, new %s", iface->id, > > + if_state_names[iface->state], > > + if_state_names[state]); > > + > > + hmapx_find_and_delete(&mgr->ifaces_per_state[iface->state], iface); > > + iface->state = state; > > + hmapx_add(&mgr->ifaces_per_state[iface->state], iface); > > + iface->install_seqno = 0; > > +} > > + > > +static void > > +if_status_mgr_update_bindings(struct if_status_mgr *mgr, > > + struct local_binding_data *binding_data, > > + bool sb_readonly, bool ovs_readonly) > > +{ > > + if (!binding_data) { > > + return; > > + } > > + > > + struct shash *bindings = &binding_data->bindings; > > + struct hmapx_node *node; > > + > > + /* Notify the binding module to set "down" all bindings that are still > > + * in the process of being installed in OVS, i.e., are not yet > > instsalled. > > + */ > > + HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) { > > + struct ovs_iface *iface = node->data; > > + > > + local_binding_set_down(bindings, iface->id, sb_readonly, > > ovs_readonly); > > + } > > + > > + /* Notifiy the binding module to set "up" all bindings that have had > > + * their flows installed but are not yet marked "up" in the binding > > + * module. > > + */ > > + HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_MARK_UP]) { > > + struct ovs_iface *iface = node->data; > > + > > + local_binding_set_up(bindings, iface->id, sb_readonly, > > ovs_readonly); > > + } > > + > > + /* Notify the binding module to set "down" all bindings that have been > > + * released but are not yet marked as "down" in the binding module. > > + */ > > + HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) { > > + struct ovs_iface *iface = node->data; > > + > > + local_binding_set_down(bindings, iface->id, sb_readonly, > > ovs_readonly); > > + } > > +} > > diff --git a/controller/if-status.h b/controller/if-status.h > > new file mode 100644 > > index 000000000..51fe7c684 > > --- /dev/null > > +++ b/controller/if-status.h > > @@ -0,0 +1,37 @@ > > +/* Copyright (c) 2021, Red Hat, Inc. > > + * > > + * Licensed under the Apache License, Version 2.0 (the "License"); > > + * you may not use this file except in compliance with the License. > > + * You may obtain a copy of the License at: > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > + * > > + * Unless required by applicable law or agreed to in writing, software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > > + * See the License for the specific language governing permissions and > > + * limitations under the License. > > + */ > > + > > +#ifndef IF_STATUS_H > > +#define IF_STATUS_H 1 > > + > > +#include "openvswitch/shash.h" > > + > > +#include "binding.h" > > + > > +struct if_status_mgr; > > + > > +struct if_status_mgr *if_status_mgr_create(void); > > +void if_status_mgr_clear(struct if_status_mgr *); > > +void if_status_mgr_destroy(struct if_status_mgr *); > > + > > +void if_status_mgr_claim_iface(struct if_status_mgr *, const char > > *iface_id); > > +void if_status_mgr_release_iface(struct if_status_mgr *, const char > > *iface_id); > > +void if_status_mgr_delete_iface(struct if_status_mgr *, const char > > *iface_id); > > + > > +void if_status_mgr_update(struct if_status_mgr *, struct > > local_binding_data *); > > +void if_status_mgr_run(struct if_status_mgr *mgr, struct > > local_binding_data *, > > + bool sb_readonly, bool ovs_readonly); > > + > > +# endif /* controller/if-status.h */ > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 6106a9661..22b183b2b 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -33,6 +33,7 @@ > > #include "openvswitch/dynamic-string.h" > > #include "encaps.h" > > #include "fatal-signal.h" > > +#include "if-status.h" > > #include "ip-mcast.h" > > #include "openvswitch/hmap.h" > > #include "lflow.h" > > @@ -103,6 +104,7 @@ OVS_NO_RETURN static void usage(void); > > > > struct controller_engine_ctx { > > struct lflow_cache *lflow_cache; > > + struct if_status_mgr *if_mgr; > > }; > > > > /* Pending packet to be injected into connected OVS. */ > > @@ -922,6 +924,7 @@ en_ofctrl_is_connected_cleanup(void *data OVS_UNUSED) > > static void > > en_ofctrl_is_connected_run(struct engine_node *node, void *data) > > { > > + struct controller_engine_ctx *ctrl_ctx = > > engine_get_context()->client_ctx; > > struct ed_type_ofctrl_is_connected *of_data = data; > > if (of_data->connected != ofctrl_is_connected()) { > > of_data->connected = !of_data->connected; > > @@ -929,7 +932,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, > > void *data) > > /* Flush ofctrl seqno requests when the ofctrl connection goes > > down. */ > > if (!of_data->connected) { > > ofctrl_seqno_flush(); > > - binding_seqno_flush(); > > + if_status_mgr_clear(ctrl_ctx->if_mgr); > > } > > engine_set_node_state(node, EN_UPDATED); > > return; > > @@ -1141,6 +1144,8 @@ init_binding_ctx(struct engine_node *node, > > engine_get_input("SB_port_binding", node), > > "datapath"); > > > > + struct controller_engine_ctx *ctrl_ctx = > > engine_get_context()->client_ctx; > > + > > b_ctx_in->ovnsb_idl_txn = engine_get_context()->ovnsb_idl_txn; > > b_ctx_in->ovs_idl_txn = engine_get_context()->ovs_idl_txn; > > b_ctx_in->sbrec_datapath_binding_by_key = > > sbrec_datapath_binding_by_key; > > @@ -1166,6 +1171,7 @@ init_binding_ctx(struct engine_node *node, > > b_ctx_out->lbinding_data = &rt_data->lbinding_data; > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > > b_ctx_out->tracked_dp_bindings = NULL; > > + b_ctx_out->if_mgr = ctrl_ctx->if_mgr; > > } > > > > static void > > @@ -2421,7 +2427,6 @@ main(int argc, char *argv[]) > > /* Register ofctrl seqno types. */ > > ofctrl_seq_type_nb_cfg = ofctrl_seqno_add_type(); > > > > - binding_init(); > > patch_init(); > > pinctrl_init(); > > lflow_init(); > > @@ -2725,7 +2730,9 @@ main(int argc, char *argv[]) > > > > struct controller_engine_ctx ctrl_engine_ctx = { > > .lflow_cache = lflow_cache_create(), > > + .if_mgr = if_status_mgr_create(), > > }; > > + struct if_status_mgr *if_mgr = ctrl_engine_ctx.if_mgr; > > > > char *ovn_version = ovn_get_internal_version(); > > VLOG_INFO("OVN internal version is : [%s]", ovn_version); > > @@ -2935,9 +2942,10 @@ main(int argc, char *argv[]) > > > > ovnsb_idl_loop.idl), > > ovnsb_cond_seqno, > > ovnsb_expected_cond_seqno)); > > - if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) { > > - binding_seqno_run(&runtime_data->lbinding_data); > > - } > > + > > + struct local_binding_data *binding_data = > > + runtime_data ? &runtime_data->lbinding_data : NULL; > > + if_status_mgr_update(if_mgr, binding_data); > > > > flow_output_data = engine_get_data(&en_flow_output); > > if (flow_output_data && ct_zones_data) { > > @@ -2948,9 +2956,8 @@ main(int argc, char *argv[]) > > engine_node_changed(&en_flow_output)); > > } > > ofctrl_seqno_run(ofctrl_get_cur_cfg()); > > - if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) { > > - > > binding_seqno_install(&runtime_data->lbinding_data); > > - } > > + if_status_mgr_run(if_mgr, binding_data, !ovnsb_idl_txn, > > + !ovs_idl_txn); > > } > > > > } > > @@ -3116,6 +3123,7 @@ loop_done: > > ofctrl_destroy(); > > pinctrl_destroy(); > > patch_destroy(); > > + if_status_mgr_destroy(if_mgr); > > > > ovsdb_idl_loop_destroy(&ovs_idl_loop); > > ovsdb_idl_loop_destroy(&ovnsb_idl_loop); > > diff --git a/tests/ovn.at b/tests/ovn.at > > index fe6a7c85b..3aeec7744 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -25022,6 +25022,14 @@ as hv1 ovn-appctl -t ovn-controller debug/resume > > wait_column "true" Port_Binding up logical_port=lsp1 > > wait_column "true" nb:Logical_Switch_Port up name=lsp1 > > > > +AS_BOX([ovn-controller should set Port_Binding.up - to false when OVS port > > is released]) > > +check ovs-vsctl remove Interface lsp1 external_ids iface-id > > +check ovs-vsctl remove Interface lsp2 external_ids iface-id > > +wait_column "false" Port_Binding up logical_port=lsp1 > > +wait_column "false" Port_Binding up logical_port=lsp2 > > +wait_column "false" Port_Binding up logical_port=lsp1 > > +wait_column "false" nb:Logical_Switch_Port up name=lsp1 > > + > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > ]) > > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
