Hi Mark
Thanks for the review and the comments.
On Mon, Sep 25, 2023 at 9:48 PM Mark Michelson <mmich...@redhat.com> wrote:
>
> Hi Xavier,
>
> Functionality-wise, this looks good. I have a couple of small comments
> though.
>
> On 9/20/23 11:45, Xavier Simonart wrote:
> > Before this patch, if-status module handled pb->chassis and pb->up
> > for vif ports, but not for non-VIF ports.
> > This caused a few potential issues:
> > - It was difficult to check that a no-vif port has been previously claimed
> > as there was no state in if-status. Hence it was difficult to always
> > release
> > such a port when pb->chassis was set to a different chassis.
> > This issue will be fixed in a future patch.
> > - Releasing such ports caused ovn-controller to log warnings such as
> > "Trying to delete unknown ports".
> > - If sb is readonly at the time of the claim, it forced the module to
> > recompute.
> >
> > This patch fixed issues two and three by moving the work of setting
> > pb->chassis
> > and pb->up to the if-status module.
> >
> > Signed-off-by: Xavier Simonart <xsimo...@redhat.com>
> >
> > ---
> > v2: Avoid clearing iface if already deleted
> > ---
> > controller/binding.c | 122 ++++++++++++++++++++++++---------
> > controller/binding.h | 18 +++++
> > controller/if-status.c | 130 ++++++++++++++++++++++++++++--------
> > controller/if-status.h | 9 ++-
> > controller/ovn-controller.c | 6 +-
> > tests/ovn.at | 94 ++++++++++++++++++++++++++
> > 6 files changed, 318 insertions(+), 61 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index dbd2d52b7..92ca3ebbe 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -1200,7 +1200,7 @@ local_binding_set_pb(struct shash *local_bindings,
> > const char *pb_name,
> > * Updates the 'pb->up' field only if it's explicitly set to 'false'.
> > * This is to ensure compatibility with older versions of ovn-northd.
> > */
> > -static bool
> > +bool
> > claimed_lport_set_up(const struct sbrec_port_binding *pb,
> > const struct sbrec_port_binding *parent_pb,
> > bool sb_readonly)
> > @@ -1213,6 +1213,8 @@ claimed_lport_set_up(const struct sbrec_port_binding
> > *pb,
> > if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
> > if (!sb_readonly) {
> > if (pb->n_up) {
> > + VLOG_INFO("Setting lport %s up in Southbound",
> > + pb->logical_port);
> > sbrec_port_binding_set_up(pb, &up, 1);
> > }
> > } else if (pb->n_up && !pb->up[0]) {
> > @@ -1347,39 +1349,26 @@ claim_lport(const struct sbrec_port_binding *pb,
> > }
> > update_tracked = true;
> >
> > - if (!notify_up) {
> > - if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
> > - return false;
> > - }
> > - if (sb_readonly) {
> > - return false;
> > - }
> > - set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> > - } else {
> > - if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> > iface_rec,
> > - sb_readonly, can_bind);
> > - }
> > + if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec,
> > + sb_readonly, can_bind, notify_up,
> > + parent_pb);
> > register_claim_timestamp(pb->logical_port, now);
> > sset_find_and_delete(postponed_ports, pb->logical_port);
> > } else {
> > - if (!notify_up) {
> > - if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
> > - return false;
> > - }
> > - } else {
> > - if ((pb->n_up && !pb->up[0]) ||
> > - !smap_get_bool(&iface_rec->external_ids,
> > - OVN_INSTALLED_EXT_ID, false)) {
> > - if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> > - iface_rec, sb_readonly,
> > - can_bind);
> > - }
> > + if ((pb->n_up && !pb->up[0]) ||
> > + (notify_up && !smap_get_bool(&iface_rec->external_ids,
> > + OVN_INSTALLED_EXT_ID, false))) {
> > + if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> > + iface_rec, sb_readonly,
> > + can_bind, notify_up,
> > + parent_pb);
> > }
> > }
> > } else if (can_bind == CAN_BIND_AS_ADDITIONAL) {
> > if (!is_additional_chassis(pb, chassis_rec)) {
> > if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec,
> > - sb_readonly, can_bind);
> > + sb_readonly, can_bind, notify_up,
> > + parent_pb);
> > update_tracked = true;
> > }
> > }
> > @@ -2446,14 +2435,14 @@ consider_iface_release(const struct
> > ovsrec_interface *iface_rec,
> > remove_related_lport(b_lport->pb, b_ctx_out);
> > }
> >
> > - /* Clear the iface of the local binding. */
> > - lbinding->iface = NULL;
> > -
> > /* Check if the lbinding has children of type PB_CONTAINER.
> > * If so, don't delete the local_binding. */
> > if (!is_lbinding_container_parent(lbinding)) {
> > local_binding_delete(lbinding, local_bindings, binding_lports,
> > b_ctx_out->if_mgr);
> > + } else {
> > + /* Clear the iface of the local binding. */
> > + lbinding->iface = NULL;
> > }
> > }
> >
> > @@ -3247,7 +3236,7 @@ local_binding_delete(struct local_binding *lbinding,
> > struct if_status_mgr *if_mgr)
> > {
> > shash_find_and_delete(local_bindings, lbinding->name);
> > - if_status_mgr_delete_iface(if_mgr, lbinding->name);
> > + if_status_mgr_delete_iface(if_mgr, lbinding->name, lbinding->iface);
> > local_binding_destroy(lbinding, binding_lports);
> > }
> >
> > @@ -3464,6 +3453,79 @@ port_binding_set_down(const struct sbrec_chassis
> > *chassis_rec,
> > }
> > }
> >
> > +void
> > +port_binding_set_up(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);
> > + return;
> > + }
> > + if (pb->n_up && !pb->up[0]) {
> > + bool up = true;
> > + sbrec_port_binding_set_up(pb, &up, 1);
> > + VLOG_INFO("Setting lport %s up in Southbound", pb->logical_port);
> > + set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> > + }
> > +}
> > +
> > +void
> > +port_binding_set_pb(const struct sbrec_chassis *chassis_rec,
> > + const struct sbrec_port_binding_table *pb_table,
> > + const char *iface_id,
> > + const struct uuid *pb_uuid,
> > + enum can_bind bind_type)
> > +{
> > + 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);
> > + return;
> > + }
> > + if (bind_type == CAN_BIND_AS_MAIN) {
> > + set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> > + } else if (bind_type == CAN_BIND_AS_ADDITIONAL) {
> > + set_pb_additional_chassis_in_sbrec(pb, chassis_rec, true);
> > + }
> > +}
> > +
> > +bool
> > +port_binding_pb_chassis_is_set(const struct sbrec_chassis *chassis_rec,
> > + const struct sbrec_port_binding_table *pb_table,
> > + const struct uuid *pb_uuid)
> > +{
> > + const struct sbrec_port_binding *pb =
> > + sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid);
> > +
> > + if (pb && ((pb->chassis == chassis_rec) ||
> > + is_additional_chassis(pb, chassis_rec))) {
> > + return true;
> > + }
> > + return false;
> > +}
> > +
> > +bool
> > +port_binding_is_up(const struct sbrec_chassis *chassis_rec,
> > + const struct sbrec_port_binding_table *pb_table,
> > + const struct uuid *pb_uuid)
> > +{
> > + const struct sbrec_port_binding *pb =
> > + sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid);
> > +
> > + if (!pb || pb->chassis != chassis_rec) {
> > + return false;
> > + }
> > +
> > + if (pb->n_up && !pb->up[0]) {
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > static void
> > binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly)
> > {
> > diff --git a/controller/binding.h b/controller/binding.h
> > index 47df668a2..133badf3f 100644
> > --- a/controller/binding.h
> > +++ b/controller/binding.h
> > @@ -217,6 +217,18 @@ 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);
> > +void port_binding_set_up(const struct sbrec_chassis *chassis_rec,
> > + const struct sbrec_port_binding_table *pb_table,
> > + const char *iface_id,
> > + const struct uuid *pb_uuid);
> > +void port_binding_set_pb(const struct sbrec_chassis *chassis_rec,
> > + const struct sbrec_port_binding_table *pb_table,
> > + const char *iface_id,
> > + const struct uuid *pb_uuid,
> > + enum can_bind bind_type);
> > +bool port_binding_pb_chassis_is_set(const struct sbrec_chassis
> > *chassis_rec,
> > + const struct sbrec_port_binding_table *pb_table,
> > + const struct uuid *pb_uuid);
> >
> > /* This structure represents a logical port (or port binding)
> > * which is associated with 'struct local_binding'.
> > @@ -258,4 +270,10 @@ void update_qos(struct ovsdb_idl_index *
> > sbrec_port_binding_by_name,
> > const struct ovsrec_open_vswitch_table *ovs_table,
> > const struct ovsrec_bridge_table *bridge_table);
> >
> > +bool claimed_lport_set_up(const struct sbrec_port_binding *pb,
> > + const struct sbrec_port_binding *parent_pb,
> > + bool sb_readonly);
> > +bool port_binding_is_up(const struct sbrec_chassis *chassis_rec,
> > + const struct sbrec_port_binding_table *pb_table,
> > + const struct uuid *pb_uuid);
> > #endif /* controller/binding.h */
> > diff --git a/controller/if-status.c b/controller/if-status.c
> > index 6c5afc866..a907dbbaa 100644
> > --- a/controller/if-status.c
> > +++ b/controller/if-status.c
> > @@ -177,7 +177,9 @@ static const char *if_state_names[] = {
> >
> > struct ovs_iface {
> > char *id; /* Extracted from OVS external_ids.iface_id.
> > */
> > + char *name; /* OVS iface name. */
> > struct uuid pb_uuid; /* Port_binding uuid */
> > + struct uuid parent_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
> > @@ -185,6 +187,7 @@ struct ovs_iface {
> > */
> > uint16_t mtu; /* Extracted from OVS interface.mtu field. */
> > enum can_bind bind_type;/* CAN_BIND_AS_MAIN or CAN_BIND_AS_ADDITIONAL
> > */
> > + bool notify_up;
>
> The following comment is in controller/binding.c::claimed_lport_set_up():
>
> /* When notify_up is false in claim_port(), no state is created
> * by if_status_mgr. In such cases, return false (i.e. trigger
> recompute)
> * if we can't update sb (because it is readonly).
> */
>
> It seems that now we create state in if_status_mgr for all types of
> ports, so this comment should be removed.
>
Yes, thanks. I'll send a v2.
>
> > };
> >
> > static uint64_t ifaces_usage;
> > @@ -224,6 +227,7 @@ static void if_status_mgr_update_bindings(
> > struct if_status_mgr *mgr, struct local_binding_data *binding_data,
> > const struct sbrec_chassis *,
> > const struct ovsrec_interface_table *iface_table,
> > + const struct sbrec_port_binding_table *pb_table,
> > bool sb_readonly, bool ovs_readonly);
> >
> > static void ovn_uninstall_hash_account_mem(const char *name, bool erase);
> > @@ -273,12 +277,22 @@ if_status_mgr_destroy(struct if_status_mgr *mgr)
> > free(mgr);
> > }
> >
> > +/* notify_up controls whether we wait for flows to be updated
> > + * before setting the interface up.
> > + * Non-VIF ports are reported up as soon as they are claimed
> > + * to maintain compatibility with older versions.
> > + * See aae25e6 binding: Correctly set Port_Binding.up for container/virtual
> > + * ports.
>
> Reading this comment didn't help me much, because it is not clear what
> the relationship is between "notify_up" and whether a port is vif or
> non-vif. Based on looking at the code, I *think* that notify_up is true
> for vif ports and false for non-vif ports, but I'm not 100% sure.
>
> Would it make more sense to name the struct field something like
> "is_vif" or something to make it more clear what the field means?
Yes, agreed. Will clarify in v2.
>
> > + */
> > +
> > void
> > if_status_mgr_claim_iface(struct if_status_mgr *mgr,
> > const struct sbrec_port_binding *pb,
> > const struct sbrec_chassis *chassis_rec,
> > const struct ovsrec_interface *iface_rec,
> > - bool sb_readonly, enum can_bind bind_type)
> > + bool sb_readonly, enum can_bind bind_type,
> > + bool notify_up,
> > + const struct sbrec_port_binding *parent_pb)
> > {
> > const char *iface_id = pb->logical_port;
> > struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
> > @@ -287,8 +301,13 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
> > iface = ovs_iface_create(mgr, iface_id, iface_rec, OIF_CLAIMED);
> > }
> > iface->bind_type = bind_type;
> > + iface->notify_up = notify_up;
> >
> > memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid));
> > + if (parent_pb) {
> > + memcpy(&iface->parent_pb_uuid, &parent_pb->header_.uuid,
> > + sizeof(iface->pb_uuid));
> > + }
> > if (!sb_readonly) {
> > if (bind_type == CAN_BIND_AS_MAIN) {
> > set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> > @@ -357,7 +376,8 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr,
> > const char *iface_id)
> > }
> >
> > void
> > -if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id)
> > +if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id,
> > + const struct ovsrec_interface *iface_rec)
> > {
> > struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
> >
> > @@ -365,6 +385,12 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr,
> > const char *iface_id)
> > return;
> > }
> >
> > + if (iface_rec && strcmp(iface->name, iface_rec->name)) {
> > + VLOG_DBG("Interface %s not deleted as port %s bound to %s",
> > + iface_rec->name, iface_id, iface->name);
> > + return;
> > + }
> > +
> > switch (iface->state) {
> > case OIF_CLAIMED:
> > case OIF_INSTALL_FLOWS:
> > @@ -394,6 +420,7 @@ if_status_handle_claims(struct if_status_mgr *mgr,
> > struct local_binding_data *binding_data,
> > const struct sbrec_chassis *chassis_rec,
> > struct hmap *tracked_datapath,
> > + const struct sbrec_port_binding_table *pb_table,
> > bool sb_readonly)
> > {
> > if (!binding_data || sb_readonly) {
> > @@ -406,9 +433,15 @@ if_status_handle_claims(struct if_status_mgr *mgr,
> > bool rc = false;
> > HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> > 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, iface->bind_type);
> > + VLOG_INFO("if_status_handle_claims for %s, notify_up = %d",
> > iface->id,
> > + iface->notify_up);
>
> This feels more like a VLOG_DBG level message than VLOG_INFO, especially
> with the "notify_up = %d" part added on.
>
> > + if (iface->notify_up) {
> > + local_binding_set_pb(bindings, iface->id, chassis_rec,
> > + tracked_datapath, true, iface->bind_type);
> > + } else {
> > + port_binding_set_pb(chassis_rec, pb_table, iface->id,
> > + &iface->pb_uuid, iface->bind_type);
> > + }
> > rc = true;
> > }
> > return rc;
> > @@ -470,18 +503,32 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> > */
> > HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_UP]) {
> > struct ovs_iface *iface = node->data;
> > -
> > - if (!local_bindings_pb_chassis_is_set(bindings, iface->id,
> > - chassis_rec)) {
> > - if (!sb_readonly) {
> > - local_binding_set_pb(bindings, iface->id, chassis_rec,
> > - NULL, true, iface->bind_type);
> > - } else {
> > - continue;
> > + if (iface->notify_up) {
> > + if (!local_bindings_pb_chassis_is_set(bindings, iface->id,
> > + chassis_rec)) {
> > + if (!sb_readonly) {
> > + local_binding_set_pb(bindings, iface->id, chassis_rec,
> > + NULL, true, iface->bind_type);
> > + } else {
> > + continue;
> > + }
> > + }
> > + if (local_binding_is_up(bindings, iface->id, chassis_rec)) {
> > + ovs_iface_set_state(mgr, iface, OIF_INSTALLED);
> > + }
> > + } else {
> > + if (!port_binding_pb_chassis_is_set(chassis_rec, pb_table,
> > + &iface->pb_uuid)) {
> > + if (!sb_readonly) {
> > + port_binding_set_pb(chassis_rec, pb_table, iface->id,
> > + &iface->pb_uuid, iface->bind_type);
> > + } else {
> > + continue;
> > + }
> > + }
> > + if (port_binding_is_up(chassis_rec, pb_table,
> > &iface->pb_uuid)) {
> > + ovs_iface_set_state(mgr, iface, OIF_INSTALLED);
> > }
> > - }
> > - if (local_binding_is_up(bindings, iface->id, chassis_rec)) {
> > - ovs_iface_set_state(mgr, iface, OIF_INSTALLED);
> > }
> > }
> >
> > @@ -528,9 +575,13 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> > /* No need to to update pb->chassis as already done
> > * in if_status_handle_claims or if_status_mgr_claim_iface
> > */
> > - ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
> > - iface->install_seqno = mgr->iface_seqno + 1;
> > - new_ifaces = true;
> > + if (iface->notify_up) {
> > + ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
> > + iface->install_seqno = mgr->iface_seqno + 1;
> > + new_ifaces = true;
> > + } else {
> > + ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> > + }
> > }
> > } else {
> > HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
> > @@ -587,6 +638,7 @@ if_status_mgr_run(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 sb_readonly, bool ovs_readonly)
> > {
> > struct ofctrl_acked_seqnos *acked_seqnos =
> > @@ -622,15 +674,18 @@ if_status_mgr_run(struct if_status_mgr *mgr,
> >
> > /* Update binding states. */
> > if_status_mgr_update_bindings(mgr, binding_data, chassis_rec,
> > - iface_table,
> > + iface_table, pb_table,
> > sb_readonly, ovs_readonly);
> > }
> >
> > static void
> > -ovs_iface_account_mem(const char *iface_id, bool erase)
> > +ovs_iface_account_mem(const char *iface_id, char *iface_name, bool erase)
> > {
> > uint32_t size = (strlen(iface_id) + sizeof(struct ovs_iface) +
> > sizeof(struct shash_node));
> > + if (iface_name) {
> > + size += strlen(iface_name);
> > + }
> > if (erase) {
> > ifaces_usage -= size;
> > } else {
> > @@ -682,12 +737,18 @@ ovs_iface_create(struct if_status_mgr *mgr, const
> > char *iface_id,
> > {
> > struct ovs_iface *iface = xzalloc(sizeof *iface);
> >
> > - VLOG_DBG("Interface %s create.", iface_id);
> > + VLOG_DBG("Interface %s create for iface %s.", iface_id,
> > + iface_rec ? iface_rec->name : "");
> > iface->id = xstrdup(iface_id);
> > shash_add_nocopy(&mgr->ifaces, iface->id, iface);
> > ovs_iface_set_state(mgr, iface, state);
> > - ovs_iface_account_mem(iface_id, false);
> > - if_status_mgr_iface_update(mgr, iface_rec);
> > + if (iface_rec) {
> > + ovs_iface_account_mem(iface_id, iface_rec->name, false);
> > + if_status_mgr_iface_update(mgr, iface_rec);
> > + iface->name = xstrdup(iface_rec->name);
> > + } else {
> > + ovs_iface_account_mem(iface_id, NULL, false);
> > + }
> > return iface;
> > }
> >
> > @@ -711,7 +772,8 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct
> > ovs_iface *iface)
> > if (node) {
> > shash_steal(&mgr->ifaces, node);
> > }
> > - ovs_iface_account_mem(iface->id, true);
> > + ovs_iface_account_mem(iface->id, iface->name, true);
> > + free(iface->name);
> > free(iface->id);
> > free(iface);
> > }
> > @@ -752,6 +814,7 @@ if_status_mgr_update_bindings(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 sb_readonly, bool ovs_readonly)
> > {
> > if (!binding_data) {
> > @@ -788,9 +851,20 @@ if_status_mgr_update_bindings(struct if_status_mgr
> > *mgr,
> > char *ts_now_str = xasprintf("%lld", time_wall_msec());
> > HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_MARK_UP]) {
> > struct ovs_iface *iface = node->data;
> > -
> > - local_binding_set_up(bindings, iface->id, chassis_rec, ts_now_str,
> > - sb_readonly, ovs_readonly);
> > + if (iface->notify_up) {
> > + local_binding_set_up(bindings, iface->id, chassis_rec,
> > ts_now_str,
> > + sb_readonly, ovs_readonly);
> > + } else if (!sb_readonly) {
> > + const struct sbrec_port_binding *pb =
> > + sbrec_port_binding_table_get_for_uuid(pb_table,
> > + &iface->pb_uuid);
> > + if (pb) {
> > + const struct sbrec_port_binding *parent_pb =
> > + sbrec_port_binding_table_get_for_uuid(pb_table,
> > + &iface->parent_pb_uuid);
> > + claimed_lport_set_up(pb, parent_pb, sb_readonly);
> > + }
> > + }
> > }
> > free(ts_now_str);
> >
> > diff --git a/controller/if-status.h b/controller/if-status.h
> > index 9714f6d8d..4ae5ad481 100644
> > --- a/controller/if-status.h
> > +++ b/controller/if-status.h
> > @@ -32,9 +32,12 @@ void if_status_mgr_claim_iface(struct if_status_mgr *,
> > const struct sbrec_port_binding *pb,
> > const struct sbrec_chassis *chassis_rec,
> > const struct ovsrec_interface *iface_rec,
> > - bool sb_readonly, enum can_bind bind_type);
> > + bool sb_readonly, enum can_bind bind_type,
> > + bool notify_up,
> > + const struct sbrec_port_binding *parent_pb);
> > void if_status_mgr_release_iface(struct if_status_mgr *, const char
> > *iface_id);
> > -void if_status_mgr_delete_iface(struct if_status_mgr *, const char
> > *iface_id);
> > +void if_status_mgr_delete_iface(struct if_status_mgr *, const char
> > *iface_id,
> > + const struct ovsrec_interface *iface_rec);
> >
> > void if_status_mgr_update(struct if_status_mgr *, struct
> > local_binding_data *,
> > const struct sbrec_chassis *chassis,
> > @@ -45,6 +48,7 @@ void if_status_mgr_update(struct if_status_mgr *, struct
> > local_binding_data *,
> > void if_status_mgr_run(struct if_status_mgr *mgr, struct
> > local_binding_data *,
> > const struct sbrec_chassis *,
> > const struct ovsrec_interface_table *iface_table,
> > + const struct sbrec_port_binding_table *pb_table,
> > bool sb_readonly, bool ovs_readonly);
> > void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr,
> > struct simap *usage);
> > @@ -54,6 +58,7 @@ bool if_status_handle_claims(struct if_status_mgr *mgr,
> > struct local_binding_data *binding_data,
> > const struct sbrec_chassis *chassis_rec,
> > struct hmap *tracked_datapath,
> > + const struct sbrec_port_binding_table
> > *pb_table,
> > bool sb_readonly);
> > void if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr,
> > const char *name,
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 859d9cab9..b09ac047e 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1815,6 +1815,8 @@ runtime_data_sb_ro_handler(struct engine_node *node,
> > void *data)
> > engine_ovsdb_node_get_index(
> > engine_get_input("SB_chassis", node),
> > "name");
> > + const struct sbrec_port_binding_table *pb_table =
> > + EN_OVSDB_GET(engine_get_input("SB_port_binding", node));
> >
> > if (chassis_id) {
> > chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
> > chassis_id);
> > @@ -1829,7 +1831,7 @@ runtime_data_sb_ro_handler(struct engine_node *node,
> > void *data)
> > &rt_data->lbinding_data,
> > chassis,
> > &rt_data->tracked_dp_bindings,
> > - sb_readonly)) {
> > + pb_table, sb_readonly)) {
> > engine_set_node_state(node, EN_UPDATED);
> > rt_data->tracked = true;
> > }
> > @@ -5871,6 +5873,8 @@ main(int argc, char *argv[])
> > if_status_mgr_run(if_mgr, binding_data, chassis,
> > ovsrec_interface_table_get(
> > ovs_idl_loop.idl),
> > + sbrec_port_binding_table_get(
> > + ovnsb_idl_loop.idl),
> > !ovnsb_idl_txn, !ovs_idl_txn);
> > stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
> > time_msec());
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index ba5ce298a..b4f146590 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -37004,3 +37004,97 @@ AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE"
> > hv1/ovs-vswitchd.log], [0], [dnl
> > OVN_CLEANUP([hv1])
> > AT_CLEANUP
> > ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([virtual port claim race condition])
> > +AT_KEYWORDS([virtual ports])
> > +ovn_start
> > +
> > +send_garp() {
> > + local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 spa=$5 tpa=$6
> > + local
> > request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
>
> Can this be changed to use fmt_pkt()? It makes understanding and
> debugging tests much easier. Don't forget to add an AT_SKIP to the test
> if HAVE_SCAPY is false.
I'd prefer to have a separate patch for this: looking at the whole
test, we have 6 implementations of send_garp, all very similar but
slightly different.
A separate patch could clean them all. WDYT?
>
> > + as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request
> > +}
> > +
> > +net_add n1
> > +
> > +sim_add hv1
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.1
> > +ovn-appctl vlog/set dbg
> > +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
> > + options:tx_pcap=hv1/vif1-tx.pcap \
> > + options:rxq_pcap=hv1/vif1-rx.pcap \
> > + ofport-request=1
> > +ovs-vsctl -- add-port br-int hv1-vif3 -- \
> > + set interface hv1-vif3 \
> > + options:tx_pcap=hv1/vif3-tx.pcap \
> > + options:rxq_pcap=hv1/vif3-rx.pcap \
> > + ofport-request=3
> > +
> > +ovs-appctl -t ovn-controller vlog/set dbg
> > +
> > +ovn-nbctl ls-add sw0
> > +
> > +check ovn-nbctl lsp-add sw0 sw0-vir
> > +check ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10"
> > +check ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10"
> > +check ovn-nbctl lsp-set-type sw0-vir virtual
> > +check ovn-nbctl set logical_switch_port sw0-vir
> > options:virtual-ip=10.0.0.10
> > +check ovn-nbctl set logical_switch_port sw0-vir
> > options:virtual-parents=sw0-p1
> > +
> > +check ovn-nbctl lsp-add sw0 sw0-p1
> > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3
> > 1000::3"
> > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3
> > 10.0.0.10 1000::3"
> > +
> > +# Create a logical router and attach both logical switches
> > +check ovn-nbctl lr-add lr0
> > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
> > 1000::a/64
> > +check ovn-nbctl lsp-add sw0 sw0-lr0
> > +check ovn-nbctl lsp-set-type sw0-lr0 router
> > +check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
> > +check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
> > +
> > +wait_for_ports_up
> > +ovn-nbctl --wait=hv sync
> > +hv1_ch_uuid=`ovn-sbctl --bare --columns _uuid find chassis name="hv1"`
> > +
> > +# Try to bind sw0-vir directly to an OVS port. This should be ignored by
> > +# ovn-controller.
> > +as hv1 ovs-vsctl set interface hv1-vif3 external-ids:iface-id=sw0-vir
> > +
> > +# Make sb to sleep, so that claim of sw0-vir (through pinctrl) and
> > hv1-vif3 can be handled within same idl by controller
> > +sleep_sb
> > +
> > +# From sw0-p0 send GARP for 10.0.0.10. hv1 should claim sw0-vir
> > +# and sw0-p1 should be its virtual_parent.
> > +eth_src=505400000003
> > +eth_dst=ffffffffffff
> > +spa=$(ip_to_hex 10 0 0 10)
> > +tpa=$(ip_to_hex 10 0 0 10)
> > +send_garp 1 1 $eth_src $eth_dst $spa $tpa
> > +
> > +OVS_WAIT_UNTIL([test 1 = `cat hv1/ovn-controller.log | grep "pinctrl
> > received packet-in" | \
> > +grep opcode=BIND_VPORT | grep OF_Table_ID=29 | wc -l`])
> > +
> > +sleep_controller hv1
> > +
> > +# Cleanup hv1-vif3. This should not interfere with sw0-vir claim
> > +as hv1 ovs-vsctl del-port hv1-vif3
> > +
> > +wake_up_sb
> > +ovn-nbctl --wait=sb sync
> > +wake_up_controller hv1
> > +check ovn-nbctl --wait=hv sync
> > +
> > +wait_row_count Port_Binding 1 logical_port=sw0-vir chassis=$hv1_ch_uuid
> > +check_row_count Port_Binding 1 logical_port=sw0-vir virtual_parent=sw0-p1
> > +wait_for_ports_up sw0-vir
> > +check ovn-nbctl --wait=hv sync
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
> > +
>
Thanks
Xavier
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
