Message ID | 20230419124118.3576664-2-xsimonar@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,v3,1/2] ovn-controller: fixed ovn-installed not always properly added or removed. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Wed, Apr 19, 2023 at 2:41 PM Xavier Simonart <xsimonar@redhat.com> wrote: > When interface was unbound, the port was not always set down and the > port_binding->chassis not always removed. > > Fixes: a7c7d4519e50 ("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> > Hi Xavier, I have one comment down below other than that it looks good. > > --- > v3: - fixed another pb->chassis not being cleared > - avoid setting port down (and logging) if already in idl > --- > controller/binding.c | 20 ++++++- > controller/binding.h | 4 ++ > controller/if-status.c | 102 +++++++++++++++++++++++++----------- > controller/if-status.h | 1 + > controller/ovn-controller.c | 1 + > tests/system-ovn.at | 12 ++--- > 6 files changed, 99 insertions(+), 41 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 4e79c1c87..2aebae721 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -912,7 +912,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); > @@ -3389,6 +3388,24 @@ binding_lport_delete(struct shash *binding_lports, > binding_lport_destroy(b_lport); > } > > +void > +port_binding_set_down(struct ovsdb_idl_index *sbrec_port_binding_by_name, > + const struct sbrec_chassis *chassis_rec, > + const char *iface_id) > +{ > + const struct sbrec_port_binding *pb = lport_lookup_by_name( > + sbrec_port_binding_by_name, iface_id); > + > + if (!pb || sbrec_port_binding_is_deleted(pb)) { > I think this poses the same problem as the previous patch. This should be probably also solved by uuid lookup in the DB. > + 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", iface_id); > + set_pb_chassis_in_sbrec(pb, chassis_rec, false); > + } > +} > + > static void > binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly) > { > @@ -3406,6 +3423,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); > diff --git a/controller/binding.h b/controller/binding.h > index fdf59b813..8ba764f8c 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -204,6 +204,10 @@ 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(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > + const struct sbrec_chassis *chassis_rec, > + const char *iface_id); > + > /* 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 7caa65aca..aa0b95273 100644 > --- a/controller/if-status.c > +++ b/controller/if-status.c > @@ -16,6 +16,7 @@ > #include <config.h> > > #include "binding.h" > +#include "lport.h" > #include "if-status.h" > #include "ofctrl-seqno.h" > #include "simap.h" > @@ -75,6 +76,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, > }; > > @@ -85,18 +89,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() > | | | > @@ -136,25 +142,34 @@ 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) > */ > > @@ -278,6 +293,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: > @@ -306,9 +322,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: > @@ -318,6 +334,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: > @@ -338,9 +355,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: > @@ -350,6 +367,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: > @@ -403,6 +421,7 @@ void > if_status_mgr_update(struct if_status_mgr *mgr, > struct local_binding_data *binding_data, > const struct sbrec_chassis *chassis_rec, > + struct ovsdb_idl_index *sbrec_port_binding_by_name, > const struct ovsrec_interface_table *iface_table, > bool ovs_readonly, > bool sb_readonly) > @@ -459,6 +478,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); > @@ -506,6 +529,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(sbrec_port_binding_by_name, chassis_rec, > + iface->id); > + 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 > diff --git a/controller/if-status.h b/controller/if-status.h > index 6341b5dc6..922345e8e 100644 > --- a/controller/if-status.h > +++ b/controller/if-status.h > @@ -36,6 +36,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, > + struct ovsdb_idl_index > *sbrec_port_binding_by_name, > const struct ovsrec_interface_table > *iface_table, > bool ovs_readonly, > bool sb_readonly); > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 8ddc21f5e..909ef3823 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -5243,6 +5243,7 @@ main(int argc, char *argv[]) > stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME, > time_msec()); > if_status_mgr_update(if_mgr, binding_data, chassis, > + sbrec_port_binding_by_name, > ovsrec_interface_table_get( > ovs_idl_loop.idl), > !ovs_idl_txn, > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index bf15d2aac..f697ebe87 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -10970,10 +10970,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 > > @@ -11029,10 +11027,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 > > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales
diff --git a/controller/binding.c b/controller/binding.c index 4e79c1c87..2aebae721 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -912,7 +912,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); @@ -3389,6 +3388,24 @@ binding_lport_delete(struct shash *binding_lports, binding_lport_destroy(b_lport); } +void +port_binding_set_down(struct ovsdb_idl_index *sbrec_port_binding_by_name, + const struct sbrec_chassis *chassis_rec, + const char *iface_id) +{ + const struct sbrec_port_binding *pb = lport_lookup_by_name( + sbrec_port_binding_by_name, iface_id); + + if (!pb || sbrec_port_binding_is_deleted(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", iface_id); + set_pb_chassis_in_sbrec(pb, chassis_rec, false); + } +} + static void binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly) { @@ -3406,6 +3423,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); diff --git a/controller/binding.h b/controller/binding.h index fdf59b813..8ba764f8c 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -204,6 +204,10 @@ 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(struct ovsdb_idl_index *sbrec_port_binding_by_name, + const struct sbrec_chassis *chassis_rec, + const char *iface_id); + /* 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 7caa65aca..aa0b95273 100644 --- a/controller/if-status.c +++ b/controller/if-status.c @@ -16,6 +16,7 @@ #include <config.h> #include "binding.h" +#include "lport.h" #include "if-status.h" #include "ofctrl-seqno.h" #include "simap.h" @@ -75,6 +76,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, }; @@ -85,18 +89,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() | | | @@ -136,25 +142,34 @@ 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) */ @@ -278,6 +293,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: @@ -306,9 +322,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: @@ -318,6 +334,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: @@ -338,9 +355,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: @@ -350,6 +367,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: @@ -403,6 +421,7 @@ void if_status_mgr_update(struct if_status_mgr *mgr, struct local_binding_data *binding_data, const struct sbrec_chassis *chassis_rec, + struct ovsdb_idl_index *sbrec_port_binding_by_name, const struct ovsrec_interface_table *iface_table, bool ovs_readonly, bool sb_readonly) @@ -459,6 +478,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); @@ -506,6 +529,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(sbrec_port_binding_by_name, chassis_rec, + iface->id); + 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 diff --git a/controller/if-status.h b/controller/if-status.h index 6341b5dc6..922345e8e 100644 --- a/controller/if-status.h +++ b/controller/if-status.h @@ -36,6 +36,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, + struct ovsdb_idl_index *sbrec_port_binding_by_name, const struct ovsrec_interface_table *iface_table, bool ovs_readonly, bool sb_readonly); diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 8ddc21f5e..909ef3823 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5243,6 +5243,7 @@ main(int argc, char *argv[]) stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME, time_msec()); if_status_mgr_update(if_mgr, binding_data, chassis, + sbrec_port_binding_by_name, ovsrec_interface_table_get( ovs_idl_loop.idl), !ovs_idl_txn, diff --git a/tests/system-ovn.at b/tests/system-ovn.at index bf15d2aac..f697ebe87 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -10970,10 +10970,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 @@ -11029,10 +11027,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
When interface was unbound, the port was not always set down and the port_binding->chassis not always removed. Fixes: a7c7d4519e50 ("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> --- v3: - fixed another pb->chassis not being cleared - avoid setting port down (and logging) if already in idl --- controller/binding.c | 20 ++++++- controller/binding.h | 4 ++ controller/if-status.c | 102 +++++++++++++++++++++++++----------- controller/if-status.h | 1 + controller/ovn-controller.c | 1 + tests/system-ovn.at | 12 ++--- 6 files changed, 99 insertions(+), 41 deletions(-)