Skip to content

Commit

Permalink
ovn-controller: fixed port not always set down when unbinding interface
Browse files Browse the repository at this point in the history 
When interface was unbound, the port was not always set down and the
port_binding->chassis not always removed.

Fixes: a7c7d45 ("controller: avoid recomputes triggered by SBDB Port_Binding updates.")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2150905

Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
Reviewed-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
  • Loading branch information
@simonartxavier @dceara
simonartxavier authored and dceara committed May 8, 2023
1 parent ec1db7a commit 7b8bc00
Show file tree
Hide file tree
Showing 6 changed files with 102 additions and 41 deletions.
20 changes: 19 additions & 1 deletion controller/binding.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,6 @@ local_binding_set_down(struct shash *local_bindings, const char *pb_name,

if (!sb_readonly && b_lport && b_lport->pb->n_up && b_lport->pb->up[0] &&
(!b_lport->pb->chassis || b_lport->pb->chassis == chassis_rec)) {
VLOG_INFO("Setting lport %s down in Southbound", pb_name);
binding_lport_set_down(b_lport, sb_readonly);
LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
binding_lport_set_down(b_lport, sb_readonly);
Expand Down Expand Up @@ -3376,6 +3375,24 @@ binding_lport_delete(struct shash *binding_lports,
binding_lport_destroy(b_lport);
}

void
port_binding_set_down(const struct sbrec_chassis *chassis_rec,
const struct sbrec_port_binding_table *pb_table,
const char *iface_id,
const struct uuid *pb_uuid)
{
const struct sbrec_port_binding *pb =
sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid);
if (!pb) {
VLOG_DBG("port_binding already deleted for %s", iface_id);
} else if (pb->n_up && pb->up[0]) {
bool up = false;
sbrec_port_binding_set_up(pb, &up, 1);
VLOG_INFO("Setting lport %s down in Southbound", pb->logical_port);
set_pb_chassis_in_sbrec(pb, chassis_rec, false);
}
}

static void
binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly)
{
Expand All @@ -3393,6 +3410,7 @@ binding_lport_set_down(struct binding_lport *b_lport, bool sb_readonly)
if (sb_readonly || !b_lport || !b_lport->pb->n_up || !b_lport->pb->up[0]) {
return;
}
VLOG_INFO("Setting lport %s down in Southbound", b_lport->name);

bool up = false;
sbrec_port_binding_set_up(b_lport->pb, &up, 1);
Expand Down
5 changes: 5 additions & 0 deletions controller/binding.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
void remove_ovn_installed_for_uuid(const struct ovsrec_interface_table *,
const struct uuid *);

void port_binding_set_down(const struct sbrec_chassis *chassis_rec,
const struct sbrec_port_binding_table *pb_table,
const char *iface_id,
const struct uuid *pb_uuid);

/* Corresponds to each Port_Binding.type. */
enum en_lport_type {
LP_UNKNOWN,
Expand Down
103 changes: 71 additions & 32 deletions controller/if-status.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ enum if_state {
OIF_INSTALLED, /* Interface flows programmed in OVS and binding
* marked "up" in the binding module.
*/
OIF_UPDATE_PORT, /* Logical ports need to be set down, and pb->chassis
* removed.
*/
OIF_MAX,
};

Expand All @@ -85,18 +88,20 @@ static const char *if_state_names[] = {
[OIF_MARK_UP] = "MARK_UP",
[OIF_MARK_DOWN] = "MARK_DOWN",
[OIF_INSTALLED] = "INSTALLED",
[OIF_UPDATE_PORT] = "UPDATE_PORT",
};

/*
* +----------------------+
* +---> | |
* | +-> | NULL | <--------------------------------------+++-+
* | | +----------------------+ |
* | | ^ release_iface | claim_iface() |
* | | | V - sbrec_update_chassis(if sb is rw) |
* | | +----------------------+ |
* | | | | <----------------------------------------+ |
* | | | CLAIMED | <--------------------------------------+ | |
* | +-> | NULL |
* | | +----------------------+
* | | ^ release_iface | claim_iface()
* | | | V - sbrec_update_chassis(if sb is rw)
* | | +----------------------+
* | | | | <------------------------------------------+
* | | | CLAIMED | <----------------------------------------+ |
* | | | | <--------------------------------------+ | |
* | | +----------------------+ | | |
* | | | V ^ | | |
* | | | | | handle_claims() | | |
Expand Down Expand Up @@ -136,31 +141,41 @@ static const char *if_state_names[] = {
* | V 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()
* +---| 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 | |
* |mgr_update( | | |
* | rcvd port down) | | |
* | V | |
* | +----------------------+ | |
* | | | ------------> claim_iface -----------------+ |
* +---+ MARK_DOWN | mgr_run() |
* | | | - set port down in sb |
* | | | mgr_update(sb is rw) |
* | +----------------------+ - sbrec_update_chassis(NULL) |
* | | |
* | | mgr_update(local binding not found) |
* | | |
* | V |
* | +----------------------+ |
* | | | ------------> claim_iface -------------------+
* +---+ UPDATE_PORT | mgr_run()
* +----------------------+ - sbrec_update_chassis(NULL)
*/


struct ovs_iface {
char *id; /* Extracted from OVS external_ids.iface_id. */
struct uuid pb_uuid; /* Port_binding uuid */
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
Expand Down Expand Up @@ -266,6 +281,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
iface = ovs_iface_create(mgr, iface_id, OIF_CLAIMED);
}

memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid));
if (!sb_readonly) {
set_pb_chassis_in_sbrec(pb, chassis_rec, true);
}
Expand All @@ -279,6 +295,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
break;
case OIF_INSTALLED:
case OIF_MARK_DOWN:
case OIF_UPDATE_PORT:
ovs_iface_set_state(mgr, iface, OIF_CLAIMED);
break;
case OIF_MAX:
Expand Down Expand Up @@ -307,9 +324,9 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id)
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;
/* Not yet fully installed interfaces:
* pb->chassis still need to be deleted.
*/
case OIF_REM_OLD_OVN_INST:
case OIF_MARK_UP:
case OIF_INSTALLED:
Expand All @@ -319,6 +336,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id)
ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN);
break;
case OIF_MARK_DOWN:
case OIF_UPDATE_PORT:
/* Nothing to do here. */
break;
case OIF_MAX:
Expand All @@ -339,9 +357,9 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id)
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;
/* Not yet fully installed interfaces:
* pb->chassis still need to be deleted.
*/
case OIF_REM_OLD_OVN_INST:
case OIF_MARK_UP:
case OIF_INSTALLED:
Expand All @@ -351,6 +369,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id)
ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN);
break;
case OIF_MARK_DOWN:
case OIF_UPDATE_PORT:
/* Nothing to do here. */
break;
case OIF_MAX:
Expand Down Expand Up @@ -405,6 +424,7 @@ if_status_mgr_update(struct if_status_mgr *mgr,
struct local_binding_data *binding_data,
const struct sbrec_chassis *chassis_rec,
const struct ovsrec_interface_table *iface_table,
const struct sbrec_port_binding_table *pb_table,
bool ovs_readonly,
bool sb_readonly)
{
Expand Down Expand Up @@ -460,6 +480,10 @@ 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_find(bindings, iface->id)) {
ovs_iface_set_state(mgr, iface, OIF_UPDATE_PORT);
continue;
}
if (!sb_readonly) {
local_binding_set_pb(bindings, iface->id, chassis_rec,
NULL, false);
Expand Down Expand Up @@ -507,6 +531,21 @@ if_status_mgr_update(struct if_status_mgr *mgr,
}
}

