[ovs-dev] [PATCH ovn v4 7/7] northd: Add functionality to runtime node
Mark Gray
mark.d.gray at redhat.com
Thu Sep 16 14:32:05 UTC 2021
On 16/09/2021 08:08, Han Zhou wrote:
> On Fri, Sep 3, 2021 at 5:22 AM Mark Gray <mark.d.gray at redhat.com> wrote:
>>
Also, thanks to Han and Mark for reviewing!
>> 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