Skip to content

Commit

Permalink
vif-plug: Avoid Transaction failures in ovsdb.
Browse files Browse the repository at this point in the history 
Transaction failures could occur as trying to plug multiple times
the same interface in ovsdb i.e.
"Transaction causes multiple rows in \"Interface\" table to have identical values".
This could for instance occur between the time the VIF is plug first and
an ofport is created.

Signed-off-by: Xavier Simonart <[email protected]>
Reviewed-by: Ales Musil <[email protected]>
Signed-off-by: Numan Siddique <[email protected]>
  • Loading branch information
@simonartxavier @numansiddique
simonartxavier authored and numansiddique committed Oct 12, 2024
1 parent ed3dc80 commit fcc2bf1
Show file tree
Hide file tree
Showing 6 changed files with 137 additions and 46 deletions.
2 changes: 1 addition & 1 deletion controller/binding.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2529,7 +2529,7 @@ is_iface_vif(const struct ovsrec_interface *iface_rec)
return true;
}

static bool
bool
is_iface_in_int_bridge(const struct ovsrec_interface *iface,
const struct ovsrec_bridge *br_int)
{
Expand Down
3 changes: 3 additions & 0 deletions controller/binding.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,4 +284,7 @@ void claimed_lport_set_up(const struct sbrec_port_binding *pb,
bool port_binding_is_up(const struct sbrec_chassis *chassis_rec,
const struct sbrec_port_binding_table *,
const struct uuid *pb_uuid);
bool is_iface_in_int_bridge(const struct ovsrec_interface *iface,
const struct ovsrec_bridge *br_int);

#endif /* controller/binding.h */
7 changes: 7 additions & 0 deletions controller/ovn-controller.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4868,6 +4868,9 @@ main(int argc, char *argv[])
struct ovsdb_idl_index *ovsrec_port_by_qos
= ovsdb_idl_index_create1(ovs_idl_loop.idl,
&ovsrec_port_col_qos);
struct ovsdb_idl_index *ovsrec_interface_by_name
= ovsdb_idl_index_create1(ovs_idl_loop.idl,
&ovsrec_interface_col_name);
struct ovsdb_idl_index *ovsrec_queue_by_external_ids
= ovsdb_idl_index_create1(ovs_idl_loop.idl,
&ovsrec_queue_col_external_ids);
Expand Down Expand Up @@ -5250,6 +5253,8 @@ main(int argc, char *argv[])
engine_ovsdb_node_add_index(&en_ovs_flow_sample_collector_set, "id",
ovsrec_flow_sample_collector_set_by_id);
engine_ovsdb_node_add_index(&en_ovs_port, "qos", ovsrec_port_by_qos);
engine_ovsdb_node_add_index(&en_ovs_interface, "name",
ovsrec_interface_by_name);
engine_ovsdb_node_add_index(&en_ovs_queue, "external_ids",
ovsrec_queue_by_external_ids);

Expand Down Expand Up @@ -5604,6 +5609,8 @@ main(int argc, char *argv[])
sbrec_port_binding_by_requested_chassis,
.ovsrec_port_by_interfaces =
ovsrec_port_by_interfaces,
.ovsrec_interface_by_name =
ovsrec_interface_by_name,
.ovs_table = ovs_table,
.br_int = br_int,
.iface_table =
Expand Down
117 changes: 72 additions & 45 deletions controller/vif-plug.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,22 @@ transact_update_port(const struct ovsrec_interface *iface_rec,
mtu_request);
}

static const struct ovsrec_interface *
iface_lookup_by_name(struct ovsdb_idl_index *ovsrec_interface_by_name,
char *name)
{
struct ovsrec_interface *iface = ovsrec_interface_index_init_row(
ovsrec_interface_by_name);
ovsrec_interface_set_name(iface, name);

const struct ovsrec_interface *retval
= ovsrec_interface_index_find(ovsrec_interface_by_name,
iface);

ovsrec_interface_index_destroy_row(iface);

return retval;
}

static bool
consider_unplug_iface(const struct ovsrec_interface *iface,
Expand Down Expand Up @@ -287,11 +303,10 @@ get_plug_mtu_request(const struct smap *lport_options)
}

static bool
consider_plug_lport_create__(const struct vif_plug_class *vif_plug,
const struct smap *iface_external_ids,
const struct sbrec_port_binding *pb,
struct vif_plug_ctx_in *vif_plug_ctx_in,
struct vif_plug_ctx_out *vif_plug_ctx_out)
consider_plug_lport__(const struct vif_plug_class *vif_plug,
const struct sbrec_port_binding *pb,
struct vif_plug_port_ctx **vif_plug_port_ctx_ptr,
struct vif_plug_ctx_in *vif_plug_ctx_in)
{
if (!vif_plug_ctx_in->chassis_rec || !vif_plug_ctx_in->br_int
|| !vif_plug_ctx_in->ovs_idl_txn) {
Expand All @@ -317,13 +332,22 @@ consider_plug_lport_create__(const struct vif_plug_class *vif_plug,
&vif_plug_port_ctx->vif_plug_port_ctx_out)) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
VLOG_INFO_RL(&rl,
"Not plugging lport %s on direction from VIF plug "
"provider.",
"Not plugging/updating lport %s on direction from VIF "
"plug provider.",
pb->logical_port);
destroy_port_ctx(vif_plug_port_ctx);
return true;
}

*vif_plug_port_ctx_ptr = vif_plug_port_ctx;
return true;
}
static bool
consider_plug_lport_create__(const struct smap *iface_external_ids,
const struct sbrec_port_binding *pb,
const struct vif_plug_port_ctx *vif_plug_port_ctx,
struct vif_plug_ctx_in *vif_plug_ctx_in,
struct vif_plug_ctx_out *vif_plug_ctx_out)
{
VLOG_INFO("Plugging port %s into %s for lport %s on this "
"chassis.",
vif_plug_port_ctx->vif_plug_port_ctx_out.name,
Expand All @@ -340,41 +364,12 @@ static bool
consider_plug_lport_update__(const struct vif_plug_class *vif_plug,
const struct smap *iface_external_ids,
const struct sbrec_port_binding *pb,
struct local_binding *lbinding,
const struct ovsrec_interface *iface_rec,
struct vif_plug_port_ctx *vif_plug_port_ctx,
struct vif_plug_ctx_in *vif_plug_ctx_in,
struct vif_plug_ctx_out *vif_plug_ctx_out)
{
if (!vif_plug_ctx_in->chassis_rec || !vif_plug_ctx_in->br_int
|| !vif_plug_ctx_in->ovs_idl_txn) {
/* Some of our prerequisites are not available, ask for a recompute. */
return false;
}
/* Our contract with the VIF plug provider is that vif_plug_port_finish
* will be called with vif_plug_port_ctx_in and vif_plug_port_ctx_out
* objects once the transaction commits.
*
* Since this happens asynchronously we need to allocate memory for
* and duplicate any database references so that they stay valid.
*
* The data is freed with a call to destroy_port_ctx after the
* transaction completes at the end of the ovn-controller main
* loop. */
struct vif_plug_port_ctx *vif_plug_port_ctx = build_port_ctx(
vif_plug, PLUG_OP_CREATE, vif_plug_ctx_in, pb, NULL, NULL);

if (!vif_plug_port_prepare(vif_plug,
&vif_plug_port_ctx->vif_plug_port_ctx_in,
&vif_plug_port_ctx->vif_plug_port_ctx_out)) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
VLOG_INFO_RL(&rl,
"Not updating lport %s on direction from VIF plug "
"provider.",
pb->logical_port);
destroy_port_ctx(vif_plug_port_ctx);
return true;
}

if (strcmp(lbinding->iface->name,
if (strcmp(iface_rec->name,
vif_plug_port_ctx->vif_plug_port_ctx_out.name)) {
VLOG_WARN("Attempt of incompatible change to existing "
"port detected, please recreate port: %s",
Expand All @@ -386,7 +381,7 @@ consider_plug_lport_update__(const struct vif_plug_class *vif_plug,
return false;
}
VLOG_DBG("updating iface for: %s", pb->logical_port);
transact_update_port(lbinding->iface, vif_plug_ctx_in, vif_plug_ctx_out,
transact_update_port(iface_rec, vif_plug_ctx_in, vif_plug_ctx_out,
vif_plug_port_ctx, iface_external_ids,
get_plug_mtu_request(&pb->options));

Expand Down Expand Up @@ -438,13 +433,45 @@ consider_plug_lport(const struct sbrec_port_binding *pb,
UUID_ARGS(&lbinding->iface->header_.uuid));
return false;
}
struct vif_plug_port_ctx *vif_plug_port_ctx = NULL;
if (!consider_plug_lport__(vif_plug, pb, &vif_plug_port_ctx,
vif_plug_ctx_in)) {
return false;
}

ret = consider_plug_lport_update__(vif_plug, &iface_external_ids,
pb, lbinding, vif_plug_ctx_in,
pb, lbinding->iface,
vif_plug_port_ctx,
vif_plug_ctx_in,
vif_plug_ctx_out);
} else {
ret = consider_plug_lport_create__(vif_plug, &iface_external_ids,
pb, vif_plug_ctx_in,
vif_plug_ctx_out);
struct vif_plug_port_ctx *vif_plug_port_ctx = NULL;
if (!consider_plug_lport__(vif_plug, pb, &vif_plug_port_ctx,
vif_plug_ctx_in)) {
return false;
}

const struct ovsrec_interface *iface_rec =
iface_lookup_by_name(vif_plug_ctx_in->ovsrec_interface_by_name,
vif_plug_port_ctx->vif_plug_port_ctx_out.name);
if (iface_rec &&
smap_get(&iface_rec->external_ids, "iface-id") &&
smap_get(&iface_rec->external_ids,
OVN_PLUGGED_EXT_ID) &&
is_iface_in_int_bridge(iface_rec, vif_plug_ctx_in->br_int)) {

ret = consider_plug_lport_update__(vif_plug,
&iface_external_ids,
pb, iface_rec,
vif_plug_port_ctx,
vif_plug_ctx_in,
vif_plug_ctx_out);
} else {
ret = consider_plug_lport_create__(&iface_external_ids, pb,
vif_plug_port_ctx,
vif_plug_ctx_in,
vif_plug_ctx_out);
}
}
}

Expand Down
1 change: 1 addition & 0 deletions controller/vif-plug.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ struct vif_plug_ctx_in {
struct ovsdb_idl_index *sbrec_port_binding_by_name;
struct ovsdb_idl_index *sbrec_port_binding_by_requested_chassis;
struct ovsdb_idl_index *ovsrec_port_by_interfaces;
struct ovsdb_idl_index *ovsrec_interface_by_name;
const struct ovsrec_open_vswitch_table *ovs_table;
const struct ovsrec_bridge *br_int;
const struct ovsrec_interface_table *iface_table;
Expand Down
53 changes: 53 additions & 0 deletions tests/ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39180,3 +39180,56 @@ OVN_CLEANUP([hv1],[hv2])

AT_CLEANUP
])

OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn-controller - vif-plug transaction failures])
AT_KEYWORDS([vif-plug])

ovn_start

net_add n1
sim_add hv1
ovs-vsctl add-br br-phys
ovn_attach n1 br-phys 192.168.0.1
ovn-appctl vlog/set dbg

sim_add hv2
ovs-vsctl add-br br-phys
ovn_attach n1 br-phys 192.168.0.2
ovn-appctl vlog/set dbg

check ovn-nbctl ls-add lsw0

check ovn-nbctl lsp-add lsw0 lsp1
check ovn-nbctl lsp-set-addresses lsp1 "f0:00:00:00:00:01 172.16.0.101"

sleep_ovs hv1

# This used to cause Transaction failures in ovsdb until ofport is created.
check ovn-nbctl lsp-set-options lsp1 \
requested-chassis=hv1 \
vif-plug-type=dummy \
vif-plug-mtu-request=420

check ovn-nbctl lsp-add lsw0 lsp2
check ovn-nbctl lsp-set-addresses lsp2 "f0:00:00:00:00:02 172.16.0.102"

sleep_ovs hv2

check ovn-nbctl lsp-set-options lsp2 \
requested-chassis=hv2 \
vif-plug-type=dummy \
vif-plug-mtu-request=420

wake_up_ovs hv1
wait_for_ports_up lsp1

# Exit ovn-controller while last transaction to ovsdb is not completed
as hv2
OVS_APP_EXIT_AND_WAIT([ovn-controller])
wake_up_ovs hv2
OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])

OVN_CLEANUP([hv1])
AT_CLEANUP
])

0 comments on commit fcc2bf1

Please sign in to comment.