| Message ID | 20230104083209.3880669-2-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2,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 1/4/23 09:32, Xavier Simonart 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, > controller/binding.c | 2 +- > controller/if-status.c | 44 +++++++++++++++++++++++++++++++++++++ > controller/if-status.h | 4 ++++ > controller/ovn-controller.c | 12 ++++++++++ > tests/ovn.at | 12 ++++------ > 5 files changed, 65 insertions(+), 9 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index d85a17730..eb8d580c8 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -894,7 +894,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); > @@ -3384,6 +3383,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/if-status.c b/controller/if-status.c > index c008aa79e..26535c9e8 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,6 +89,7 @@ static const char *if_state_names[] = { > [OIF_MARK_UP] = "MARK_UP", > [OIF_MARK_DOWN] = "MARK_DOWN", > [OIF_INSTALLED] = "INSTALLED", > + [OIF_UPDATE_PORT] = "UPDATE_PORT", > }; > > /* > @@ -264,6 +269,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: > @@ -304,6 +310,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: > @@ -336,6 +343,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: > @@ -344,6 +352,36 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) > } > } > > +void > +if_status_handle_lports(struct if_status_mgr *mgr, > + const struct sbrec_chassis *chassis_rec, > + struct ovsdb_idl_index *sbrec_port_binding_by_name, > + bool sb_readonly) > +{ > + if (sb_readonly) { > + return; > + } > + > + struct hmapx_node *node; > + > + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_UPDATE_PORT]) { > + struct ovs_iface *iface = node->data; > + const struct sbrec_port_binding *pb = lport_lookup_by_name( > + sbrec_port_binding_by_name, iface->id); > + > + if ((pb == NULL) || sbrec_port_binding_is_deleted(pb)) { Nit: s/(pb == NULL)/!pb/ > + VLOG_DBG("Removing lport %s from list of ports to set down", > + iface->id); > + } else { > + bool up = false; > + sbrec_port_binding_set_up(pb, &up, 1); > + VLOG_INFO("Setting lport %s down in Southbound", iface->id); We really avoided changing IDL records in if-status.c until now. It makes it more contained. I think all port binding updates should happen in binding.c Maybe we need to wrap this whole if/else into a new function exposed by binding.c > + set_pb_chassis_in_sbrec(pb, chassis_rec, false); > + } > + ovs_iface_destroy(mgr, node->data); > + } > +} > + > bool > if_status_handle_claims(struct if_status_mgr *mgr, > struct local_binding_data *binding_data, > @@ -424,6 +462,12 @@ 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) && sb_readonly) { > + VLOG_DBG("%s not found in local_bindings and sb read only => " > + "port down delayed", 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); > diff --git a/controller/if-status.h b/controller/if-status.h > index 5bd187a25..593597af7 100644 > --- a/controller/if-status.h > +++ b/controller/if-status.h > @@ -48,5 +48,9 @@ bool if_status_handle_claims(struct if_status_mgr *mgr, > const struct sbrec_chassis *chassis_rec, > struct hmap *tracked_datapath, > bool sb_readonly); > +void if_status_handle_lports(struct if_status_mgr *mgr, > + const struct sbrec_chassis *chassis_rec, > + struct ovsdb_idl_index *sbrec_port_binding_by_name, > + bool sb_readonly); > > # endif /* controller/if-status.h */ > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 814a88117..4094eb56e 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1603,6 +1603,11 @@ runtime_data_sb_ro_handler(struct engine_node *node, void *data) > engine_get_input("SB_chassis", node), > "name"); > > + struct ovsdb_idl_index *sbrec_port_binding_by_name = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_port_binding", node), > + "name"); > + > if (chassis_id) { > chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > } > @@ -1620,6 +1625,13 @@ runtime_data_sb_ro_handler(struct engine_node *node, void *data) > engine_set_node_state(node, EN_UPDATED); > rt_data->tracked = true; > } > + > + if (sbrec_port_binding_by_name) { > + if_status_handle_lports(ctrl_ctx->if_mgr, > + chassis, > + sbrec_port_binding_by_name, > + sb_readonly); > + } It seems a bit strange to me that we only do this during the incremental processing stage. Why can't we do this inside if_status_mgr_update()? > } > return true; > } > diff --git a/tests/ovn.at b/tests/ovn.at > index a0378a728..a849a483d 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -34403,10 +34403,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 > > @@ -34443,10 +34441,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 > Regards, Dumitru
diff --git a/controller/binding.c b/controller/binding.c index d85a17730..eb8d580c8 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -894,7 +894,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); @@ -3384,6 +3383,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/if-status.c b/controller/if-status.c index c008aa79e..26535c9e8 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,6 +89,7 @@ static const char *if_state_names[] = { [OIF_MARK_UP] = "MARK_UP", [OIF_MARK_DOWN] = "MARK_DOWN", [OIF_INSTALLED] = "INSTALLED", + [OIF_UPDATE_PORT] = "UPDATE_PORT", }; /* @@ -264,6 +269,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: @@ -304,6 +310,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: @@ -336,6 +343,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: @@ -344,6 +352,36 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) } } +void +if_status_handle_lports(struct if_status_mgr *mgr, + const struct sbrec_chassis *chassis_rec, + struct ovsdb_idl_index *sbrec_port_binding_by_name, + bool sb_readonly) +{ + if (sb_readonly) { + return; + } + + struct hmapx_node *node; + + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_UPDATE_PORT]) { + struct ovs_iface *iface = node->data; + const struct sbrec_port_binding *pb = lport_lookup_by_name( + sbrec_port_binding_by_name, iface->id); + + if ((pb == NULL) || sbrec_port_binding_is_deleted(pb)) { + VLOG_DBG("Removing lport %s from list of ports to set down", + iface->id); + } else { + 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); + } + ovs_iface_destroy(mgr, node->data); + } +} + bool if_status_handle_claims(struct if_status_mgr *mgr, struct local_binding_data *binding_data, @@ -424,6 +462,12 @@ 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) && sb_readonly) { + VLOG_DBG("%s not found in local_bindings and sb read only => " + "port down delayed", 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); diff --git a/controller/if-status.h b/controller/if-status.h index 5bd187a25..593597af7 100644 --- a/controller/if-status.h +++ b/controller/if-status.h @@ -48,5 +48,9 @@ bool if_status_handle_claims(struct if_status_mgr *mgr, const struct sbrec_chassis *chassis_rec, struct hmap *tracked_datapath, bool sb_readonly); +void if_status_handle_lports(struct if_status_mgr *mgr, + const struct sbrec_chassis *chassis_rec, + struct ovsdb_idl_index *sbrec_port_binding_by_name, + bool sb_readonly); # endif /* controller/if-status.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 814a88117..4094eb56e 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1603,6 +1603,11 @@ runtime_data_sb_ro_handler(struct engine_node *node, void *data) engine_get_input("SB_chassis", node), "name"); + struct ovsdb_idl_index *sbrec_port_binding_by_name = + engine_ovsdb_node_get_index( + engine_get_input("SB_port_binding", node), + "name"); + if (chassis_id) { chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); } @@ -1620,6 +1625,13 @@ runtime_data_sb_ro_handler(struct engine_node *node, void *data) engine_set_node_state(node, EN_UPDATED); rt_data->tracked = true; } + + if (sbrec_port_binding_by_name) { + if_status_handle_lports(ctrl_ctx->if_mgr, + chassis, + sbrec_port_binding_by_name, + sb_readonly); + } } return true; } diff --git a/tests/ovn.at b/tests/ovn.at index a0378a728..a849a483d 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -34403,10 +34403,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 @@ -34443,10 +34441,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> --- controller/binding.c | 2 +- controller/if-status.c | 44 +++++++++++++++++++++++++++++++++++++ controller/if-status.h | 4 ++++ controller/ovn-controller.c | 12 ++++++++++ tests/ovn.at | 12 ++++------ 5 files changed, 65 insertions(+), 9 deletions(-)