if (!sb_readonly) {
HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_UPDATE_PORT]) {
struct ovs_iface *iface = node->data;
port_binding_set_down(chassis_rec, pb_table, iface->id,
&iface->pb_uuid);
ovs_iface_destroy(mgr, node->data);
}
} else {
HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_UPDATE_PORT]) {
struct ovs_iface *iface = node->data;
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
VLOG_INFO_RL(&rl, "Not setting lport %s down as sb is readonly",
iface->id);
}
}
/* 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
Expand Down
1 change: 1 addition & 0 deletions controller/if-status.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ 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 *,
const struct sbrec_chassis *chassis,
const struct ovsrec_interface_table *iface_table,
const struct sbrec_port_binding_table *pb_table,
bool ovs_readonly,
bool sb_readonly);
void if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data *,
Expand Down
2 changes: 2 additions & 0 deletions controller/ovn-controller.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5227,6 +5227,8 @@ main(int argc, char *argv[])
if_status_mgr_update(if_mgr, binding_data, chassis,
ovsrec_interface_table_get(
ovs_idl_loop.idl),
sbrec_port_binding_table_get(
ovnsb_idl_loop.idl),
!ovs_idl_txn,
!ovnsb_idl_txn);
stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
Expand Down
12 changes: 4 additions & 8 deletions tests/system-ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10878,10 +10878,8 @@ sleep_controller
wake_up_sb
wake_up_controller
check_ovn_uninstalled
# Port_down not always set on iface-id removal
# check_ports_down
# Port_Binding(chassis) not always removed on iface-id removal
# check_ports_unbound
check_ports_down
check_ports_unbound
add_iface_ids
check ovn-nbctl --wait=hv sync

Expand Down Expand Up @@ -10937,10 +10935,8 @@ sleep_controller
wake_up_sb
wake_up_controller
check_ovn_uninstalled
# Port_down not always set on Interface removal
# check_ports_down
# Port_Binding(chassis) not always removed on Interface removal
# check_ports_unbound
check_ports_down
check_ports_unbound
add_ovs_interfaces
check ovn-nbctl --wait=hv sync

Expand Down

0 comments on commit 7b8bc00

Please sign in to comment.