| Message ID | 20230627093634.1465570-1-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] binding: fixed port claims as additional_chassis | 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 | fail | github build: failed |
On Tue, Jun 27, 2023 at 11:37 AM Xavier Simonart <xsimonar@redhat.com> wrote: > When sb is read-only, the port claim is delayed until sb is rw. > However, before this patch, this resulted in the chassis always > claiming the port as main (while it was maybe an additional chassis). > > Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB > Port_Binding updates.") > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > controller/binding.c | 14 +++++++++++--- > controller/binding.h | 3 ++- > controller/if-status.c | 10 ++++++---- > 3 files changed, 19 insertions(+), 8 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 359ad6d66..9fb90cf9f 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1164,7 +1164,9 @@ local_bindings_pb_chassis_is_set(struct shash > *local_bindings, > local_binding_find(local_bindings, pb_name); > struct binding_lport *b_lport = > local_binding_get_primary_lport(lbinding); > > - if (b_lport && b_lport->pb && b_lport->pb->chassis == chassis_rec) { > + if (b_lport && b_lport->pb && > + ((b_lport->pb->chassis == chassis_rec) || > + is_additional_chassis(b_lport->pb, chassis_rec))) { > return true; > } > return false; > @@ -1173,14 +1175,20 @@ local_bindings_pb_chassis_is_set(struct shash > *local_bindings, > void > local_binding_set_pb(struct shash *local_bindings, const char *pb_name, > const struct sbrec_chassis *chassis_rec, > - struct hmap *tracked_datapaths, bool is_set) > + struct hmap *tracked_datapaths, bool is_set, > + enum can_bind bind_type) > { > struct local_binding *lbinding = > local_binding_find(local_bindings, pb_name); > struct binding_lport *b_lport = > local_binding_get_primary_lport(lbinding); > > if (b_lport) { > - set_pb_chassis_in_sbrec(b_lport->pb, chassis_rec, is_set); > + if (bind_type == CAN_BIND_AS_MAIN) { > + set_pb_chassis_in_sbrec(b_lport->pb, chassis_rec, is_set); > + } else if (bind_type == CAN_BIND_AS_ADDITIONAL) { > + set_pb_additional_chassis_in_sbrec(b_lport->pb, chassis_rec, > + is_set); > + } > if (tracked_datapaths) { > update_lport_tracking(b_lport->pb, tracked_datapaths, true); > } > diff --git a/controller/binding.h b/controller/binding.h > index 319cbd263..abc3d6117 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -23,6 +23,7 @@ > #include "openvswitch/uuid.h" > #include "openvswitch/list.h" > #include "sset.h" > +#include "lport.h" > > struct hmap; > struct ovsdb_idl; > @@ -177,7 +178,7 @@ void local_binding_set_down(struct shash > *local_bindings, const char *pb_name, > void local_binding_set_pb(struct shash *local_bindings, const char > *pb_name, > const struct sbrec_chassis *chassis_rec, > struct hmap *tracked_datapaths, > - bool is_set); > + bool is_set, enum can_bind); > bool local_bindings_pb_chassis_is_set(struct shash *local_bindings, > const char *pb_name, > const struct sbrec_chassis > *chassis_rec); > diff --git a/controller/if-status.c b/controller/if-status.c > index 2b2eb1679..b45208746 100644 > --- a/controller/if-status.c > +++ b/controller/if-status.c > @@ -184,6 +184,7 @@ struct ovs_iface { > * OIF_INSTALL_FLOWS. > */ > uint16_t mtu; /* Extracted from OVS interface.mtu field. */ > + enum can_bind bind_type;/* CAN_BIND_AS_MAIN or CAN_BIND_AS_ADDITIONAL > */ > }; > > static uint64_t ifaces_usage; > @@ -285,6 +286,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, > if (!iface) { > iface = ovs_iface_create(mgr, iface_id, iface_rec, OIF_CLAIMED); > } > + iface->bind_type = bind_type; > > memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid)); > if (!sb_readonly) { > @@ -406,7 +408,7 @@ if_status_handle_claims(struct if_status_mgr *mgr, > struct ovs_iface *iface = node->data; > VLOG_INFO("if_status_handle_claims for %s", iface->id); > local_binding_set_pb(bindings, iface->id, chassis_rec, > - tracked_datapath, true); > + tracked_datapath, true, iface->bind_type); > rc = true; > } > return rc; > @@ -473,7 +475,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, > chassis_rec)) { > if (!sb_readonly) { > local_binding_set_pb(bindings, iface->id, chassis_rec, > - NULL, true); > + NULL, true, iface->bind_type); > } else { > continue; > } > @@ -495,7 +497,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, > } > if (!sb_readonly) { > local_binding_set_pb(bindings, iface->id, chassis_rec, > - NULL, false); > + NULL, false, iface->bind_type); > } > if (local_binding_is_down(bindings, iface->id, chassis_rec)) { > ovs_iface_destroy(mgr, iface); > @@ -512,7 +514,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, > if (!local_bindings_pb_chassis_is_set(bindings, iface->id, > chassis_rec)) { > local_binding_set_pb(bindings, iface->id, chassis_rec, > - NULL, true); > + NULL, true, iface->bind_type); > } > } > } > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 7/7/23 14:11, Ales Musil wrote: > On Tue, Jun 27, 2023 at 11:37 AM Xavier Simonart <xsimonar@redhat.com> > wrote: > >> When sb is read-only, the port claim is delayed until sb is rw. >> However, before this patch, this resulted in the chassis always >> claiming the port as main (while it was maybe an additional chassis). >> >> Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB >> Port_Binding updates.") >> >> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> >> --- >> controller/binding.c | 14 +++++++++++--- >> controller/binding.h | 3 ++- >> controller/if-status.c | 10 ++++++---- >> 3 files changed, 19 insertions(+), 8 deletions(-) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index 359ad6d66..9fb90cf9f 100644 >> --- a/controller/binding.c >> +++ b/controller/binding.c >> @@ -1164,7 +1164,9 @@ local_bindings_pb_chassis_is_set(struct shash >> *local_bindings, >> local_binding_find(local_bindings, pb_name); >> struct binding_lport *b_lport = >> local_binding_get_primary_lport(lbinding); >> >> - if (b_lport && b_lport->pb && b_lport->pb->chassis == chassis_rec) { >> + if (b_lport && b_lport->pb && >> + ((b_lport->pb->chassis == chassis_rec) || >> + is_additional_chassis(b_lport->pb, chassis_rec))) { >> return true; >> } >> return false; >> @@ -1173,14 +1175,20 @@ local_bindings_pb_chassis_is_set(struct shash >> *local_bindings, >> void >> local_binding_set_pb(struct shash *local_bindings, const char *pb_name, >> const struct sbrec_chassis *chassis_rec, >> - struct hmap *tracked_datapaths, bool is_set) >> + struct hmap *tracked_datapaths, bool is_set, >> + enum can_bind bind_type) >> { >> struct local_binding *lbinding = >> local_binding_find(local_bindings, pb_name); >> struct binding_lport *b_lport = >> local_binding_get_primary_lport(lbinding); >> >> if (b_lport) { >> - set_pb_chassis_in_sbrec(b_lport->pb, chassis_rec, is_set); >> + if (bind_type == CAN_BIND_AS_MAIN) { >> + set_pb_chassis_in_sbrec(b_lport->pb, chassis_rec, is_set); >> + } else if (bind_type == CAN_BIND_AS_ADDITIONAL) { >> + set_pb_additional_chassis_in_sbrec(b_lport->pb, chassis_rec, >> + is_set); >> + } >> if (tracked_datapaths) { >> update_lport_tracking(b_lport->pb, tracked_datapaths, true); >> } >> diff --git a/controller/binding.h b/controller/binding.h >> index 319cbd263..abc3d6117 100644 >> --- a/controller/binding.h >> +++ b/controller/binding.h >> @@ -23,6 +23,7 @@ >> #include "openvswitch/uuid.h" >> #include "openvswitch/list.h" >> #include "sset.h" >> +#include "lport.h" >> >> struct hmap; >> struct ovsdb_idl; >> @@ -177,7 +178,7 @@ void local_binding_set_down(struct shash >> *local_bindings, const char *pb_name, >> void local_binding_set_pb(struct shash *local_bindings, const char >> *pb_name, >> const struct sbrec_chassis *chassis_rec, >> struct hmap *tracked_datapaths, >> - bool is_set); >> + bool is_set, enum can_bind); >> bool local_bindings_pb_chassis_is_set(struct shash *local_bindings, >> const char *pb_name, >> const struct sbrec_chassis >> *chassis_rec); >> diff --git a/controller/if-status.c b/controller/if-status.c >> index 2b2eb1679..b45208746 100644 >> --- a/controller/if-status.c >> +++ b/controller/if-status.c >> @@ -184,6 +184,7 @@ struct ovs_iface { >> * OIF_INSTALL_FLOWS. >> */ >> uint16_t mtu; /* Extracted from OVS interface.mtu field. */ >> + enum can_bind bind_type;/* CAN_BIND_AS_MAIN or CAN_BIND_AS_ADDITIONAL >> */ >> }; >> >> static uint64_t ifaces_usage; >> @@ -285,6 +286,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, >> if (!iface) { >> iface = ovs_iface_create(mgr, iface_id, iface_rec, OIF_CLAIMED); >> } >> + iface->bind_type = bind_type; >> >> memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid)); >> if (!sb_readonly) { >> @@ -406,7 +408,7 @@ if_status_handle_claims(struct if_status_mgr *mgr, >> struct ovs_iface *iface = node->data; >> VLOG_INFO("if_status_handle_claims for %s", iface->id); >> local_binding_set_pb(bindings, iface->id, chassis_rec, >> - tracked_datapath, true); >> + tracked_datapath, true, iface->bind_type); >> rc = true; >> } >> return rc; >> @@ -473,7 +475,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, >> chassis_rec)) { >> if (!sb_readonly) { >> local_binding_set_pb(bindings, iface->id, chassis_rec, >> - NULL, true); >> + NULL, true, iface->bind_type); >> } else { >> continue; >> } >> @@ -495,7 +497,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, >> } >> if (!sb_readonly) { >> local_binding_set_pb(bindings, iface->id, chassis_rec, >> - NULL, false); >> + NULL, false, iface->bind_type); >> } >> if (local_binding_is_down(bindings, iface->id, chassis_rec)) { >> ovs_iface_destroy(mgr, iface); >> @@ -512,7 +514,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, >> if (!local_bindings_pb_chassis_is_set(bindings, iface->id, >> chassis_rec)) { >> local_binding_set_pb(bindings, iface->id, chassis_rec, >> - NULL, true); >> + NULL, true, iface->bind_type); >> } >> } >> } >> -- >> 2.31.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> > Thanks Xavier and Ales! I applied this to main and backported it to all stable branches down to 22.09. Regards, Dumitru
diff --git a/controller/binding.c b/controller/binding.c index 359ad6d66..9fb90cf9f 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1164,7 +1164,9 @@ local_bindings_pb_chassis_is_set(struct shash *local_bindings, local_binding_find(local_bindings, pb_name); struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding); - if (b_lport && b_lport->pb && b_lport->pb->chassis == chassis_rec) { + if (b_lport && b_lport->pb && + ((b_lport->pb->chassis == chassis_rec) || + is_additional_chassis(b_lport->pb, chassis_rec))) { return true; } return false; @@ -1173,14 +1175,20 @@ local_bindings_pb_chassis_is_set(struct shash *local_bindings, void local_binding_set_pb(struct shash *local_bindings, const char *pb_name, const struct sbrec_chassis *chassis_rec, - struct hmap *tracked_datapaths, bool is_set) + struct hmap *tracked_datapaths, bool is_set, + enum can_bind bind_type) { struct local_binding *lbinding = local_binding_find(local_bindings, pb_name); struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding); if (b_lport) { - set_pb_chassis_in_sbrec(b_lport->pb, chassis_rec, is_set); + if (bind_type == CAN_BIND_AS_MAIN) { + set_pb_chassis_in_sbrec(b_lport->pb, chassis_rec, is_set); + } else if (bind_type == CAN_BIND_AS_ADDITIONAL) { + set_pb_additional_chassis_in_sbrec(b_lport->pb, chassis_rec, + is_set); + } if (tracked_datapaths) { update_lport_tracking(b_lport->pb, tracked_datapaths, true); } diff --git a/controller/binding.h b/controller/binding.h index 319cbd263..abc3d6117 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -23,6 +23,7 @@ #include "openvswitch/uuid.h" #include "openvswitch/list.h" #include "sset.h" +#include "lport.h" struct hmap; struct ovsdb_idl; @@ -177,7 +178,7 @@ void local_binding_set_down(struct shash *local_bindings, const char *pb_name, void local_binding_set_pb(struct shash *local_bindings, const char *pb_name, const struct sbrec_chassis *chassis_rec, struct hmap *tracked_datapaths, - bool is_set); + bool is_set, enum can_bind); bool local_bindings_pb_chassis_is_set(struct shash *local_bindings, const char *pb_name, const struct sbrec_chassis *chassis_rec); diff --git a/controller/if-status.c b/controller/if-status.c index 2b2eb1679..b45208746 100644 --- a/controller/if-status.c +++ b/controller/if-status.c @@ -184,6 +184,7 @@ struct ovs_iface { * OIF_INSTALL_FLOWS. */ uint16_t mtu; /* Extracted from OVS interface.mtu field. */ + enum can_bind bind_type;/* CAN_BIND_AS_MAIN or CAN_BIND_AS_ADDITIONAL */ }; static uint64_t ifaces_usage; @@ -285,6 +286,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, if (!iface) { iface = ovs_iface_create(mgr, iface_id, iface_rec, OIF_CLAIMED); } + iface->bind_type = bind_type; memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid)); if (!sb_readonly) { @@ -406,7 +408,7 @@ if_status_handle_claims(struct if_status_mgr *mgr, struct ovs_iface *iface = node->data; VLOG_INFO("if_status_handle_claims for %s", iface->id); local_binding_set_pb(bindings, iface->id, chassis_rec, - tracked_datapath, true); + tracked_datapath, true, iface->bind_type); rc = true; } return rc; @@ -473,7 +475,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, chassis_rec)) { if (!sb_readonly) { local_binding_set_pb(bindings, iface->id, chassis_rec, - NULL, true); + NULL, true, iface->bind_type); } else { continue; } @@ -495,7 +497,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, } if (!sb_readonly) { local_binding_set_pb(bindings, iface->id, chassis_rec, - NULL, false); + NULL, false, iface->bind_type); } if (local_binding_is_down(bindings, iface->id, chassis_rec)) { ovs_iface_destroy(mgr, iface); @@ -512,7 +514,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, if (!local_bindings_pb_chassis_is_set(bindings, iface->id, chassis_rec)) { local_binding_set_pb(bindings, iface->id, chassis_rec, - NULL, true); + NULL, true, iface->bind_type); } } }
When sb is read-only, the port claim is delayed until sb is rw. However, before this patch, this resulted in the chassis always claiming the port as main (while it was maybe an additional chassis). Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB Port_Binding updates.") Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- controller/binding.c | 14 +++++++++++--- controller/binding.h | 3 ++- controller/if-status.c | 10 ++++++---- 3 files changed, 19 insertions(+), 8 deletions(-)