Hi Mark,
thank you for the review.
On Wed, Jun 11, 2025 at 12:10 AM Mark Michelson <mmich...@redhat.com> wrote:
> On 5/21/25 08:59, Ales Musil via dev wrote:
> > Handle ACL changes in port group incrementally. The logical switch
> > ACL changes are already handled, add similar method for port groups
> > which should prevent lflow recomputes. There is still lflow dependency
> > on ACL table that results in full lflow recompute. That will be solved
> > in future commit.
> >
> > Signed-off-by: Ales Musil <amu...@redhat.com>
> > ---
> > v3: Rebase on top of latest main.
> > v2: Rebase on top of latest main.
> > ---
> > northd/en-northd.c | 24 ++++++++++++
> > northd/en-northd.h | 2 +
> > northd/inc-proc-northd.c | 2 +
> > northd/northd.c | 56 +++++++++++++++++++++++++++
> > northd/northd.h | 3 ++
> > tests/ovn-northd.at | 81 +++++++++++++++++++++++++++++++++++-----
> > 6 files changed, 159 insertions(+), 9 deletions(-)
> >
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index 3359d8d0e..0f1425ec7 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -78,6 +78,8 @@ northd_get_input_data(struct engine_node *node,
> > EN_OVSDB_GET(engine_get_input("NB_mirror", node));
> > input_data->nbrec_mirror_rule_table =
> > EN_OVSDB_GET(engine_get_input("NB_mirror_rule", node));
> > + input_data->nbrec_port_gorup_table =
>
> s/gorup/group/ here and every place this field is used.
>
Fixed in v4.
>
> > + EN_OVSDB_GET(engine_get_input("NB_port_group", node));
> >
> > input_data->sbrec_datapath_binding_table =
> > EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
> > @@ -238,6 +240,28 @@ northd_global_config_handler(struct engine_node
> *node, void *data OVS_UNUSED)
> > return EN_HANDLED_UNCHANGED;
> > }
> >
> > +enum engine_input_handler_result
> > +northd_nb_port_group_handler(struct engine_node *node, void *data)
> > +{
> > + struct northd_data *nd = data;
> > +
> > + struct northd_input input_data;
> > + northd_get_input_data(node, &input_data);
> > +
> > + /* This handler cares only about ACLs, the port group itself has
> separate
> > + * node. */
> > + if (!northd_handle_pgs_acl_changes(&input_data, nd)) {
> > + return EN_UNHANDLED;
> > + }
> > +
> > + if (northd_has_tracked_data(&nd->trk_data)) {
> > + return EN_HANDLED_UPDATED;
> > + }
> > +
> > + return EN_HANDLED_UNCHANGED;
> > +}
> > +
> > +
> > enum engine_input_handler_result
> > route_policies_northd_change_handler(struct engine_node *node,
> > void *data OVS_UNUSED)
> > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > index b19b73270..4f744a6c5 100644
> > --- a/northd/en-northd.h
> > +++ b/northd/en-northd.h
> > @@ -26,6 +26,8 @@ northd_sb_port_binding_handler(struct engine_node *,
> void *data);
> > enum engine_input_handler_result northd_lb_data_handler(struct
> engine_node *,
> > void *data);
> > enum engine_input_handler_result
> > +northd_nb_port_group_handler(struct engine_node *node, void *data);
> > +enum engine_input_handler_result
> > northd_sb_fdb_change_handler(struct engine_node *node, void *data);
> > void *en_routes_init(struct engine_node *node OVS_UNUSED,
> > struct engine_arg *arg OVS_UNUSED);
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index 5905462ec..be2f36e33 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -246,6 +246,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > engine_add_input(&en_northd, &en_nb_logical_router,
> > northd_nb_logical_router_handler);
> > engine_add_input(&en_northd, &en_lb_data, northd_lb_data_handler);
> > + engine_add_input(&en_northd, &en_nb_port_group,
> > + northd_nb_port_group_handler);
> >
> > engine_add_input(&en_lr_nat, &en_northd, lr_nat_northd_handler);
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 919049137..e3c5b9426 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -5216,6 +5216,62 @@ fail:
> > return false;
> > }
> >
> > +static bool
> > +is_pg_acls_changed(const struct nbrec_port_group *npg) {
> > +
> > + return (nbrec_port_group_is_updated(npg, NBREC_PORT_GROUP_COL_ACLS)
> > + || is_acls_seqno_changed(npg->acls, npg->n_acls));
> > +}
> > +
> > +bool
> > +northd_handle_pgs_acl_changes(const struct northd_input *ni,
> > + struct northd_data *nd)
> > +{
> > + const struct nbrec_port_group *nb_pg;
> > + struct northd_tracked_data *trk_data = &nd->trk_data;
> > +
> > + NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg,
> > +
> ni->nbrec_port_gorup_table) {
> > + if (!is_pg_acls_changed(nb_pg)) {
> > + continue;
> > + }
>
> I'm curious what happens here if a new port group is created with an
> ACL, such as:
>
> ovn-nbctl pg-add pg lsp0 -- \
> ovn-nbctl set Port_Group pg acls=<some_acl>
>
> In this case, is_pg_acls_changed() should return true, since the port
> group is not updated (it is new), but its ACLs column is modified.
>
> Is this intended? I don't see a test case for this in the modifications
> to ovn-northd.at.
>
So there is no harm as the port group addition/deletion will trigger
recompute
of lflow node anyway as it still cannot be processed incrementally.
> One difference between northd_handle_pgs_acl_changes() and
> northd_handle_ls_changes() is that northd_handle_ls_changes() only calls
> is_ls_acl_changed() on logical switches that are known not to be new or
> deleted. Should northd_handle_pgs_acl_changes() do something similar?
>
That's a fair point, even as I said there shouldn't be any harm we can
still skip
those to make it more obvious so I have added that check in v4.
>
> > +
> > + for (size_t i = 0; i < nb_pg->n_ports; i++) {
> > + const char *port_name = nb_pg->ports[i]->name;
> > + const struct ovn_datapath *od =
> > + northd_get_datapath_for_port(&nd->ls_ports, port_name);
> > +
> > + if (!od) {
> > + static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 1);
> > + VLOG_ERR_RL(&rl, "lport %s in port group %s not found.",
> > + port_name, nb_pg->name);
> > + goto fail;
> > + }
> > +
> > + if (!od->nbs) {
> > + static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 1);
> > + VLOG_WARN_RL(&rl, "lport %s in port group %s has no
> lswitch.",
> > + nb_pg->ports[i]->name,
> > + nb_pg->name);
> > + goto fail;
> > + }
> > +
> > + hmapx_add(&trk_data->ls_with_changed_acls,
> > + CONST_CAST(struct ovn_datapath *, od));
> > + }
> > + }
> > +
> > + if (!hmapx_is_empty(&trk_data->ls_with_changed_acls)) {
> > + trk_data->type |= NORTHD_TRACKED_LS_ACLS;
> > + }
> > +
> > + return true;
> > +
> > +fail:
> > + destroy_northd_data_tracked_changes(nd);
> > + return false;
> > +}
> > +
> > /* Returns true if the logical router has changes which can be
> > * incrementally handled.
> > * Presently supports i-p for the below changes:
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 8d9ba505c..2b927183c 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -38,6 +38,7 @@ struct northd_input {
> > *nbrec_chassis_template_var_table;
> > const struct nbrec_mirror_table *nbrec_mirror_table;
> > const struct nbrec_mirror_rule_table *nbrec_mirror_rule_table;
> > + const struct nbrec_port_group_table *nbrec_port_gorup_table;
> >
> > /* Southbound table references */
> > const struct sbrec_datapath_binding_table
> *sbrec_datapath_binding_table;
> > @@ -836,6 +837,8 @@ bool northd_handle_ls_changes(struct ovsdb_idl_txn *,
> > struct northd_data *);
> > bool northd_handle_lr_changes(const struct northd_input *,
> > struct northd_data *);
> > +bool northd_handle_pgs_acl_changes(const struct northd_input *ni,
> > + struct northd_data *nd);
> > 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 28b568663..945727a3f 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -10095,7 +10095,7 @@ dnl The northd node should not recompute.
> > AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats
> northd], [0], [dnl
> > Node: northd
> > - recompute: 0
> > -- compute: 0
> > +- compute: 1
> > - cancel: 0
> > ])
> > dnl The port_group node recomputes every time a NB port group is
> added/deleted.
> > @@ -10132,7 +10132,7 @@ dnl The northd node should not recompute.
> > AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats
> northd], [0], [dnl
> > Node: northd
> > - recompute: 0
> > -- compute: 0
> > +- compute: 1
> > - cancel: 0
> > ])
> > dnl The port_group node recomputes also every time a port from a new
> switch
> > @@ -10168,7 +10168,7 @@ dnl The northd node should not recompute.
> > AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats
> northd], [0], [dnl
> > Node: northd
> > - recompute: 0
> > -- compute: 0
> > +- compute: 1
> > - cancel: 0
> > ])
> > dnl The port_group node recomputes also every time a port from a new
> switch
> > @@ -10205,7 +10205,7 @@ dnl The northd node should not recompute.
> > AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats
> northd], [0], [dnl
> > Node: northd
> > - recompute: 0
> > -- compute: 0
> > +- compute: 1
> > - cancel: 0
> > ])
> > dnl We did not change the set of switches a pg is applied to, there
> should be
> > @@ -10243,7 +10243,7 @@ dnl though, therefore "compute: 1".
> > AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats
> northd], [0], [dnl
> > Node: northd
> > - recompute: 0
> > -- compute: 0
> > +- compute: 1
> > - cancel: 0
> > ])
> > dnl We did not change the set of switches a pg is applied to, there
> should be
> > @@ -10279,7 +10279,7 @@ dnl The northd node should not recompute.
> > AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats
> northd], [0], [dnl
> > Node: northd
> > - recompute: 0
> > -- compute: 0
> > +- compute: 1
> > - cancel: 0
> > ])
> > dnl We changed the set of switches a pg is applied to, there should be
> > @@ -10316,7 +10316,7 @@ dnl The northd node should not recompute.
> > AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats
> northd], [0], [dnl
> > Node: northd
> > - recompute: 0
> > -- compute: 0
> > +- compute: 1
> > - cancel: 0
> > ])
> > dnl We changed the set of switches a pg is applied to, there should be
> > @@ -10353,7 +10353,7 @@ dnl The northd node should not recompute.
> > AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats
> northd], [0], [dnl
> > Node: northd
> > - recompute: 0
> > -- compute: 0
> > +- compute: 1
> > - cancel: 0
> > ])
> > dnl We changed the set of switches a pg is applied to, there should be
> a
> > @@ -10392,7 +10392,7 @@ dnl The northd node should not recompute,.
> > AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats
> northd], [0], [dnl
> > Node: northd
> > - recompute: 0
> > -- compute: 0
> > +- compute: 1
> > - cancel: 0
> > ])
> > dnl We changed the set of switches a pg is applied to, there should be
> a
> > @@ -17521,6 +17521,9 @@ AT_SETUP([ACL incremental processing])
> > ovn_start
> >
> > check ovn-nbctl ls-add ls
> > +check ovn-nbctl lsp-add ls lsp0
> > +check ovn-nbctl pg-add pg lsp0
> > +
> > check ovn-nbctl meter-add meter1 drop 10 kbps
> > check ovn-nbctl meter-add meter2 drop 20 kbps
> >
> > @@ -17589,5 +17592,65 @@ check_engine_stats sync_meters recompute
> nocompute
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >
> > +AS_BOX([ACLs attached to LS])
> > +check ovn-nbctl --wait=sb acl-add ls from-lport 100 tcp drop
> > +acl_id=$(fetch_column nb:Acl _uuid match=tcp action=drop)
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_meters norecompute compute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +
> > +check ovn-nbctl --wait=sb acl-add ls from-lport 101 ip4 drop
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_meters norecompute compute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +
> > +check ovn-nbctl --wait=sb set acl $acl_id match=udp
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_meters norecompute compute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +
> > +check ovn-nbctl --wait=sb acl-del ls
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_meters norecompute compute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +
> > +AS_BOX([ACLs attached to PG])
> > +check ovn-nbctl --wait=sb acl-add pg from-lport 100 tcp drop
> > +acl_id=$(fetch_column nb:Acl _uuid match=tcp action=drop)
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_meters norecompute compute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +
> > +check ovn-nbctl --wait=sb acl-add pg from-lport 101 ip4 drop
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_meters norecompute compute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +
> > +check ovn-nbctl --wait=sb set acl $acl_id match=udp
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_meters norecompute compute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +
> > +check ovn-nbctl --wait=sb acl-del pg
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +check_engine_stats sync_meters norecompute compute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> > +
> > AT_CLEANUP
> > ])
>
>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
