The virtual port binding requests were commited twice in case there
was a failure in between them. That unfortunately doesn't solve the
issue for very rare case when the commit fails multiple times in
a row. To make sure we remove only really commited requests use
the next_cfg and cur_cfg mechanism of idl_loop. We can store
the cur_cfg and check against the next loop cur_cfg to see
if the commit was successful. This mechanism is exposed to
the whole pinctrl so it can be used by any other SB transaction
if needed.
Signed-off-by: Ales Musil <amu...@redhat.com>
---
controller/ovn-controller.c | 16 ++++++-
controller/pinctrl.c | 87 +++++++++++++------------------------
controller/pinctrl.h | 3 +-
3 files changed, 47 insertions(+), 59 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 440c76ca1..a42e21c5c 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -6326,7 +6326,8 @@ main(int argc, char *argv[])
&runtime_data->local_active_ports_ipv6_pd,
&runtime_data->local_active_ports_ras,
ovsrec_open_vswitch_table_get(
- ovs_idl_loop.idl));
+ ovs_idl_loop.idl),
+ ovnsb_idl_loop.cur_cfg);
stopwatch_stop(PINCTRL_RUN_STOPWATCH_NAME,
time_msec());
mirror_run(ovs_idl_txn,
@@ -6490,9 +6491,20 @@ main(int argc, char *argv[])
poll_immediate_wake();
}
- if (!ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop)) {
+ int ovnsb_txn_status = ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
+ if (!ovnsb_txn_status) {
VLOG_INFO("OVNSB commit failed, force recompute next time.");
engine_set_force_recompute_immediate();
+ } else if (ovnsb_txn_status == 1) {
+ if (ovnsb_idl_loop.next_cfg == INT64_MAX) {
+ ovnsb_idl_loop.next_cfg = 0;
+ } else {
+ ovnsb_idl_loop.next_cfg++;
+ }
+ } else if (ovnsb_txn_status == -1) {
+ /* The commit is still in progress */
+ } else {
+ OVS_NOT_REACHED();
}
int ovs_txn_status = ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop);
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index bdb619b4d..436e5d487 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -184,6 +184,7 @@ struct pinctrl {
static struct pinctrl pinctrl;
+static bool pinctrl_is_sb_commited(int64_t commit_cfg, int64_t cur_cfg);
static void init_buffered_packets_ctx(void);
static void destroy_buffered_packets_ctx(void);
static void
@@ -321,7 +322,7 @@ static void run_put_vport_bindings(
struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
- const struct sbrec_chassis *chassis)
+ const struct sbrec_chassis *chassis, int64_t cur_cfg)
OVS_REQUIRES(pinctrl_mutex);
static void wait_put_vport_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
static void pinctrl_handle_bind_vport(const struct flow *md,
@@ -4084,14 +4085,15 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
const struct sset *active_tunnels,
const struct shash *local_active_ports_ipv6_pd,
const struct shash *local_active_ports_ras,
- const struct ovsrec_open_vswitch_table *ovs_table)
+ const struct ovsrec_open_vswitch_table *ovs_table,
+ int64_t cur_cfg)
{
ovs_mutex_lock(&pinctrl_mutex);
run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key,
sbrec_mac_binding_by_lport_ip);
run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
- sbrec_port_binding_by_key, chassis);
+ sbrec_port_binding_by_key, chassis, cur_cfg);
send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
sbrec_port_binding_by_name,
sbrec_mac_binding_by_lport_ip, ecmp_nh_table,
@@ -4629,6 +4631,15 @@ pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn)
ovs_mutex_unlock(&pinctrl_mutex);
}
+#define PINCTRL_CFG_INTERVAL 100
+
+static bool
+pinctrl_is_sb_commited(int64_t commit_cfg, int64_t cur_cfg)
+{
+ return (INT64_MAX - commit_cfg < PINCTRL_CFG_INTERVAL &&
+ cur_cfg < PINCTRL_CFG_INTERVAL) || cur_cfg > commit_cfg;
+}
+
/* Called by ovn-controller. */
void
pinctrl_destroy(void)
@@ -7280,45 +7291,13 @@ struct put_vport_binding {
uint32_t vport_parent_key;
- /* This vport record Only relevant if "new_record" is true. */
- bool new_record;
+ /* Current commit cfg number. */
+ int64_t cfg;
};
/* Contains "struct put_vport_binding"s. */
static struct hmap put_vport_bindings;
-/*
- * Validate if the vport_binding record that was added
- * by the pinctrl thread is still relevant and needs
- * to be updated in the SBDB or not.
- *
- * vport_binding record is only relevant and needs to be updated in SB if:
- * 2. The put_vport_binding:new_record is true:
- * The new_record will be set to "true" when this vport record is created
- * by function "pinctrl_handle_bind_vport".
- *
- * After the first attempt to bind this vport to the chassis and
- * virtual_parent by function "run_put_vport_bindings" we will set the
- * value of vpb:new_record to "false" and keep it in "put_vport_bindings"
- *
- * After the second attempt of binding the vpb it will be removed by
- * this function.
- *
- * The above guarantees that we will try to bind the vport twice in
- * a certain amount of time.
- *
-*/
-static bool
-is_vport_binding_relevant(struct put_vport_binding *vpb)
-{
-
- if (vpb->new_record) {
- vpb->new_record = false;
- return true;
- }
- return false;
-}
-
static void
init_put_vport_bindings(void)
{
@@ -7326,21 +7305,13 @@ init_put_vport_bindings(void)
}
static void
-flush_put_vport_bindings(bool force_flush)
+destroy_put_vport_bindings(void)
{
struct put_vport_binding *vport_b;
HMAP_FOR_EACH_SAFE (vport_b, hmap_node, &put_vport_bindings) {
- if (!is_vport_binding_relevant(vport_b) || force_flush) {
- hmap_remove(&put_vport_bindings, &vport_b->hmap_node);
- free(vport_b);
- }
+ hmap_remove(&put_vport_bindings, &vport_b->hmap_node);
+ free(vport_b);
}
-}
-
-static void
-destroy_put_vport_bindings(void)
-{
- flush_put_vport_bindings(true);
hmap_destroy(&put_vport_bindings);
}
@@ -7405,20 +7376,24 @@ static void
run_put_vport_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
- const struct sbrec_chassis *chassis)
+ const struct sbrec_chassis *chassis, int64_t cur_cfg)
OVS_REQUIRES(pinctrl_mutex)
{
if (!ovnsb_idl_txn) {
return;
}
- const struct put_vport_binding *vpb;
- HMAP_FOR_EACH (vpb, hmap_node, &put_vport_bindings) {
- run_put_vport_binding(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
- sbrec_port_binding_by_key, chassis, vpb);
+ struct put_vport_binding *vpb;
+ HMAP_FOR_EACH_SAFE (vpb, hmap_node, &put_vport_bindings) {
+ if (vpb->cfg >= 0 && pinctrl_is_sb_commited(vpb->cfg, cur_cfg)) {
+ hmap_remove(&put_vport_bindings, &vpb->hmap_node);
+ free(vpb);
+ } else {
+ run_put_vport_binding(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
+ sbrec_port_binding_by_key, chassis, vpb);
+ vpb->cfg = cur_cfg;
+ }
}
-
- flush_put_vport_bindings(false);
}
/* Called with in the pinctrl_handler thread context. */
@@ -7456,7 +7431,7 @@ pinctrl_handle_bind_vport(
vpb->dp_key = dp_key;
vpb->vport_key = vport_key;
vpb->vport_parent_key = vport_parent_key;
- vpb->new_record = true;
+ vpb->cfg = -1;
notify_pinctrl_main();
}
diff --git a/controller/pinctrl.h b/controller/pinctrl.h
index e33d7cbf5..d3d5d8831 100644
--- a/controller/pinctrl.h
+++ b/controller/pinctrl.h
@@ -61,7 +61,8 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
const struct sset *active_tunnels,
const struct shash *local_active_ports_ipv6_pd,
const struct shash *local_active_ports_ras,
- const struct ovsrec_open_vswitch_table *ovs_table);
+ const struct ovsrec_open_vswitch_table *ovs_table,
+ int64_t cur_cfg);
void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn);
void pinctrl_destroy(void);
