[ovs-dev] [PATCH ovn v4 7/7] northd: Add functionality to runtime node
Han Zhou
hzhou at ovn.org
Thu Sep 16 07:08:18 UTC 2021
On Fri, Sep 3, 2021 at 5:22 AM Mark Gray <mark.d.gray at redhat.com> wrote:
>
> Update the runtime node to initialize and destroy some common data
> used by multiple functions in northd.
Hi Mark,
It looks good overall as an initiate step to employ the inc-proc engine for
ovn-northd. It is great that it achieves the no-input-change-no-compute
outcome with this series.
However, for the en-runtime node it doesn't look right. I understand it is
only for demonstration purposes and is supposed to evolve, but it may look
misleading in how would we expand it further for I-P:
1) I think the I-P engine should focus on data dependency. Each node should
define its data (output data computed by the node's run()/change-handlers).
The en-runtime node defines its own data as lr_list, datapaths and ports,
but the run() function merely initiates empty data structures that are
later filled by en-northd, which is wrong. Instead, en-runtime should have
its own data computed according to its input (all those NB/SB tables), and
then en-northd should use en-runtime data in a read-only fashion. It is
better not adding this node if we don't have this data dependency defined.
2) en-northd run() calls ovn_db_run(), which accesses the inputs (DB
tables) using the northd_context instead of the data from its input nodes.
If we really want to use the engine for incremental processing, then I
think the first step is to make sure we never access data through any
global context in engine functions, and only use input data from the engine
nodes it depends on. The engine context should only contain idl_txn for
writing to DBs (even this could be improved so that we don't have any
global ctx in engines at all, but at least that's how we are living with in
ovn-controller). Otherwise it would be hard to ensure correctness of I-P.
For example, what if in ovn_db_run() it depends on some data from the
context that is not in any of the input nodes, and now if that data
changes, the ovn_db_run() won't be triggered any more because engine inputs
not changed, and this would be a bug.
Thanks,
Han
>
> Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
> ---
> northd/en-northd.c | 9 ++++++++-
> northd/en-runtime.c | 30 ++++++++++++++++++++++++++++--
> northd/en-runtime.h | 8 ++++++++
> northd/northd.c | 15 +++++----------
> northd/northd.h | 5 ++++-
> 5 files changed, 53 insertions(+), 14 deletions(-)
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index d310fa4dd31f..2a3250f3d57a 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -19,6 +19,7 @@
> #include <stdio.h>
>
> #include "en-northd.h"
> +#include "en-runtime.h"
> #include "lib/inc-proc-eng.h"
> #include "northd.h"
> #include "openvswitch/vlog.h"
> @@ -29,7 +30,13 @@ void en_northd_run(struct engine_node *node, void
*data OVS_UNUSED)
> {
> const struct engine_context *eng_ctx = engine_get_context();
> struct northd_context *ctx = eng_ctx->client_ctx;
> - ovn_db_run(ctx);
> +
> + struct ed_type_runtime *runtime_data =
> + engine_get_input_data("runtime", node);
> +
> + ovn_db_run(ctx, &runtime_data->lr_list,
> + &runtime_data->datapaths,
> + &runtime_data->ports);
>
> engine_set_node_state(node, EN_UPDATED);
>
> diff --git a/northd/en-runtime.c b/northd/en-runtime.c
> index aac01cd0351f..b8e5766823bf 100644
> --- a/northd/en-runtime.c
> +++ b/northd/en-runtime.c
> @@ -19,7 +19,9 @@
> #include <stdio.h>
>
> #include "en-runtime.h"
> +#include "openvswitch/hmap.h"
> #include "lib/inc-proc-eng.h"
> +#include "openvswitch/list.h"
> #include "northd.h"
> #include "openvswitch/vlog.h"
>
> @@ -27,14 +29,38 @@ VLOG_DEFINE_THIS_MODULE(en_runtime);
>
> void en_runtime_run(struct engine_node *node, void *data OVS_UNUSED)
> {
> + struct ed_type_runtime *runtime_data = data;
> +
> + struct ovs_list *lr_list = &runtime_data->lr_list;
> + struct hmap *datapaths = &runtime_data->datapaths;
> + struct hmap *ports = &runtime_data->ports;
> +
> + destroy_datapaths_and_ports(datapaths, ports, lr_list);
> +
> + ovs_list_init(lr_list);
> + hmap_init(datapaths);
> + hmap_init(ports);
> +
> engine_set_node_state(node, EN_UPDATED);
> }
> void *en_runtime_init(struct engine_node *node OVS_UNUSED,
> struct engine_arg *arg OVS_UNUSED)
> {
> - return NULL;
> + struct ed_type_runtime *data = xzalloc(sizeof *data);
> + ovs_list_init(&data->lr_list);
> + hmap_init(&data->datapaths);
> + hmap_init(&data->ports);
> +
> + return data;
> }
>
> -void en_runtime_cleanup(void *data OVS_UNUSED)
> +void en_runtime_cleanup(void *data)
> {
> + struct ed_type_runtime *runtime_data = data;
> +
> + struct ovs_list *lr_list = &runtime_data->lr_list;
> + struct hmap *datapaths = &runtime_data->datapaths;
> + struct hmap *ports = &runtime_data->ports;
> +
> + destroy_datapaths_and_ports(datapaths, ports, lr_list);
> }
> diff --git a/northd/en-runtime.h b/northd/en-runtime.h
> index 2547c9ec470a..7a1b2f6873e5 100644
> --- a/northd/en-runtime.h
> +++ b/northd/en-runtime.h
> @@ -7,7 +7,15 @@
> #include <stdlib.h>
> #include <stdio.h>
>
> +#include "openvswitch/hmap.h"
> #include "lib/inc-proc-eng.h"
> +#include "openvswitch/list.h"
> +
> +struct ed_type_runtime {
> + struct ovs_list lr_list;
> + struct hmap datapaths;
> + struct hmap ports;
> +};
>
> void en_runtime_run(struct engine_node *node, void *data);
> void *en_runtime_init(struct engine_node *node,
> diff --git a/northd/northd.c b/northd/northd.c
> index 829c4479f14b..43792f0d7ff7 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -13838,7 +13838,7 @@ sync_dns_entries(struct northd_context *ctx,
struct hmap *datapaths)
> hmap_destroy(&dns_map);
> }
>
> -static void
> +void
> destroy_datapaths_and_ports(struct hmap *datapaths, struct hmap *ports,
> struct ovs_list *lr_list)
> {
> @@ -14548,13 +14548,9 @@ ovnsb_db_run(struct northd_context *ctx,
> }
>
> void
> -ovn_db_run(struct northd_context *ctx)
> +ovn_db_run(struct northd_context *ctx, struct ovs_list *lr_list,
> + struct hmap *datapaths, struct hmap *ports)
> {
> - struct hmap datapaths, ports;
> - struct ovs_list lr_list;
> - ovs_list_init(&lr_list);
> - hmap_init(&datapaths);
> - hmap_init(&ports);
> use_parallel_build = ctx->use_parallel_build;
> lflow_locks = ctx->lflow_locks;
>
> @@ -14562,12 +14558,11 @@ ovn_db_run(struct northd_context *ctx)
>
> stopwatch_start(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
> ovnnb_db_run(ctx, ctx->sbrec_chassis_by_name, ctx->ovnsb_idl_loop,
> - &datapaths, &ports, &lr_list, start_time,
> + datapaths, ports, lr_list, start_time,
> ctx->ovn_internal_version);
> stopwatch_stop(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
> stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
> - ovnsb_db_run(ctx, ctx->ovnsb_idl_loop, &ports, start_time);
> + ovnsb_db_run(ctx, ctx->ovnsb_idl_loop, ports, start_time);
> stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
> - destroy_datapaths_and_ports(&datapaths, &ports, &lr_list);
> }
>
> diff --git a/northd/northd.h b/northd/northd.h
> index fa941d8ec83b..45a153ba2aa4 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -37,6 +37,9 @@ struct northd_context {
> const char *ovn_internal_version;
> };
>
> -void ovn_db_run(struct northd_context *ctx);
> +void ovn_db_run(struct northd_context *ctx, struct ovs_list *,
> + struct hmap *, struct hmap *);
> +void destroy_datapaths_and_ports(struct hmap *datapaths, struct hmap
*ports,
> + struct ovs_list *lr_list);
>
> #endif /* NORTHD_H */
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list