On Fri, Aug 19, 2022 at 8:40 AM Xavier Simonart <[email protected]> wrote:
>
> When VIF ports are claimed on a chassis, SBDB Port_Binding table is
updated.
> If the SBDB IDL is still is read-only ("in transaction") when such a
update
> is required, the update is not possible and recompute is triggered through
> I+P failure.
>
> This situation can happen:
> - after updating Port_Binding->chassis to SBDB for one port, in a
following
> iteration, ovn-controller handles Interface:external_ids:ovn-installed
> (for the same port) while SBDB is still read-only.
> - after updating Port_Binding->chassis to SBDB for one port, in a
following
> iteration, ovn-controller updates Port_Binding->chassis for another
port,
> while SBDB is still read-only.
>
> This patch prevent the recompute, by having the if-status module
> updating the Port_Binding chassis (if needed) when possible.
> This does not delay Port_Binding chassis update compared to before this
patch.
> - With the patch, Port_Binding chassis will be updated as soon as SBDB is
> again writable, without recompute.
> - Without the patch, Port_Binding chassis was updated as soon as SBDB was
> again writable, through a recompute.
>
> As part of this patch, ovn-installed will not be updated for additional
chassis;
> it will only be updated when the migration is completed.
>
> Note that this patch also fixes an issue where port was not always
properly
> reported up for virtual ports (causing flaky failures in "virtual ports"
> test case).
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
> Signed-off-by: Xavier Simonart <[email protected]>
>
> ---
> v2: - handled Dumitru's comments.
> - handled Han's comments, mainly ensure we moved out of CLAIMED state
> only after updating pb->chassis to guarentee physical flows are
installed
> when ovn-installed is updated in OVS.
> - slighly reorganize the code to isolate 'notify_up = false' cases in
> claim_port (i.e. ports such as virtual ports), in the idea of
making
> future patch preventing recomputes when virtual ports are claimed.
> - updated test case to cause more race conditions.
> - rebased on origin/main
> - note that "additional chassis" as now supported by
> "Support LSP:options:requested-chassis as a list" might still cause
> recomputes.
> - fixed missing flows when Port_Binding chassis was updated by
mgr_update
> w/o any lflow recalculation.
> v3: - handled Dumitru's comments on v2, mainly have runtime_data handler
> handling pb_claims when sb becomes writable (instead of a lflow
handler).
> - fixed test as it was not checking recomputes on all hv, as well as
a flaky
> v4: - handled Han's comments, mainly update pb->chassis until it's
confirmed.
> - updated doc to reflect that pb->chassis update can happen at any
state, and
> the CLAIMED state is only for the initial attempt.
Thanks Xavier. All looks good to me except that it seems you updated the
diagram but forgot to update the description, I have the below small
incremental change, and if you are ok, I can merge it without the need for
v6:
diff --git a/controller/if-status.c b/controller/if-status.c
index 2590ec27f..d1c14ac30 100644
--- a/controller/if-status.c
+++ b/controller/if-status.c
@@ -54,11 +54,12 @@ VLOG_DEFINE_THIS_MODULE(if_status);
*/
enum if_state {
- OIF_CLAIMED, /* Newly claimed interface. pb->chassis not yet
updated.
- */
- OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis successfully
- * updated in SB and for which flows are still being
- * installed.
+ OIF_CLAIMED, /* Newly claimed interface. pb->chassis update not
yet
+ initiated. */
+ OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update sent to
+ * SB (but update notification not confirmed, so the
+ * update may be resent in any of the following
states)
+ * and 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
Thanks,
Han
> v5: - rebased on origin/main.
>
> Signed-off-by: Xavier Simonart <[email protected]>
> ---
> controller/binding.c | 174 +++++++++++++++++++++++++---------
> controller/binding.h | 18 +++-
> controller/if-status.c | 183 ++++++++++++++++++++++++++++++++----
> controller/if-status.h | 17 +++-
> controller/ovn-controller.c | 72 +++++++++++++-
> tests/ovn-macros.at | 11 +++
> tests/ovn.at | 134 +++++++++++++++++++++++++-
> tests/perf-northd.at | 19 +---
> 8 files changed, 540 insertions(+), 88 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 9f5393a92..74a156525 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -707,11 +707,17 @@ local_binding_get_lport_ofport(const struct shash
*local_bindings,
> }
>
> bool
> -local_binding_is_up(struct shash *local_bindings, const char *pb_name)
> +local_binding_is_up(struct shash *local_bindings, const char *pb_name,
> + const struct sbrec_chassis *chassis_rec)
> {
> struct local_binding *lbinding =
> local_binding_find(local_bindings, pb_name);
> struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> +
> + if (b_lport && b_lport->pb->chassis != chassis_rec) {
> + return false;
> + }
> +
> if (lbinding && b_lport && lbinding->iface) {
> if (b_lport->pb->n_up && !b_lport->pb->up[0]) {
> return false;
> @@ -723,13 +729,23 @@ 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)
> +local_binding_is_down(struct shash *local_bindings, const char *pb_name,
> + const struct sbrec_chassis *chassis_rec)
> {
> struct local_binding *lbinding =
> local_binding_find(local_bindings, pb_name);
>
> struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
>
> + if (b_lport) {
> + if (b_lport->pb->chassis == chassis_rec) {
> + return false;
> + } else if (b_lport->pb->chassis) {
> + VLOG_DBG("lport %s already claimed by other chassis",
> + b_lport->pb->logical_port);
> + }
> + }
> +
> if (!lbinding) {
> return true;
> }
> @@ -947,37 +963,95 @@ get_lport_type_str(enum en_lport_type lport_type)
> OVS_NOT_REACHED();
> }
>
> -/* For newly claimed ports, if 'notify_up' is 'false':
> +void
> +set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
> + const struct sbrec_chassis *chassis_rec,
> + bool is_set)
> +{
> + if (pb->chassis != chassis_rec) {
> + if (is_set) {
> + if (pb->chassis) {
> + VLOG_INFO("Changing chassis for lport %s from %s to %s.",
> + pb->logical_port, pb->chassis->name,
> + chassis_rec->name);
> + } else {
> + VLOG_INFO("Claiming lport %s for this chassis.",
> + pb->logical_port);
> + }
> + for (int i = 0; i < pb->n_mac; i++) {
> + VLOG_INFO("%s: Claiming %s", pb->logical_port,
pb->mac[i]);
> + }
> + sbrec_port_binding_set_chassis(pb, chassis_rec);
> + }
> + } else if (!is_set) {
> + sbrec_port_binding_set_chassis(pb, NULL);
> + }
> +}
> +
> +bool
> +local_bindings_pb_chassis_is_set(struct shash *local_bindings,
> + const char *pb_name,
> + const struct sbrec_chassis *chassis_rec)
> +{
> + struct local_binding *lbinding =
> + local_binding_find(local_bindings, pb_name);
> + struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> +
> + if (b_lport && b_lport->pb && b_lport->pb->chassis == chassis_rec) {
> + return true;
> + }
> + return false;
> +}
> +
> +void
> +local_binding_set_pb(struct shash *local_bindings, const char *pb_name,
> + const struct sbrec_chassis *chassis_rec,
> + struct hmap *tracked_datapaths, bool is_set)
> +{
> + struct local_binding *lbinding =
> + local_binding_find(local_bindings, pb_name);
> + struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> +
> + if (b_lport) {
> + set_pb_chassis_in_sbrec(b_lport->pb, chassis_rec, is_set);
> + if (tracked_datapaths) {
> + update_lport_tracking(b_lport->pb, tracked_datapaths, true);
> + }
> + }
> +}
> +
> +/* For newly claimed ports:
> * - set the 'pb.up' field to true if 'pb' has no 'parent_pb'.
> * - set the 'pb.up' field to true if 'parent_pb.up' is 'true' (e.g., for
> * container and virtual ports).
> - * Otherwise request a notification to be sent when the OVS flows
> - * corresponding to 'pb' have been installed.
> + *
> + * Returns false if lport is not claimed due to 'sb_readonly'.
> + * Returns true otherwise.
> *
> * Note:
> - * Updates (directly or through a notification) the 'pb->up' field
only if
> - * it's explicitly set to 'false'.
> + * Updates the 'pb->up' field only if it's explicitly set to 'false'.
> * This is to ensure compatibility with older versions of ovn-northd.
> */
> -static void
> +static bool
> 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, struct if_status_mgr *if_mgr)
> + bool sb_readonly)
> {
> - if (!notify_up) {
> - bool up = true;
> - if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
> + /* When notify_up is false in claim_port(), no state is created
> + * by if_status_mgr. In such cases, return false (i.e. trigger
recompute)
> + * if we can't update sb (because it is readonly).
> + */
> + bool up = true;
> + if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
> + if (!sb_readonly) {
> if (pb->n_up) {
> sbrec_port_binding_set_up(pb, &up, 1);
> }
> + } else if (pb->n_up && !pb->up[0]) {
> + return false;
> }
> - return;
> - }
> -
> - if (pb->chassis != chassis_rec || (pb->n_up && !pb->up[0])) {
> - if_status_mgr_claim_iface(if_mgr, pb->logical_port);
> }
> + return true;
> }
>
> typedef void (*set_func)(const struct sbrec_port_binding *pb,
> @@ -1085,44 +1159,51 @@ claim_lport(const struct sbrec_port_binding *pb,
> struct if_status_mgr *if_mgr,
> struct sset *postponed_ports)
> {
> - if (!sb_readonly) {
> - claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up,
if_mgr);
> - }
> -
> enum can_bind can_bind = lport_can_bind_on_this_chassis(chassis_rec,
pb);
> bool update_tracked = false;
>
> if (can_bind == CAN_BIND_AS_MAIN) {
> if (pb->chassis != chassis_rec) {
> - if (sb_readonly) {
> - return false;
> - }
> -
> long long int now = time_msec();
> if (pb->chassis) {
> if (lport_maybe_postpone(pb->logical_port, now,
> postponed_ports)) {
> return true;
> }
> - VLOG_INFO("Changing chassis for lport %s from %s to %s.",
> - pb->logical_port, pb->chassis->name,
> - chassis_rec->name);
> - } else {
> - VLOG_INFO("Claiming lport %s for this chassis.",
> - pb->logical_port);
> - }
> - for (size_t i = 0; i < pb->n_mac; i++) {
> - VLOG_INFO("%s: Claiming %s", pb->logical_port,
pb->mac[i]);
> }
> -
> - sbrec_port_binding_set_chassis(pb, chassis_rec);
> if (is_additional_chassis(pb, chassis_rec)) {
> + if (sb_readonly) {
> + return false;
> + }
> remove_additional_chassis(pb, chassis_rec);
> }
> update_tracked = true;
>
> + if (!notify_up) {
> + if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
> + return false;
> + }
> + if (sb_readonly) {
> + return false;
> + }
> + set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> + } else {
> + if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> + sb_readonly);
> + }
> register_claim_timestamp(pb->logical_port, now);
> sset_find_and_delete(postponed_ports, pb->logical_port);
> + } else {
> + if (!notify_up) {
> + if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
> + return false;
> + }
> + } else {
> + if (pb->n_up && !pb->up[0]) {
> + if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> + sb_readonly);
> + }
> + }
> }
> } else if (can_bind == CAN_BIND_AS_ADDITIONAL) {
> if (!is_additional_chassis(pb, chassis_rec)) {
> @@ -1171,7 +1252,8 @@ claim_lport(const struct sbrec_port_binding *pb,
> */
> static bool
> release_lport_main_chassis(const struct sbrec_port_binding *pb,
> - bool sb_readonly)
> + bool sb_readonly,
> + struct if_status_mgr *if_mgr)
> {
> if (pb->encap) {
> if (sb_readonly) {
> @@ -1180,11 +1262,14 @@ release_lport_main_chassis(const struct
sbrec_port_binding *pb,
> sbrec_port_binding_set_encap(pb, NULL);
> }
>
> + /* If sb is readonly, pb->chassis is unset through if-status if
present. */
> +
> if (pb->chassis) {
> - if (sb_readonly) {
> + if (!sb_readonly) {
> + sbrec_port_binding_set_chassis(pb, NULL);
> + } else if (!if_status_mgr_iface_is_present(if_mgr,
pb->logical_port)) {
> return false;
> }
> - sbrec_port_binding_set_chassis(pb, NULL);
> }
>
> if (pb->virtual_parent) {
> @@ -1194,7 +1279,9 @@ release_lport_main_chassis(const struct
sbrec_port_binding *pb,
> sbrec_port_binding_set_virtual_parent(pb, NULL);
> }
>
> - VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
> + VLOG_INFO("Releasing lport %s from this chassis (sb_readonly=%d)",
> + pb->logical_port, sb_readonly);
> +
> return true;
> }
>
> @@ -1228,7 +1315,7 @@ release_lport(const struct sbrec_port_binding *pb,
> struct hmap *tracked_datapaths, struct if_status_mgr
*if_mgr)
> {
> if (pb->chassis == chassis_rec) {
> - if (!release_lport_main_chassis(pb, sb_readonly)) {
> + if (!release_lport_main_chassis(pb, sb_readonly, if_mgr)) {
> return false;
> }
> } else if (is_additional_chassis(pb, chassis_rec)) {
> @@ -1567,7 +1654,8 @@ consider_localport(const struct sbrec_port_binding
*pb,
> enum can_bind can_bind = lport_can_bind_on_this_chassis(
> b_ctx_in->chassis_rec, pb);
> if (can_bind == CAN_BIND_AS_MAIN) {
> - if (!release_lport_main_chassis(pb, !b_ctx_in->ovnsb_idl_txn)) {
> + if (!release_lport_main_chassis(pb, !b_ctx_in->ovnsb_idl_txn,
> + b_ctx_out->if_mgr)) {
> return false;
> }
> } else if (can_bind == CAN_BIND_AS_ADDITIONAL) {
> diff --git a/controller/binding.h b/controller/binding.h
> index b2360bac2..ad959a9e6 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -153,8 +153,11 @@ const struct sbrec_port_binding
*local_binding_get_primary_pb(
> 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_down(struct shash *local_bindings, const char
*pb_name);
> +bool local_binding_is_up(struct shash *local_bindings, const char
*pb_name,
> + const struct sbrec_chassis *);
> +bool local_binding_is_down(struct shash *local_bindings, const char
*pb_name,
> + const struct sbrec_chassis *);
> +
> void local_binding_set_up(struct shash *local_bindings, const char
*pb_name,
> const struct sbrec_chassis *chassis_rec,
> const char *ts_now_str, bool sb_readonly,
> @@ -162,6 +165,13 @@ void local_binding_set_up(struct shash
*local_bindings, const char *pb_name,
> void local_binding_set_down(struct shash *local_bindings, const char
*pb_name,
> const struct sbrec_chassis *chassis_rec,
> bool sb_readonly, bool ovs_readonly);
> +void local_binding_set_pb(struct shash *local_bindings, const char
*pb_name,
> + const struct sbrec_chassis *chassis_rec,
> + struct hmap *tracked_datapaths,
> + bool is_set);
> +bool local_bindings_pb_chassis_is_set(struct shash *local_bindings,
> + const char *pb_name,
> + const struct sbrec_chassis
*chassis_rec);
>
> void binding_register_ovs_idl(struct ovsdb_idl *);
> void binding_run(struct binding_ctx_in *, struct binding_ctx_out *);
> @@ -180,6 +190,10 @@ void binding_dump_local_bindings(struct
local_binding_data *, struct ds *);
> bool is_additional_chassis(const struct sbrec_port_binding *pb,
> const struct sbrec_chassis *chassis_rec);
>
> +void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
> + const struct sbrec_chassis *chassis_rec,
> + bool is_set);
> +
> /* Corresponds to each Port_Binding.type. */
> enum en_lport_type {
> LP_UNKNOWN,
> diff --git a/controller/if-status.c b/controller/if-status.c
> index ad61844d8..2590ec27f 100644
> --- a/controller/if-status.c
> +++ b/controller/if-status.c
> @@ -24,6 +24,7 @@
> #include "lib/util.h"
> #include "timeval.h"
> #include "openvswitch/vlog.h"
> +#include "lib/ovn-sb-idl.h"
>
> VLOG_DEFINE_THIS_MODULE(if_status);
>
> @@ -53,9 +54,11 @@ VLOG_DEFINE_THIS_MODULE(if_status);
> */
>
> enum if_state {
> - OIF_CLAIMED, /* Newly claimed interface. */
> - OIF_INSTALL_FLOWS, /* Already claimed interface for which flows are
still
> - * being installed.
> + OIF_CLAIMED, /* Newly claimed interface. pb->chassis not yet
updated.
> + */
> + OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis successfully
> + * updated in SB and 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
> @@ -78,6 +81,63 @@ static const char *if_state_names[] = {
> [OIF_INSTALLED] = "INSTALLED",
> };
>
> +/*
> + * +----------------------+
> + * +---> | |
> + * | +-> | NULL |
<--------------------------------------+++-+
> + * | | +----------------------+
|
> + * | | ^ release_iface | claim_iface()
|
> + * | | | V - sbrec_update_chassis(if sb is rw)
|
> + * | | +----------------------+
|
> + * | | | |
<----------------------------------------+ |
> + * | | | CLAIMED |
<--------------------------------------+ | |
> + * | | +----------------------+
| | |
> + * | | | V ^
| | |
> + * | | | | | handle_claims()
| | |
> + * | | | | | - sbrec_update_chassis(if sb is rw)
| | |
> + * | | | +--+
| | |
> + * | | |
| | |
> + * | | | mgr_update(when sb is rw i.e. pb->chassis)
| | |
> + * | | | has been updated
| | |
> + * | | release_iface | - request seqno
| | |
> + * | | |
| | |
> + * | | V
| | |
> + * | | +----------------------+
| | |
> + * | +-- | | mgr_run(seqno not rcvd)
| | |
> + * | | INSTALL_FLOWS | - set port down in sb
| | |
> + * | | | - remove ovn-installed from ovsdb
| | |
> + * | | | mgr_update()
| | |
> + * | +----------------------+ - sbrec_update_chassis if needed
| | |
> + * | |
| | |
> + * | | mgr_run(seqno rcvd)
| | |
> + * | | - set port up in sb
| | |
> + * | release_iface | - set ovn-installed in ovs
| | |
> + * | V
| | |
> + * | +----------------------+
| | |
> + * | | | mgr_run()
| | |
> + * +-- | MARK_UP | - set port up in sb
| | |
> + * | | - set ovn-installed in ovs
| | |
> + * | | mgr_update()
| | |
> + * +----------------------+ - sbrec_update_chassis if needed
| | |
> + * |
| | |
> + * | mgr_update(rcvd port up / ovn_installed & chassis set)
| | |
> + * V
| | |
> + * +----------------------+
| | |
> + * | INSTALLED | ------------> claim_iface
---------------+ | |
> + * +----------------------+
| |
> + * |
| |
> + * | release_iface
| |
> + * V
| |
> + * +----------------------+
| |
> + * | | ------------> claim_iface
-----------------+ |
> + * | MARK_DOWN | ------> mgr_update(rcvd port down)
----------+
> + * | | mgr_run()
> + * | | - set port down in sb
> + * | | mgr_update()
> + * +----------------------+ - sbrec_update_chassis(NULL)
> + */
> +
> +
> struct ovs_iface {
> char *id; /* Extracted from OVS external_ids.iface_id.
*/
> enum if_state state; /* State of the interface in the state
machine. */
> @@ -158,14 +218,22 @@ if_status_mgr_destroy(struct if_status_mgr *mgr)
> }
>
> void
> -if_status_mgr_claim_iface(struct if_status_mgr *mgr, const char
*iface_id)
> +if_status_mgr_claim_iface(struct if_status_mgr *mgr,
> + const struct sbrec_port_binding *pb,
> + const struct sbrec_chassis *chassis_rec,
> + bool sb_readonly)
> {
> + const char *iface_id = pb->logical_port;
> struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
>
> if (!iface) {
> iface = ovs_iface_create(mgr, iface_id, OIF_CLAIMED);
> }
>
> + if (!sb_readonly) {
> + set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> + }
> +
> switch (iface->state) {
> case OIF_CLAIMED:
> case OIF_INSTALL_FLOWS:
> @@ -182,6 +250,12 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
const char *iface_id)
> }
> }
>
> +bool
> +if_status_mgr_iface_is_present(struct if_status_mgr *mgr, const char
*iface_id)
> +{
> + return !!shash_find_data(&mgr->ifaces, iface_id);
> +}
> +
> void
> if_status_mgr_release_iface(struct if_status_mgr *mgr, const char
*iface_id)
> {
> @@ -246,9 +320,36 @@ if_status_mgr_delete_iface(struct if_status_mgr
*mgr, const char *iface_id)
> }
> }
>
> +bool
> +if_status_handle_claims(struct if_status_mgr *mgr,
> + struct local_binding_data *binding_data,
> + const struct sbrec_chassis *chassis_rec,
> + struct hmap *tracked_datapath,
> + bool sb_readonly)
> +{
> + if (!binding_data || sb_readonly) {
> + return false;
> + }
> +
> + struct shash *bindings = &binding_data->bindings;
> + struct hmapx_node *node;
> +
> + bool rc = false;
> + HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> + struct ovs_iface *iface = node->data;
> + VLOG_INFO("if_status_handle_claims for %s", iface->id);
> + local_binding_set_pb(bindings, iface->id, chassis_rec,
> + tracked_datapath, true);
> + rc = true;
> + }
> + return rc;
> +}
> +
> void
> if_status_mgr_update(struct if_status_mgr *mgr,
> - struct local_binding_data *binding_data)
> + struct local_binding_data *binding_data,
> + const struct sbrec_chassis *chassis_rec,
> + bool sb_readonly)
> {
> if (!binding_data) {
> return;
> @@ -257,13 +358,27 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> struct shash *bindings = &binding_data->bindings;
> struct hmapx_node *node;
>
> + /* Interfaces in OIF_MARK_UP/INSTALL_FLOWS state have already set
their
> + * pb->chassis. However, the update might still be in fly
(confirmation
> + * not received yet) or pb->chassis was overwitten by another
chassis.
> + */
> +
> /* Move all interfaces that have been confirmed "up" by the binding
module,
> * from OIF_MARK_UP to OIF_INSTALLED.
> */
> HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_UP]) {
> struct ovs_iface *iface = node->data;
>
> - if (local_binding_is_up(bindings, iface->id)) {
> + if (!local_bindings_pb_chassis_is_set(bindings, iface->id,
> + chassis_rec)) {
> + if (!sb_readonly) {
> + local_binding_set_pb(bindings, iface->id, chassis_rec,
> + NULL, true);
> + } else {
> + continue;
> + }
> + }
> + if (local_binding_is_up(bindings, iface->id, chassis_rec)) {
> ovs_iface_set_state(mgr, iface, OIF_INSTALLED);
> }
> }
> @@ -274,26 +389,56 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) {
> struct ovs_iface *iface = node->data;
>
> - if (local_binding_is_down(bindings, iface->id)) {
> + if (!sb_readonly) {
> + local_binding_set_pb(bindings, iface->id, chassis_rec,
> + NULL, false);
> + }
> + if (local_binding_is_down(bindings, iface->id, chassis_rec)) {
> 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.
> + /* Update pb->chassis in case it's not set (previous update still in
fly
> + * or pb->chassis was overwitten by another chassis.
> */
> - bool new_ifaces = false;
> - HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> - struct ovs_iface *iface = node->data;
> + if (!sb_readonly) {
> + HMAPX_FOR_EACH_SAFE (node,
&mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) {
> + struct ovs_iface *iface = node->data;
> +
> + if (!local_bindings_pb_chassis_is_set(bindings, iface->id,
> + chassis_rec)) {
> + local_binding_set_pb(bindings, iface->id, chassis_rec,
> + NULL, true);
> + }
> + }
> + }
>
> - ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
> - iface->install_seqno = mgr->iface_seqno + 1;
> - new_ifaces = true;
> + /* Move newly claimed interfaces from OIF_CLAIMED to
OIF_INSTALL_FLOWS.
> + */
> + bool new_ifaces = false;
> + if (!sb_readonly) {
> + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> + struct ovs_iface *iface = node->data;
> + /* No need to to update pb->chassis as already done
> + * in if_status_handle_claims or if_status_mgr_claim_iface
> + */
> + ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
> + iface->install_seqno = mgr->iface_seqno + 1;
> + new_ifaces = true;
> + }
> + } else {
> + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> + struct ovs_iface *iface = node->data;
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
1);
> + VLOG_INFO_RL(&rl,
> + "Not updating pb chassis for %s now as "
> + "sb is readonly", iface->id);
> + }
> }
>
> - /* Request a seqno update when the flows for new interfaces have been
> + /* Register for a notification about flows being installed in OVS
for all
> + * newly claimed interfaces for which pb->chassis has been updated.
> + * Request a seqno update when the flows for new interfaces have been
> * installed in OVS.
> */
> if (new_ifaces) {
> @@ -403,7 +548,7 @@ if_status_mgr_update_bindings(struct if_status_mgr
*mgr,
> 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.
> + * in the process of being installed in OVS, i.e., are not yet
installed.
> */
> HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) {
> struct ovs_iface *iface = node->data;
> diff --git a/controller/if-status.h b/controller/if-status.h
> index bb8a3950d..5bd187a25 100644
> --- a/controller/if-status.h
> +++ b/controller/if-status.h
> @@ -26,16 +26,27 @@ struct simap;
> 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_claim_iface(struct if_status_mgr *,
> + const struct sbrec_port_binding *pb,
> + const struct sbrec_chassis *chassis_rec,
> + bool sb_readonly);
> 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_update(struct if_status_mgr *, struct
local_binding_data *,
> + const struct sbrec_chassis *chassis,
> + bool sb_readonly);
> void if_status_mgr_run(struct if_status_mgr *mgr, struct
local_binding_data *,
> const struct sbrec_chassis *,
> bool sb_readonly, bool ovs_readonly);
> void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr,
> struct simap *usage);
> +bool if_status_mgr_iface_is_present(struct if_status_mgr *mgr,
> + const char *iface_id);
> +bool if_status_handle_claims(struct if_status_mgr *mgr,
> + struct local_binding_data *binding_data,
> + const struct sbrec_chassis *chassis_rec,
> + struct hmap *tracked_datapath,
> + bool sb_readonly);
>
> # endif /* controller/if-status.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c97744d57..b2b2a4434 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1509,6 +1509,73 @@ en_runtime_data_run(struct engine_node *node, void
*data)
> engine_set_node_state(node, EN_UPDATED);
> }
>
> +struct ed_type_sb_ro {
> + bool sb_readonly;
> +};
> +
> +static void *
> +en_sb_ro_init(struct engine_node *node OVS_UNUSED,
> + struct engine_arg *arg OVS_UNUSED)
> +{
> + struct ed_type_sb_ro *data = xzalloc(sizeof *data);
> + return data;
> +}
> +
> +static void
> +en_sb_ro_run(struct engine_node *node, void *data)
> +{
> + struct ed_type_sb_ro *sb_ro_data = data;
> + bool sb_readonly = !engine_get_context()->ovnsb_idl_txn;
> + if (sb_ro_data->sb_readonly != sb_readonly) {
> + sb_ro_data->sb_readonly = sb_readonly;
> + if (!sb_ro_data->sb_readonly) {
> + engine_set_node_state(node, EN_UPDATED);
> + }
> + }
> +}
> +
> +static void
> +en_sb_ro_cleanup(void *data OVS_UNUSED)
> +{
> +}
> +
> +static bool
> +runtime_data_sb_ro_handler(struct engine_node *node, void *data)
> +{
> + const struct sbrec_chassis *chassis = NULL;
> +
> + struct ovsrec_open_vswitch_table *ovs_table =
> + (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
> + engine_get_input("OVS_open_vswitch", node));
> +
> + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> +
> + struct ovsdb_idl_index *sbrec_chassis_by_name =
> + engine_ovsdb_node_get_index(
> + engine_get_input("SB_chassis", node),
> + "name");
> +
> + if (chassis_id) {
> + chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
chassis_id);
> + }
> + if (chassis) {
> + struct ed_type_runtime_data *rt_data = data;
> + bool sb_readonly = !engine_get_context()->ovnsb_idl_txn;
> + struct controller_engine_ctx *ctrl_ctx =
> + engine_get_context()->client_ctx;
> +
> + if (if_status_handle_claims(ctrl_ctx->if_mgr,
> + &rt_data->lbinding_data,
> + chassis,
> + &rt_data->tracked_dp_bindings,
> + sb_readonly)) {
> + engine_set_node_state(node, EN_UPDATED);
> + rt_data->tracked = true;
> + }
> + }
> + return true;
> +}
> +
> static bool
> runtime_data_ovs_interface_shadow_handler(struct engine_node *node, void
*data)
> {
> @@ -3592,6 +3659,7 @@ main(int argc, char *argv[])
> stopwatch_create(VIF_PLUG_RUN_STOPWATCH_NAME, SW_MS);
>
> /* Define inc-proc-engine nodes. */
> + ENGINE_NODE(sb_ro, "sb_ro");
> ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones");
> ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow,
> "ovs_interface_shadow");
> @@ -3729,6 +3797,7 @@ main(int argc, char *argv[])
> engine_add_input(&en_ovs_interface_shadow, &en_ovs_interface,
> ovs_interface_shadow_ovs_interface_handler);
>
> + engine_add_input(&en_runtime_data, &en_sb_ro,
runtime_data_sb_ro_handler);
> engine_add_input(&en_runtime_data, &en_ofctrl_is_connected, NULL);
>
> engine_add_input(&en_runtime_data, &en_ovs_open_vswitch, NULL);
> @@ -4166,7 +4235,8 @@ main(int argc, char *argv[])
> runtime_data ? &runtime_data->lbinding_data :
NULL;
> stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
> time_msec());
> - if_status_mgr_update(if_mgr, binding_data);
> + if_status_mgr_update(if_mgr, binding_data, chassis,
> + !ovnsb_idl_txn);
> stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
> time_msec());
>
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index 2ba930a85..a3c7f8125 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -814,3 +814,14 @@ m4_define([OVN_FOR_EACH_NORTHD],
> [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no],
> [m4_foreach([OVN_MONITOR_ALL], [yes, no], [$1
> ])])])])
> +
> +# OVN_NBCTL(NBCTL_COMMAND) adds NBCTL_COMMAND to list of commands to be
run by RUN_OVN_NBCTL().
> +m4_define([OVN_NBCTL], [
> + command="${command} -- $1"
> +])
> +
> +# RUN_OVN_NBCTL() executes list of commands built by the OVN_NBCTL()
macro.
> +m4_define([RUN_OVN_NBCTL], [
> + check ovn-nbctl ${command}
> + unset command
> +])
> diff --git a/tests/ovn.at b/tests/ovn.at
> index bbad0f194..d0ea87952 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -130,6 +130,8 @@ m4_define([OVN_WAIT_PATCH_PORT_FLOWS],
> m4_define([OVN_WAIT_REMOTE_OUTPUT_FLOWS],
> [ovn_wait_remote_output_flows "$1" "$2" "__file__:__line__"])
>
> +m4_define([OVN_WAIT_REMOTE_INPUT_FLOWS],
> + [ovn_wait_remote_input_flows "$1" "$2" "__file__:__line__"])
>
> AT_BANNER([OVN components])
>
> @@ -13936,6 +13938,11 @@ wait_column "$hv1_uuid" Port_Binding
requested_chassis logical_port=lsp0
> wait_column "$hv2_uuid" Port_Binding additional_chassis logical_port=lsp0
> wait_column "$hv2_uuid" Port_Binding requested_additional_chassis
logical_port=lsp0
>
> +# Check ovn-installed updated for main chassis
> +wait_for_ports_up
> +OVS_WAIT_UNTIL([test `as hv1 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = '"true"'])
> +OVS_WAIT_UNTIL([test x`as hv2 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = x])
> +
> # Check that setting iface:encap-ip populates
Port_Binding:additional_encap
> wait_row_count Encap 2 chassis_name=hv1
> wait_row_count Encap 2 chassis_name=hv2
> @@ -13961,6 +13968,11 @@ wait_column "$hv2_uuid" Port_Binding
requested_chassis logical_port=lsp0
> wait_column "" Port_Binding additional_chassis logical_port=lsp0
> wait_column "" Port_Binding requested_additional_chassis
logical_port=lsp0
>
> +# Check ovn-installed updated for main chassis and not for other chassis
> +wait_for_ports_up
> +OVS_WAIT_UNTIL([test `as hv2 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = '"true"'])
> +OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface lsp0
external_ids:ovn-installed` = x])
> +
> # Check that additional_encap is cleared
> wait_column "" Port_Binding additional_encap logical_port=lsp0
>
> @@ -15259,7 +15271,9 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int
table=65 | grep actions=output:1],
> echo "verifying that lsp0 binding moves when requested-chassis is
changed"
>
> ovn-nbctl lsp-set-options lsp0 requested-chassis=hv2
> -OVS_WAIT_UNTIL([test 1 = $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> +# We might see multiple "Releasing lport ...", when sb is read only
> +OVS_WAIT_UNTIL([test 1 -le $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> +
> wait_column "$hv2_uuid" Port_Binding chassis logical_port=lsp0
>
> # (6) Chassis hv2 should add flows and hv1 should not.
> @@ -15346,7 +15360,7 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int
table=0 | grep in_port=1], [0], [ig
> AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=65 | grep
actions=output:1], [0], [ignore])
>
> check ovn-nbctl --wait=hv lsp-set-options lsp0
requested-chassis=non-existant-chassis
> -OVS_WAIT_UNTIL([test 1 = $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> +OVS_WAIT_UNTIL([test 1 -le $(grep -c "Releasing lport lsp0 from this
chassis" hv1/ovn-controller.log)])
> check ovn-nbctl --wait=hv sync
> wait_column '' Port_Binding chasssi logical_port=lsp0
> AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep in_port=1],
[1], [])
> @@ -32517,3 +32531,119 @@ OVS_WAIT_UNTIL([
> OVN_CLEANUP([hv1], [hv2])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([recomputes])
> +ovn_start
> +
> +n_hv=4
> +
> +# Add chassis
> +net_add n1
> +for i in $(seq 1 $n_hv); do
> + sim_add hv$i
> + as hv$i
> + check ovs-vsctl add-br br-phys
> + ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> + ovn_attach n1 br-phys 192.168.0.$i 24 geneve
> +done
> +
> +add_switch_ports() {
> + start_port=$1
> + end_port=$2
> + nb_hv=$3
> + bulk_size=$4
> + for ((i=start_port; i<end_port; )) do
> + start_bulk=$i
> + for hv in $(seq 1 $nb_hv); do
> + end_bulk=$((start_bulk+bulk_size-1))
> + for port in $(seq $start_bulk $end_bulk); do
> + logical_switch_port=lsp${port}
> + OVN_NBCTL(lsp-add ls1 $logical_switch_port)
> + OVN_NBCTL(lsp-set-addresses $logical_switch_port dynamic)
> + done
> + start_bulk=$((end_bulk+1))
> + done
> + RUN_OVN_NBCTL()
> +
> + start_bulk=$i
> + for hv in $(seq 1 $nb_hv); do
> + end_bulk=$((start_bulk+bulk_size-1))
> + for port in $(seq $start_bulk $end_bulk); do
> + logical_switch_port=lsp${port}
> + as hv$hv ovs-vsctl \
> + --no-wait -- add-port br-int vif${port} \
> + -- set Interface vif${port}
external_ids:iface-id=$logical_switch_port
> + done
> + start_bulk=$((end_bulk+1))
> + done
> + i=$((end_bulk+1))
> + done
> +}
> +check ovn-nbctl ls-add ls1
> +check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16
> +check ovn-nbctl set Logical_Switch ls1
other_config:exclude_ips=10.1.255.254
> +
> +check ovn-nbctl lr-add lr1
> +check ovn-nbctl lsp-add ls1 lsp0 -- set Logical_Switch_Port lsp0
type=router options:router-port=lrp0 addresses=dynamic
> +check ovn-nbctl lrp-add lr1 lrp0 "f0:00:00:01:00:01" 10.1.255.254/16
> +check ovn-nbctl lr-nat-add lr1 snat 10.2.0.1 10.1.0.0/16
> +
> +lflow_run=0
> +check ovn-nbctl --wait=hv sync
> +
> +# Tunnel ports might not be added (yet) at this point on slow system.
> +# Wait for flows related to such ports to ensure those ports have been
added
> +# before we measure recomputes. Otherwise, ovs_interface handler might
be run
> +# afterwards for tunnel ports, causing recomputes.
> +for i in $(seq 1 $n_hv); do
> + for j in $(seq 1 $n_hv); do
> + if test $i != $j; then
> + OVN_WAIT_REMOTE_INPUT_FLOWS(["hv$i"],["hv$j"])
> + fi
> + done
> +done
> +
> +for i in $(seq 1 $n_hv); do
> + as hv$i
> + lflow_run1=$(ovn-appctl -t ovn-controller coverage/read-counter
lflow_run)
> + lflow_run=`expr $lflow_run1 + $lflow_run`
> +done
> +
> +add_switch_ports 1 1000 $n_hv 5
> +
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +for i in $(seq 1 $n_hv); do
> + pid=$(cat hv${i}/ovn-controller.pid)
> + u=$((u+$(cat "/proc/$pid/stat" | awk '{print $14}')))
> + s=$((s+$(cat "/proc/$pid/stat" | awk '{print $15}')))
> +done
> +
> +n_pid=$(cat northd/ovn-northd.pid)
> +n_u=$(cat "/proc/$pid/stat" | awk '{print $14}')
> +n_s=$(cat "/proc/$pid/stat" | awk '{print $15}')
> +
> +echo "Total Northd User Time: $n_u"
> +echo "Total Northd System Time: $n_s"
> +echo "Total Controller User Time: $u"
> +echo "Total Controller System Time: $s"
> +
> +lflow_run_end=0
> +for i in $(seq 1 $n_hv); do
> + as hv$i
> + lflow_run1=$(ovn-appctl -t ovn-controller coverage/read-counter
lflow_run)
> + lflow_run_end=`expr $lflow_run1 + $lflow_run_end`
> +done
> +n_recomputes=`expr $lflow_run_end - $lflow_run`
> +echo "$n_recomputes recomputes"
> +
> +AT_CHECK([test $lflow_run_end == $lflow_run])
> +
> +for i in $(seq 2 $n_hv); do
> + OVN_CLEANUP_SBOX([hv$i])
> +done
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> diff --git a/tests/perf-northd.at b/tests/perf-northd.at
> index 74b69e9d4..ca115dadc 100644
> --- a/tests/perf-northd.at
> +++ b/tests/perf-northd.at
> @@ -76,23 +76,6 @@ m4_define([PERF_RECORD_STOP], [
> PERF_RECORD_STOPWATCH(ovn-northd-loop, ["Short term average"],
[Average (northd-loop in msec)])
> ])
>
> -# OVN_NBCTL([NBCTL_COMMAND])
> -#
> -# Add NBCTL_COMMAND to list of commands to be run by RUN_OVN_NBCTL().
> -#
> -m4_define([OVN_NBCTL], [
> - command="${command} -- $1"
> -])
> -
> -# RUN_OVN_NBCTL()
> -#
> -# Execute list of commands built by the OVN_NBCTL() macro.
> -#
> -m4_define([RUN_OVN_NBCTL], [
> - check ovn-nbctl ${command}
> - unset command
> -])
> -
> OVS_START_SHELL_HELPERS
> generate_subnet () {
> local a=$(printf %d $(expr $1 / 256 + 10))
> @@ -208,4 +191,4 @@ BUILD_NBDB(OVN_BASIC_SCALE_CONFIG(500, 50))
>
> PERF_RECORD_STOP()
> AT_CLEANUP
> -])
> \ No newline at end of file
> +])
> --
> 2.31.1
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
