On Mon, Jul 31, 2023 at 8:18 PM Han Zhou <hz...@ovn.org> wrote:
>
> On Wed, Jul 19, 2023 at 2:01 AM Numan Siddique <num...@ovn.org> wrote:
> >
> > On Tue, Jul 18, 2023 at 11:08 AM Han Zhou <hz...@ovn.org> wrote:
> > >
> > > On Fri, Jul 7, 2023 at 1:56 PM <num...@ovn.org> wrote:
> > > >
> > > > From: Numan Siddique <num...@ovn.org>
> > > >
> > > > When a logical router gets updated due to load balancer
> > > > or load balancer groups changes, it is now incrementally
> > > > handled in the 'northd' engine node. Other logical router
> > > > updates result in the full recompute of 'northd' engine node.
> > > >
> > > > lflow engine node still falls back to recompute due to
> > > > logical router changes.
> > > >
> > > > Signed-off-by: Numan Siddique <num...@ovn.org>
> > > > ---
> > > > lib/lb.c | 32 +++-
> > > > lib/lb.h | 5 +
> > > > northd/en-lflow.c | 5 +
> > > > northd/en-northd.c | 20 ++
> > > > northd/en-northd.h | 1 +
> > > > northd/inc-proc-northd.c | 3 +-
> > > > northd/northd.c | 392
> ++++++++++++++++++++++++++++++++++-----
> > > > northd/northd.h | 5 +
> > > > tests/ovn-northd.at | 12 +-
> > > > 9 files changed, 422 insertions(+), 53 deletions(-)
> > > >
> > > > diff --git a/lib/lb.c b/lib/lb.c
> > > > index a0426cc42e..a8b694d0b3 100644
> > > > --- a/lib/lb.c
> > > > +++ b/lib/lb.c
> > > > @@ -1082,7 +1082,10 @@ ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths
> > > *lb_dps, size_t n,
> > > > struct ovn_datapath **ods)
> > > > {
> > > > for (size_t i = 0; i < n; i++) {
> > > > - bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> > > > + if (!bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> > > > + bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> > > > + lb_dps->n_nb_lr++;
> > > > + }
> > > > }
> > > > }
> > > >
> > > > @@ -1110,6 +1113,18 @@ ovn_lb_datapaths_remove_ls(struct
> ovn_lb_datapaths
> > > *lb_dps, size_t n,
> > > > }
> > > > }
> > > >
> > > > +void
> > > > +ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *lb_dps, size_t n,
> > > > + struct ovn_datapath **ods)
> > > > +{
> > > > + for (size_t i = 0; i < n; i++) {
> > > > + if (bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> > > > + bitmap_set0(lb_dps->nb_lr_map, ods[i]->index);
> > > > + lb_dps->n_nb_lr--;
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > struct ovn_lb_group_datapaths *
> > > > ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group,
> > > > size_t n_ls_datapaths,
> > > > @@ -1175,5 +1190,18 @@ void
> > > > ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps,
> > > > struct ovn_datapath *lr)
> > > > {
> > > > - bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > > + if (!bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> > > > + bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > > + lbg_dps->n_nb_lr++;
> > > > + }
> > > > +}
> > > > +
> > > > +void
> > > > +ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths
> *lbg_dps,
> > > > + struct ovn_datapath *lr)
> > > > +{
> > > > + if (bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> > > > + bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > > + lbg_dps->n_nb_lr--;
> > > > + }
> > > > }
> > > > diff --git a/lib/lb.h b/lib/lb.h
> > > > index 08723e8ef3..91eec0199b 100644
> > > > --- a/lib/lb.h
> > > > +++ b/lib/lb.h
> > > > @@ -185,6 +185,8 @@ void ovn_lb_datapaths_set_ls(struct
> ovn_lb_datapaths
> > > *, size_t n,
> > > >
> > > > void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n,
> > > > struct ovn_datapath **);
> > > > +void ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *, size_t n,
> > > > + struct ovn_datapath **);
> > > >
> > > > struct ovn_lb_group_datapaths {
> > > > struct hmap_node hmap_node;
> > > > @@ -214,6 +216,9 @@ void ovn_lb_group_datapaths_remove_ls(struct
> > > ovn_lb_group_datapaths *,
> > > >
> > > > void ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *,
> > > > struct ovn_datapath *lr);
> > > > +void ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths
> *,
> > > > + struct ovn_datapath *lr);
> > > > +
> > > > struct ovn_controller_lb {
> > > > struct hmap_node hmap_node;
> > > >
> > > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > > > index db1bcbccd6..ea51f4e3e0 100644
> > > > --- a/northd/en-lflow.c
> > > > +++ b/northd/en-lflow.c
> > > > @@ -102,6 +102,11 @@ lflow_northd_handler(struct engine_node *node,
> > > > if (!northd_data->change_tracked) {
> > > > return false;
> > > > }
> > > > +
> > > > + if (northd_data->lrouters_changed) {
> > > > + return false;
> > > > + }
> > >
> > > I think lrouters_changed is not needed. If lrouters related data is
> > > changed, we should set change_tracked to false (call the
> > > destroy_northd_data_tracked_changes), because lrouter data is part of
> > > northd_data nad we don't track lrouter changes yet.
> >
> >
> > - With this patch we could are handling router changes
> > incrementally in northd engine node if only the router's load_balancer
> > column has changed.
> > If we set change_tracked to false, it means northd engine node
> > didn't handle the router changes due to load balancer column
> > incrementally and it fell back to full recompute which is not
>
> "change_tracked", as the name suggests, only indicates if the changes are
> tracked, and the purpose is for the dependent node to decide how to handle
> the changes (usually if the changes are not tracked the dependent node
> couldn't handle the changes incrementally). I think "change_tracked"
> doesn't necessarily tell if the current node is recomputed or not.
We can definitely find a better name to indicate to the dependent nodes
about it.
In the v4 there is no "lrouters_changed" but the northd tracking data has
a different field by the name - "lb_changes" which indicates to the
dependent nodes
that the northd engine data has changes to the "load balancers" in the
engine run.
Maybe we can discuss further in v4. But a few points
- northd engine node has a handler for lb_data changes which sets
the northd_tracked_data->lb_changes = true
- northd engine node has a handler for ls changes where the
handler tracks the changed VIF ports.
When lflow engine handler for the northd engine node is called, it
can decide what to do with the provided data.
Thanks
Numan
>
> > the cause. This 'lrounters_changed' indicates to the lflow engine
> > node
> > that northd engine node did a compute (and not a recompute) and
> > logical routers have changed as part of this compute.
> >
>
> Why do we need to maintain the status of whether the node is recomputed in
> such a variable?
>
> > - Since lflow engine node doesn't yet handle the lrouter changes,
> > it falls back to full recompute.
>
> I understand, but northd_data->change_tracked == false should just trigger
> the lflow node recompute, because the input changed but the changes are not
> tracked and obviously we don't know how to handle that in the lflow node.
>
> >
> > - It is also possible that in the same nb db update, a load
> > balancer was updated which is associated with both logical switch and
> > logical router.
> > In such cases we don't want to call -
> > destroy_northd_data_tracked_changes(). Even though lflow engine does
>
> Why don't we want to call destroy_northd_data_tracked_changes()? If there
> are changes already handled incrementally and tracked, and now we know we
> can't handle the LR/LS related changes (due to LB changes associated with
> LR/LS), it is ok to release the "partially" tracked changes, right?
>
> > a recompute, from northd engine point of view,
> > it is indicating what changed. It is left to the dependent nodes
> > to decide if they want to handle incrementally or fall back to
> > recompute.
> >
> Yes, dependent nodes will decide, but I think change_tracked (and tracked
> changes) is sufficient for them to make the decision.
>
> (Sorry for my late response because I was just back from vacation)
>
> Thanks,
> Han
>
> > Let me know if you disagree with this.
> >
> >
> > >
> > > > +
> > > > const struct engine_context *eng_ctx = engine_get_context();
> > > > struct lflow_data *lflow_data = data;
> > > >
> > > > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > > > index b3c03c54bd..a321eff323 100644
> > > > --- a/northd/en-northd.c
> > > > +++ b/northd/en-northd.c
> > > > @@ -188,6 +188,26 @@ northd_nb_logical_switch_handler(struct
> engine_node
> > > *node,
> > > > return true;
> > > > }
> > > >
> > > > +bool
> > > > +northd_nb_logical_router_handler(struct engine_node *node,
> > > > + void *data)
> > > > +{
> > > > + struct northd_data *nd = data;
> > > > + struct northd_input input_data;
> > > > +
> > > > + northd_get_input_data(node, &input_data);
> > > > +
> > > > + if (!northd_handle_lr_changes(&input_data, nd)) {
> > > > + return false;
> > > > + }
> > > > +
> > > > + if (nd->change_tracked) {
> > > > + engine_set_node_state(node, EN_UPDATED);
> > >
> > > change_tracked is supposed to indicate if the changes in the engine
> node is
> > > tracked, meaning the node depending on this one can potentailly
> > > incrementally process the changes.
> > > It is possible that the node is updated but the changes are not
> tracked. In
> > > this case most likely the nodes depending on it will need to be
> recomputed.
> > > Whether the node state is EN_UPDATED depends on if the node is updated.
> I
> > > think the northd_handle_lr_changes can take a parameter "updated" to
> > > indicate if it is updated.
> >
> > Agree. Will use "updated" parameter instead.
> >
> > Thanks
> > Numan
> >
> >
> >
> > >
> > > > + }
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > bool
> > > > northd_sb_port_binding_handler(struct engine_node *node,
> > > > void *data)
> > > > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > > > index 5674f4390c..739aa5e87f 100644
> > > > --- a/northd/en-northd.h
> > > > +++ b/northd/en-northd.h
> > > > @@ -16,6 +16,7 @@ void en_northd_cleanup(void *data);
> > > > void en_northd_clear_tracked_data(void *data);
> > > > bool northd_nb_nb_global_handler(struct engine_node *, void *data
> > > OVS_UNUSED);
> > > > bool northd_nb_logical_switch_handler(struct engine_node *, void
> *data);
> > > > +bool northd_nb_logical_router_handler(struct engine_node *, void
> *data);
> > > > bool northd_sb_port_binding_handler(struct engine_node *, void
> *data);
> > > > bool northd_northd_lb_data_handler(struct engine_node *, void *data);
> > > >
> > > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > > > index f5b7998f1f..362dd5e87a 100644
> > > > --- a/northd/inc-proc-northd.c
> > > > +++ b/northd/inc-proc-northd.c
> > > > @@ -156,7 +156,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
> *nb,
> > > >
> > > > engine_add_input(&en_northd, &en_nb_port_group, NULL);
> > > > engine_add_input(&en_northd, &en_nb_acl, NULL);
> > > > - engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> > > > engine_add_input(&en_northd, &en_nb_mirror, NULL);
> > > > engine_add_input(&en_northd, &en_nb_meter, NULL);
> > > > engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL);
> > > > @@ -185,6 +184,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
> *nb,
> > > > northd_nb_nb_global_handler);
> > > > engine_add_input(&en_northd, &en_nb_logical_switch,
> > > > northd_nb_logical_switch_handler);
> > > > + engine_add_input(&en_northd, &en_nb_logical_router,
> > > > + northd_nb_logical_router_handler);
> > > >
> > > > engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
> > > > engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding,
> NULL);
> > > > diff --git a/northd/northd.c b/northd/northd.c
> > > > index a7761285de..4393adc2ad 100644
> > > > --- a/northd/northd.c
> > > > +++ b/northd/northd.c
> > > > @@ -4021,6 +4021,90 @@ associate_ls_lb_groups(struct ovn_datapath
> *ls_od,
> > > > }
> > > > }
> > > >
> > > > +static void
> > > > +associate_lr_lbs(struct ovn_datapath *lr_od, struct hmap
> > > *lb_datapaths_map)
> > > > +{
> > > > + struct ovn_lb_datapaths *lb_dps;
> > > > +
> > > > + free(lr_od->lb_uuids);
> > > > + lr_od->lb_uuids = xcalloc(lr_od->nbr->n_load_balancer,
> > > > + sizeof *lr_od->lb_uuids);
> > > > + lr_od->n_lb_uuids = lr_od->nbr->n_load_balancer;
> > > > +
> > > > + if (!lr_od->lb_ips) {
> > > > + lr_od->lb_ips = ovn_lb_ip_set_create();
> > > > + }
> > > > +
> > > > + for (size_t i = 0; i < lr_od->nbr->n_load_balancer; i++) {
> > > > + const struct uuid *lb_uuid =
> > > > + &lr_od->nbr->load_balancer[i]->header_.uuid;
> > > > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > > > + ovs_assert(lb_dps);
> > > > + ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> > > > + build_lrouter_lb_ips(lr_od->lb_ips, lb_dps->lb);
> > > > + lr_od->lb_uuids[i] = *lb_uuid;
> > > > + }
> > > > +}
> > > > +
> > > > +static void
> > > > +associate_lr_lb_groups(struct ovn_datapath *lr_od,
> > > > + struct hmap *lb_group_datapaths_map,
> > > > + struct hmap *lb_datapaths_map)
> > > > +{
> > > > + const struct nbrec_load_balancer_group *nbrec_lb_group;
> > > > + struct ovn_lb_group_datapaths *lb_group_dps;
> > > > +
> > > > + free(lr_od->lb_group_uuids);
> > > > + lr_od->lb_group_uuids =
> xcalloc(lr_od->nbr->n_load_balancer_group,
> > > > + sizeof *lr_od->lb_group_uuids);
> > > > + lr_od->n_lb_group_uuids = lr_od->nbr->n_load_balancer_group;
> > > > +
> > > > + /* Checking load balancer groups first, starting from the largest
> > > one,
> > > > + * to more efficiently copy IP sets. */
> > > > + size_t largest_group = 0;
> > > > +
> > > > + for (size_t i = 1; i < lr_od->nbr->n_load_balancer_group; i++) {
> > > > + if (lr_od->nbr->load_balancer_group[i]->n_load_balancer >
> > > > +
> > > lr_od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> > > > + largest_group = i;
> > > > + }
> > > > + }
> > > > +
> > > > + for (size_t i = 0; i < lr_od->nbr->n_load_balancer_group; i++) {
> > > > + size_t idx = (i + largest_group) %
> > > lr_od->nbr->n_load_balancer_group;
> > > > +
> > > > + nbrec_lb_group = lr_od->nbr->load_balancer_group[idx];
> > > > + const struct uuid *lb_group_uuid =
> &nbrec_lb_group->header_.uuid;
> > > > +
> > > > + lb_group_dps =
> > > > + ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > + lb_group_uuid);
> > > > + ovs_assert(lb_group_dps);
> > > > + ovn_lb_group_datapaths_add_lr(lb_group_dps, lr_od);
> > > > +
> > > > + if (!lr_od->lb_ips) {
> > > > + lr_od->lb_ips =
> > > > + ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> > > > + } else {
> > > > + for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs;
> j++) {
> > > > + build_lrouter_lb_ips(lr_od->lb_ips,
> > > > + lb_group_dps->lb_group->lbs[j]);
> > > > + }
> > > > + }
> > > > +
> > > > + lr_od->lb_group_uuids[i] = *lb_group_uuid;
> > > > +
> > > > + struct ovn_lb_datapaths *lb_dps;
> > > > + for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> > > > + const struct uuid *lb_uuid =
> > > > + &lb_group_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > > > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> lb_uuid);
> > > > + ovs_assert(lb_dps);
> > > > + ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > static void
> > > > build_lb_datapaths(const struct hmap *lbs, const struct hmap
> *lb_groups,
> > > > struct ovn_datapaths *ls_datapaths,
> > > > @@ -4028,7 +4112,6 @@ build_lb_datapaths(const struct hmap *lbs, const
> > > struct hmap *lb_groups,
> > > > struct hmap *lb_datapaths_map,
> > > > struct hmap *lb_group_datapaths_map)
> > > > {
> > > > - const struct nbrec_load_balancer_group *nbrec_lb_group;
> > > > struct ovn_lb_group_datapaths *lb_group_dps;
> > > > const struct ovn_lb_group *lb_group;
> > > > struct ovn_lb_datapaths *lb_dps;
> > > > @@ -4062,53 +4145,8 @@ build_lb_datapaths(const struct hmap *lbs,
> const
> > > struct hmap *lb_groups,
> > > >
> > > > HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
> > > > ovs_assert(od->nbr);
> > > > -
> > > > - /* Checking load balancer groups first, starting from the
> > > largest one,
> > > > - * to more efficiently copy IP sets. */
> > > > - size_t largest_group = 0;
> > > > -
> > > > - for (size_t i = 1; i < od->nbr->n_load_balancer_group; i++) {
> > > > - if (od->nbr->load_balancer_group[i]->n_load_balancer >
> > > > -
> > > od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> > > > - largest_group = i;
> > > > - }
> > > > - }
> > > > -
> > > > - for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) {
> > > > - size_t idx = (i + largest_group) %
> > > od->nbr->n_load_balancer_group;
> > > > -
> > > > - nbrec_lb_group = od->nbr->load_balancer_group[idx];
> > > > - const struct uuid *lb_group_uuid =
> > > &nbrec_lb_group->header_.uuid;
> > > > -
> > > > - lb_group_dps =
> > > > - ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > - lb_group_uuid);
> > > > - ovs_assert(lb_group_dps);
> > > > - ovn_lb_group_datapaths_add_lr(lb_group_dps, od);
> > > > -
> > > > - if (!od->lb_ips) {
> > > > - od->lb_ips =
> > > > -
> ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> > > > - } else {
> > > > - for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs;
> > > j++) {
> > > > - build_lrouter_lb_ips(od->lb_ips,
> > > > -
> lb_group_dps->lb_group->lbs[j]);
> > > > - }
> > > > - }
> > > > - }
> > > > -
> > > > - if (!od->lb_ips) {
> > > > - od->lb_ips = ovn_lb_ip_set_create();
> > > > - }
> > > > -
> > > > - for (size_t i = 0; i < od->nbr->n_load_balancer; i++) {
> > > > - const struct uuid *lb_uuid =
> > > > - &od->nbr->load_balancer[i]->header_.uuid;
> > > > - lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> lb_uuid);
> > > > - ovs_assert(lb_dps);
> > > > - ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
> > > > - build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
> > > > - }
> > > > + associate_lr_lb_groups(od, lb_group_datapaths_map,
> > > lb_datapaths_map);
> > > > + associate_lr_lbs(od, lb_datapaths_map);
> > > > }
> > > >
> > > > HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_datapaths_map) {
> > > > @@ -4978,6 +5016,7 @@ destroy_northd_data_tracked_changes(struct
> > > northd_data *nd)
> > > > }
> > > >
> > > > nd->change_tracked = false;
> > > > + nd->lrouters_changed = false;
> > > > }
> > > >
> > > > /* Check if a changed LSP can be handled incrementally within the I-P
> > > engine
> > > > @@ -5513,6 +5552,263 @@ fail:
> > > > return false;
> > > > }
> > > >
> > > > +/* Returns true if the logical router has changes which are not
> > > > + * incrementally handled.
> > > > + * Presently supports i-p for the below changes:
> > > > + * - load balancers and load balancer groups.
> > > > + */
> > > > +static bool
> > > > +check_unsupported_inc_proc_for_lr_changes(
> > > > + const struct nbrec_logical_router *lr)
> > > > +{
> > > > + /* Check if the columns are changed in this row. */
> > > > + enum nbrec_logical_router_column_id col;
> > > > + for (col = 0; col < NBREC_LOGICAL_ROUTER_N_COLUMNS; col++) {
> > > > + if (nbrec_logical_router_is_updated(lr, col)) {
> > > > + if (col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER ||
> > > > + col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP)
> {
> > > > + continue;
> > > > + }
> > > > + return true;
> > > > + }
> > > > + }
> > > > +
> > > > + /* Check if the referenced rows are changed.
> > > > + XXX: Need a better OVSDB IDL interface for this check. */
> > > > + for (size_t i = 0; i < lr->n_ports; i++) {
> > > > + if (nbrec_logical_router_port_row_get_seqno(lr->ports[i],
> > > > + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > + return true;
> > > > + }
> > > > + }
> > > > + if (lr->copp && nbrec_copp_row_get_seqno(lr->copp,
> > > > + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > + return true;
> > > > + }
> > > > + for (size_t i = 0; i < lr->n_nat; i++) {
> > > > + if (nbrec_nat_row_get_seqno(lr->nat[i],
> > > > + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > + return true;
> > > > + }
> > > > + }
> > > > + for (size_t i = 0; i < lr->n_policies; i++) {
> > > > + if
> (nbrec_logical_router_policy_row_get_seqno(lr->policies[i],
> > > > + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > + return true;
> > > > + }
> > > > + }
> > > > + for (size_t i = 0; i < lr->n_static_routes; i++) {
> > > > + if (nbrec_logical_router_static_route_row_get_seqno(
> > > > + lr->static_routes[i], OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > + return true;
> > > > + }
> > > > + }
> > > > + return false;
> > > > +}
> > > > +
> > > > +static bool
> > > > +lr_check_and_handle_lb_and_lbgrp_changes(
> > > > + const struct nbrec_logical_router *changed_lr,
> > > > + struct hmap *lb_datapaths_map, struct hmap
> *lb_group_datapaths_map,
> > > > + struct ovn_datapath *od, bool *updated)
> > > > +{
> > > > + ovs_assert(od->nbr);
> > > > + bool lbs_changed = true;
> > > > + if (!nbrec_logical_router_is_updated(changed_lr,
> > > > + NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER) &&
> > > > + !nbrec_logical_router_is_updated(changed_lr,
> > > > +
> NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP)) {
> > > > + lbs_changed = false;
> > > > + for (size_t i = 0; i < changed_lr->n_load_balancer_group;
> i++) {
> > > > + if (nbrec_load_balancer_group_row_get_seqno(
> > > > + changed_lr->load_balancer_group[i],
> > > > + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > + lbs_changed = true;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + if (!lbs_changed) {
> > > > + for (size_t i = 0; i < changed_lr->n_load_balancer; i++)
> {
> > > > + if (nbrec_load_balancer_row_get_seqno(
> > > > + changed_lr->load_balancer[i],
> > > > + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > > > + lbs_changed = true;
> > > > + break;
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > + if (!lbs_changed) {
> > > > + return true;
> > > > + }
> > > > + }
> > > > +
> > > > + /* We need to do 2 things to handle the router for load balancer
> or
> > > > + * load balancer groups.
> > > > + *
> > > > + * 1. Disassociate the logical router datapath from the already
> > > associated
> > > > + * load balancers/load balancer groups.
> > > > + * 2. Associate the logical router datapath with the updated
> > > > + * load balancers/groups.
> > > > + * */
> > > > + struct ovn_lb_datapaths *lb_dps;
> > > > + size_t i, j;
> > > > + for (i = 0; i < od->n_lb_uuids; i++) {
> > > > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> > > &od->lb_uuids[i]);
> > > > + if (lb_dps) {
> > > > + if (lb_dps->lb->routable) {
> > > > + /* Can't handler if the load balancer has routable
> flag
> > > set.
> > > > + * This requires updating the router ports's routable
> > > > + * addresses. */
> > > > + return false;
> > > > + }
> > > > + ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> > > > + }
> > > > + }
> > > > +
> > > > + struct ovn_lb_group_datapaths *lbg_dps;
> > > > + for (i = 0; i < od->n_lb_group_uuids; i++) {
> > > > + lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > +
> &od->lb_group_uuids[i]);
> > > > + if (lbg_dps) {
> > > > + ovn_lb_group_datapaths_remove_lr(lbg_dps, od);
> > > > +
> > > > + if (install_ls_lb_from_router && od->n_ls_peers) {
> > > > + ovn_lb_group_datapaths_remove_ls(lbg_dps,
> od->n_ls_peers,
> > > > + od->ls_peers);
> > > > + }
> > > > + }
> > > > +
> > > > + for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> > > > + const struct uuid *lb_uuid =
> > > > + &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > > > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> lb_uuid);
> > > > + if (lb_dps) {
> > > > + if (lb_dps->lb->routable) {
> > > > + /* Can't handler if the load balancer has
> routable
> > > flag
> > > > + * set. This requires updating the router
> ports's
> > > > + * routable addresses. */
> > > > + return false;
> > > > + }
> > > > + ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> > > > +
> > > > + if (install_ls_lb_from_router && od->n_ls_peers) {
> > > > + ovn_lb_datapaths_remove_ls(lb_dps,
> od->n_ls_peers,
> > > > + od->ls_peers);
> > > > + }
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > + ovn_lb_ip_set_destroy(od->lb_ips);
> > > > + od->lb_ips = NULL;
> > > > + associate_lr_lb_groups(od, lb_group_datapaths_map,
> lb_datapaths_map);
> > > > + associate_lr_lbs(od, lb_datapaths_map);
> > > > +
> > > > + /* Do the job of build_lrouter_lbs_reachable_ips and
> > > > + * build_lswitch_lbs_from_lrouter for this lrouter datapath. */
> > > > + for (i = 0; i < od->nbr->n_load_balancer; i++) {
> > > > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> > > > +
> > > &od->nbr->load_balancer[i]->header_.uuid);
> > > > + ovs_assert(lb_dps);
> > > > + if (lb_dps->lb->routable) {
> > > > + /* Can't handler if the load balancer has routable flag
> set.
> > > > + * This requires updating the router ports's routable
> > > addresses. */
> > > > + return false;
> > > > + }
> > > > + build_lrouter_lb_reachable_ips(od, lb_dps->lb);
> > > > +
> > > > + if (install_ls_lb_from_router && od->n_ls_peers) {
> > > > + ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
> > > od->ls_peers);
> > > > + }
> > > > + }
> > > > +
> > > > + for (i = 0; i < od->nbr->n_load_balancer_group; i++) {
> > > > + const struct nbrec_load_balancer_group *nbrec_lb_group =
> > > > + od->nbr->load_balancer_group[i];
> > > > + lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > > > +
> > > &nbrec_lb_group->header_.uuid);
> > > > + ovs_assert(lbg_dps);
> > > > + if (install_ls_lb_from_router && od->n_ls_peers) {
> > > > + ovn_lb_group_datapaths_add_ls(lbg_dps, od->n_ls_peers,
> > > > + od->ls_peers);
> > > > + }
> > > > +
> > > > + for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> > > > + build_lrouter_lb_reachable_ips(od,
> > > lbg_dps->lb_group->lbs[j]);
> > > > +
> > > > + if (install_ls_lb_from_router && od->n_ls_peers) {
> > > > + const struct uuid *lb_uuid =
> > > > + &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> > > > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> > > lb_uuid);
> > > > + ovs_assert(lb_dps);
> > > > + if (lb_dps->lb->routable) {
> > > > + /* Can't handler if the load balancer has
> routable
> > > flag
> > > > + * set. This requires updating the router
> ports's
> > > > + * routable addresses. */
> > > > + return false;
> > > > + }
> > > > + ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
> > > od->ls_peers);
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > + *updated = true;
> > > > + /* Re-evaluate 'od->has_lb_vip' */
> > > > + init_lb_for_datapath(od);
> > > > + return true;
> > > > +}
> > > > +
> > > > +
> > > > +/* Return true if changes are handled incrementally, false otherwise.
> > > > + * When there are any changes, try to track what's exactly changed
> and
> > > set
> > > > + * northd_data->change_tracked accordingly: change tracked - true,
> > > otherwise,
> > > > + * false. */
> > > > +bool
> > > > +northd_handle_lr_changes(const struct northd_input *ni,
> > > > + struct northd_data *nd)
> > > > +{
> > > > + const struct nbrec_logical_router *changed_lr;
> > > > +
> > > > + bool updated = false;
> > > > +
> > > > + NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH_TRACKED (changed_lr,
> > > > +
> > > ni->nbrec_logical_router_table) {
> > > > + if (nbrec_logical_router_is_new(changed_lr) ||
> > > > + nbrec_logical_router_is_deleted(changed_lr)) {
> > > > + goto fail;
> > > > + }
> > > > +
> > > > + struct ovn_datapath *od = ovn_datapath_find(
> > > > + &nd->lr_datapaths.datapaths,
> > > > + &changed_lr->header_.uuid);
> > > > +
> > > > + /* Presently only able to handle load balancer and
> > > > + * load balancer group changes. */
> > > > + if (check_unsupported_inc_proc_for_lr_changes(changed_lr)) {
> > > > + goto fail;
> > > > + }
> > > > +
> > > > + if (!lr_check_and_handle_lb_and_lbgrp_changes(changed_lr,
> > > > +
> &nd->lb_datapaths_map,
> > > > +
> > > &nd->lb_group_datapaths_map,
> > > > + od, &updated)) {
> > > > + goto fail;
> > > > + }
> > > > + }
> > > > +
> > > > + if (updated) {
> > > > + nd->change_tracked = true;
> > >
> > > Updated doesn't mean the change is tracked. Please see the earlier
> comments.
> > >
> > > > + nd->lrouters_changed = true;
> > > > + }
> > > > +
> > > > + return true;
> > > > +fail:
> > > > + destroy_northd_data_tracked_changes(nd);
> > >
> > > I think we don't need "fail:" here. Instead, just return false and call
> the
> > > destroy_northd_data_tracked_changes if lrouter is updated (but not
> tracked)
> > > or can't be handled.
> > >
> > > Thanks,
> > > Han
> > >
> > > > + return false;
> > > > +}
> > > > +
> > > > bool
> > > > northd_handle_sb_port_binding_changes(
> > > > const struct sbrec_port_binding_table *sbrec_port_binding_table,
> > > > diff --git a/northd/northd.h b/northd/northd.h
> > > > index a79cc470c3..765720849a 100644
> > > > --- a/northd/northd.h
> > > > +++ b/northd/northd.h
> > > > @@ -119,8 +119,11 @@ struct northd_data {
> > > > struct chassis_features features;
> > > > struct sset svc_monitor_lsps;
> > > > struct hmap svc_monitor_map;
> > > > +
> > > > + /* Tracked data. */
> > > > bool change_tracked;
> > > > struct tracked_ls_changes tracked_ls_changes;
> > > > + bool lrouters_changed;
> > > > };
> > > >
> > > > struct lflow_data {
> > > > @@ -342,6 +345,8 @@ void ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> > > > bool northd_handle_ls_changes(struct ovsdb_idl_txn *,
> > > > const struct northd_input *,
> > > > struct northd_data *);
> > > > +bool northd_handle_lr_changes(const struct northd_input *,
> > > > + struct northd_data *);
> > > > void destroy_northd_data_tracked_changes(struct northd_data *);
> > > > void northd_destroy(struct northd_data *data);
> > > > void northd_init(struct northd_data *data);
> > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > > index d3a5851e35..286b311242 100644
> > > > --- a/tests/ovn-northd.at
> > > > +++ b/tests/ovn-northd.at
> > > > @@ -9760,6 +9760,14 @@ check as northd ovn-appctl -t NORTHD_TYPE
> > > inc-engine/clear-stats
> > > > check ovn-nbctl --wait=sb ls-lb-del sw0 lb1
> > > > check_engine_stats norecompute recompute
> > > >
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl lr-lb-add lr0 lb1
> > > > +check_engine_stats norecompute recompute
> > > > +
> > > > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > +check ovn-nbctl lr-lb-del lr0 lb1
> > > > +check_engine_stats norecompute recompute
> > > > +
> > > > # Test load balancer group now
> > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > lbg1_uuid=$(ovn-nbctl create load_balancer_group name=lbg1)
> > > > @@ -9796,11 +9804,11 @@ check_engine_stats norecompute recompute
> > > >
> > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid
> > > > -check_engine_stats recompute recompute
> > > > +check_engine_stats norecompute recompute
> > > >
> > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > > > check ovn-nbctl clear logical_router lr0 load_balancer_group
> > > > -check_engine_stats recompute recompute
> > > > +check_engine_stats norecompute recompute
> > > >
> > > > AT_CLEANUP
> > > > ])
> > > > --
> > > > 2.40.1
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > d...@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > _______________________________________________
> > > dev mailing list
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
