[ovs-dev] [PATCH ovn] ovn-controller: Fix incremental processing of Port_Binding deletes.

Numan Siddique numans at ovn.org
Mon Aug 31 11:24:50 UTC 2020


On Mon, Aug 31, 2020 at 3:55 PM Dumitru Ceara <dceara at redhat.com> wrote:

> If a Port_Binding is deleted from the Southbound DB and the
> corresponding OVS interface is also deleted from the OVS DB, and
> if both updates are received and processed by ovn-controller in
> the same iteration, ovn-controller should not be allowed to write
> to the (to be deleted) Port_Binding record stored in memory.
>
> This commit also adds three new unixctl debug commands for
> ovn-controller:
> - debug/pause: pause ovn-controller processing, except unixctl commands.
> - debug/resume: resume ovn-controller processing.
> - debug/status: return the status of the ovn-controller processing.
>
> These new commands are needed by the test for this scenario as without
> them we have no way of ensuring predictable results. Users should not
> use these commands in production. This is also why the commands are not
> documented.
>
> CC: Numan Siddique <numans at ovn.org>
> Fixes: 6b0f01116bab ("ovn-controller: Handle runtime data changes in flow
> output engine")
> Reported-by: Tim Rozet <trozet at redhat.com>
> Reported-at: https://bugzilla.redhat.com/1871961
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> ---
>

Thanks Dumitru for the fix.

The issue is seen because we handle OVS interface changes first and then
the port binding changes.
I think we can address this issue with lesser changes to the code by
handling port binding changes
first in the runtime data engine.

I think the I-P engine respects the order of inputs defined on an engine
node right ? If the engine guarantees
this, then I think we can take this approach.

What do you think ?

Thanks
Numan


>  controller/binding.c        | 74
> ++++++++++++++++++++++++++++++---------------
>  controller/ovn-controller.c | 58 +++++++++++++++++++++++++++++++++++
>  tests/ovn.at                | 38 +++++++++++++++++++++++
>  3 files changed, 145 insertions(+), 25 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 3c102dc..80a7898 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -837,24 +837,40 @@ claim_lport(const struct sbrec_port_binding *pb,
>              return false;
>          }
>
> -        if (pb->chassis) {
> -            VLOG_INFO("Changing chassis for lport %s from %s to %s.",
> -                    pb->logical_port, pb->chassis->name,
> -                    chassis_rec->name);
> -        } else {
> -            VLOG_INFO("Claiming lport %s for this chassis.",
> pb->logical_port);
> -        }
> -        for (int i = 0; i < pb->n_mac; i++) {
> -            VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]);
> -        }
> +        /* Update the port_binding with our chassis only if the port
> binding
> +         * is not being deleted from the SB.
> +         */
> +        if (!sbrec_port_binding_is_deleted(pb)) {
> +            if (pb->chassis) {
> +                VLOG_INFO("Changing chassis for lport %s from %s to %s.",
> +                          pb->logical_port, pb->chassis->name,
> +                          chassis_rec->name);
> +            } else {
> +                VLOG_INFO("Claiming lport %s for this chassis.",
> +                          pb->logical_port);
> +            }
> +            for (size_t i = 0; i < pb->n_mac; i++) {
> +                VLOG_INFO("%s: Claiming %s", pb->logical_port,
> pb->mac[i]);
> +            }
>
> -        sbrec_port_binding_set_chassis(pb, chassis_rec);
> +            sbrec_port_binding_set_chassis(pb, chassis_rec);
> +        }
>
> +        /* In any case, update tracked_datapaths as we need to track all
> types
> +         * of updates, including deletes.
> +         */
>          if (tracked_datapaths) {
>              update_lport_tracking(pb, tracked_datapaths);
>          }
>      }
>
> +    /* No need to check and update the encaps if the port_binding is being
> +     * deleted.
> +     */
> +    if (sbrec_port_binding_is_deleted(pb)) {
> +        return true;
> +    }
> +
>      /* Check if the port encap binding, if any, has changed */
>      struct sbrec_encap *encap_rec =
>          sbrec_get_port_encap(chassis_rec, iface_rec);
> @@ -879,27 +895,35 @@ static bool
>  release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
>                struct hmap *tracked_datapaths)
>  {
> -    if (pb->encap) {
> -        if (sb_readonly) {
> -            return false;
> +    /* Clear the port_binding encap, chassis, and virtual_parent fields
> +     * only if the port binding is not being deleted from the SB.
> +     */
> +    if (!sbrec_port_binding_is_deleted(pb)) {
> +        if (pb->encap) {
> +            if (sb_readonly) {
> +                return false;
> +            }
> +            sbrec_port_binding_set_encap(pb, NULL);
>          }
> -        sbrec_port_binding_set_encap(pb, NULL);
> -    }
>
> -    if (pb->chassis) {
> -        if (sb_readonly) {
> -            return false;
> +        if (pb->chassis) {
> +            if (sb_readonly) {
> +                return false;
> +            }
> +            sbrec_port_binding_set_chassis(pb, NULL);
>          }
> -        sbrec_port_binding_set_chassis(pb, NULL);
> -    }
>
> -    if (pb->virtual_parent) {
> -        if (sb_readonly) {
> -            return false;
> +        if (pb->virtual_parent) {
> +            if (sb_readonly) {
> +                return false;
> +            }
> +            sbrec_port_binding_set_virtual_parent(pb, NULL);
>          }
> -        sbrec_port_binding_set_virtual_parent(pb, NULL);
>      }
>
> +    /* In any case, update tracked_datapaths as we need to track all types
> +     * of updates, including deletes.
> +     */
>      update_lport_tracking(pb, tracked_datapaths);
>      VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
>      return true;
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index d299c61..37b09c4 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -73,6 +73,9 @@ static unixctl_cb_func extend_table_list;
>  static unixctl_cb_func inject_pkt;
>  static unixctl_cb_func engine_recompute_cmd;
>  static unixctl_cb_func cluster_state_reset_cmd;
> +static unixctl_cb_func debug_pause_execution;
> +static unixctl_cb_func debug_resume_execution;
> +static unixctl_cb_func debug_status_execution;
>
>  #define DEFAULT_BRIDGE_NAME "br-int"
>  #define DEFAULT_PROBE_INTERVAL_MSEC 5000
> @@ -2342,6 +2345,14 @@ main(int argc, char *argv[])
>                               cluster_state_reset_cmd,
>                               &reset_ovnsb_idl_min_index);
>
> +    bool paused = false;
> +    unixctl_command_register("debug/pause", "", 0, 0,
> debug_pause_execution,
> +                             &paused);
> +    unixctl_command_register("debug/resume", "", 0, 0,
> debug_resume_execution,
> +                             &paused);
> +    unixctl_command_register("debug/status", "", 0, 0,
> debug_status_execution,
> +                             &paused);
> +
>      unsigned int ovs_cond_seqno = UINT_MAX;
>      unsigned int ovnsb_cond_seqno = UINT_MAX;
>
> @@ -2350,6 +2361,15 @@ main(int argc, char *argv[])
>      restart = false;
>      bool sb_monitor_all = false;
>      while (!exiting) {
> +        /* If we're paused just run the unixctl server and skip most of
> the
> +         * processing loop.
> +         */
> +        if (paused) {
> +            unixctl_server_run(unixctl);
> +            unixctl_server_wait(unixctl);
> +            goto loop_done;
> +        }
> +
>          engine_init_run();
>
>          struct ovsdb_idl_txn *ovs_idl_txn =
> ovsdb_idl_loop_run(&ovs_idl_loop);
> @@ -2604,6 +2624,8 @@ main(int argc, char *argv[])
>
>          ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
>          ovsdb_idl_track_clear(ovs_idl_loop.idl);
> +
> +loop_done:
>          poll_block();
>          if (should_service_stop()) {
>              exiting = true;
> @@ -2857,3 +2879,39 @@ cluster_state_reset_cmd(struct unixctl_conn *conn,
> int argc OVS_UNUSED,
>      poll_immediate_wake();
>      unixctl_command_reply(conn, NULL);
>  }
> +
> +static void
> +debug_pause_execution(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +                      const char *argv[] OVS_UNUSED, void *paused_)
> +{
> +    bool *paused = paused_;
> +
> +    VLOG_INFO("User triggered execution pause.");
> +    *paused = true;
> +    unixctl_command_reply(conn, NULL);
> +}
> +
> +static void
> +debug_resume_execution(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +                       const char *argv[] OVS_UNUSED, void *paused_)
> +{
> +    bool *paused = paused_;
> +
> +    VLOG_INFO("User triggered execution resume.");
> +    *paused = false;
> +    poll_immediate_wake();
> +    unixctl_command_reply(conn, NULL);
> +}
> +
> +static void
> +debug_status_execution(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +                       const char *argv[] OVS_UNUSED, void *paused_)
> +{
> +    bool *paused = paused_;
> +
> +    if (*paused) {
> +        unixctl_command_reply(conn, "paused");
> +    } else {
> +        unixctl_command_reply(conn, "running");
> +    }
> +}
> diff --git a/tests/ovn.at b/tests/ovn.at
> index c4edbd9..5ad51c0 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -21371,3 +21371,41 @@ AT_CHECK([ovn-sbctl find mac ip=10.0.0.2
> mac='"00:00:00:00:03:02"' logical_port=
>  OVN_CLEANUP([hv1],[hv2])
>
>  AT_CLEANUP
> +
> +AT_SETUP([ovn -- Delete Port_Binding and OVS port Incremental Processing])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.10
> +
> +ovn-nbctl ls-add ls
> +ovn-nbctl lsp-add ls lsp
> +
> +as hv1 ovs-vsctl \
> +    -- add-port br-int vif1 \
> +    -- set Interface vif1 external_ids:iface-id=lsp
> +
> +# Wait for port to be bound.
> +OVS_WAIT_UNTIL([test $(ovn-sbctl --columns _uuid --bare list chassis hv1
> | wc -l) -eq 1])
> +ch=$(ovn-sbctl --columns _uuid --bare list chassis hv1)
> +OVS_WAIT_UNTIL([test $(ovn-sbctl --columns chassis --bare list
> port_binding lsp | grep $ch -c) -eq 1])
> +
> +# Pause ovn-controller.
> +as hv1 ovn-appctl -t ovn-controller debug/pause
> +
> +# Delete port binding and OVS port. The updates will be processed in the
> same
> +# loop in ovn-controller when it resumes.
> +ovn-nbctl --wait=sb lsp-del lsp
> +as hv1 ovs-vsctl del-port vif1
> +
> +# Resume ovn-controller.
> +as hv1 ovn-appctl -t ovn-controller debug/resume
> +
> +# Make sure ovn-controller runs fine.
> +OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status)
> = "xrunning"])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list