[ovs-dev] [PATCH ovn 2/2] if-status: Add OVS interface status management module.
Numan Siddique
numans at ovn.org
Wed Apr 28 16:39:21 UTC 2021
On Fri, Apr 23, 2021 at 10:18 AM Dumitru Ceara <dceara at redhat.com> 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 <dceara at redhat.com>
Hi Dumitru,
Thanks for fixing this issue.
During my testing, I found a regression. If suppose an ovs port
"sw0p1" is bound on
a chassis for the logical port - sw0-port1, and if I delete the ovs port sw0p1,
ovn-controller clears the chassis column of Port_Binding but it
doesn't set the "up" column to "false".
It still remains "true".
All the tests passed for me locally but still I see this issue when I
ran in fake-multinode setup.
Can you please check if there are existing test cases which check for
the "up" column of port binding to be
false when it is released ?
I would also suggest adding a few test cases to replicate the issue it
is trying to address. If possible.
To test this scenario, I have one idea. Make the SB ovsdb-server read-only
(ovn-appctl -t ovnsb_db.ctl ovsdb-server/set-active-ovsdb-server
tcp:192.0.0.1:6642 and
ovn-appctl -t ovnsb_db.ctl ovsdb-server/connect-active-ovsdb-server)
after a port is claimed, and then run ovs-vsctl commands to release
the ovs port. At this point ovn-controller will fail
to set the "chassis" and "up" column. After a few seconds, make the
ovsdb-server active
and verify that the "chassis" and "up" columns are set properly.
Is this possible to do ? Or is this a wrong configuration ? Because
ovn-controller can not come to
know when ovsdb-server becomes active. Unless it keeps trying continuously.
In my testing, ovn-controller doesn't set it properly when I make the
ovsdb-server active again.
What do you think ?
Thanks
Numan
> ---
> controller/automake.mk | 2
> controller/binding.c | 302 +++++++++++----------------------
> controller/binding.h | 14 +-
> controller/if-status.c | 395 +++++++++++++++++++++++++++++++++++++++++++
> controller/if-status.h | 37 ++++
> controller/ovn-controller.c | 24 ++-
> 6 files changed, 562 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..3baf4cf17 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -16,13 +16,14 @@
> #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"
> #include "openvswitch/poll-loop.h"
> +#include "lib/hmapx.h"
> #include "lib/sset.h"
> #include "lib/util.h"
> #include "lib/netdev.h"
> @@ -41,32 +42,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 +647,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 *,
> @@ -739,6 +715,80 @@ 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);
> +
> + return !lbinding
> + || !lbinding->iface
> + || !smap_get_bool(&lbinding->iface->external_ids,
> + OVN_INSTALLED_EXT_ID, false);
> +}
> +
> +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 (lbinding && b_lport && lbinding->iface) {
> + if (!ovs_readonly && !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 && 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, &lbinding->binding_lports) {
> + sbrec_port_binding_set_up(b_lport->pb, &up, 1);
> + }
> + }
> + }
> +}
> +
> +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) {
> + bool up = false;
> + sbrec_port_binding_set_up(b_lport->pb, &up, 1);
> + }
> +}
> +
> void
> binding_dump_local_bindings(struct local_binding_data *lbinding_data,
> struct ds *out_data)
> @@ -959,7 +1009,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 +1020,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 +1033,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 +1085,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 +1108,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,7 +1160,8 @@ 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;
> }
> }
> @@ -1140,7 +1188,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 +1220,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 +1319,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 +1418,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 +2031,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 +2587,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 +2628,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);
> }
>
> diff --git a/controller/binding.h b/controller/binding.h
> index 4fc9ef207..4f733426b 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,6 @@ 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..c6b1a7fa6
> --- /dev/null
> +++ b/controller/if-status.c
> @@ -0,0 +1,395 @@
> +/* 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;
> + }
> + ovs_iface_destroy(mgr, iface);
> +}
> +
> +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 6f7c9ea61..e599ad1df 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. */
> @@ -996,6 +998,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;
> @@ -1003,7 +1006,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;
> @@ -1375,6 +1378,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;
> @@ -1401,6 +1406,7 @@ init_binding_ctx(struct engine_node *node,
> b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
> b_ctx_out->tracked_dp_bindings = NULL;
> b_ctx_out->local_lports_changed = NULL;
> + b_ctx_out->if_mgr = ctrl_ctx->if_mgr;
> }
>
> static void
> @@ -2440,7 +2446,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();
> @@ -2744,7 +2749,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);
> @@ -2954,9 +2961,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) {
> @@ -2967,9 +2975,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);
> }
>
> }
> @@ -3135,6 +3142,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);
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list