On Tue, Jan 14, 2025 at 7:30 PM Mark Michelson <[email protected]> wrote:
> On 1/14/25 03:41, Ales Musil wrote: > > > > > > On Mon, Dec 16, 2024 at 8:03 PM Mark Michelson <[email protected] > > <mailto:[email protected]>> wrote: > > > > This introduces a new southbound table called ACL_ID for storing > > persistent ACL conntrack IDs. These IDs are generated by ovn-northd > for > > any ACL that is of type "allow-established". > > > > Signed-off-by: Mark Michelson <[email protected] > > <mailto:[email protected]>> > > --- > > > > > > Hi Mark, > > > > I have a few small comments down below. > > > > v2 -> v3: > > * Since there are now two factors that determine if an ACL should > be > > synced to the SB DB, there is a helper function in en-acl-ids.c > that > > can be used to tell if an ACL is a candidate for having an ID > > synced. > > > > v1 -> v2: > > * Formatting issues are fixed. > > * UUIDs of NB ACLs and SB ACL_IDs are the same. > > * There is no separate sync function. The incremental processing > > run function syncs the NB and SB data. > > * The engine node no longer needs to allocate data. > > > > Feedback not addressed in this version: > > * The bitmap_scan() code is unchanged. This is because a change > > in the next patch addresses the issue that Ales pointed out. > > * I did not add a comment around the ovsdb_idl_omit_alert() call > > because the comment at the top of the section already says > > what was suggested. > > --- > > northd/automake.mk <http://automake.mk> | 2 + > > northd/en-acl-ids.c | 104 > +++++++++++++++++++++++++++++++++++++++ > > northd/en-acl-ids.h | 13 +++++ > > northd/inc-proc-northd.c | 14 +++++- > > northd/northd.h | 4 ++ > > northd/ovn-northd.c | 4 ++ > > ovn-sb.ovsschema | 12 +++-- > > ovn-sb.xml | 13 +++++ > > tests/ovn.at <http://ovn.at> | 40 +++++++++++++++ > > 9 files changed, 202 insertions(+), 4 deletions(-) > > create mode 100644 northd/en-acl-ids.c > > create mode 100644 northd/en-acl-ids.h > > > > diff --git a/northd/automake.mk <http://automake.mk> > > b/northd/automake.mk <http://automake.mk> > > index 6566ad299..d46dfd763 100644 > > --- a/northd/automake.mk <http://automake.mk> > > +++ b/northd/automake.mk <http://automake.mk> > > @@ -34,6 +34,8 @@ northd_ovn_northd_SOURCES = \ > > northd/en-ls-stateful.h \ > > northd/en-sampling-app.c \ > > northd/en-sampling-app.h \ > > + northd/en-acl-ids.c \ > > + northd/en-acl-ids.h \ > > northd/inc-proc-northd.c \ > > northd/inc-proc-northd.h \ > > northd/ipam.c \ > > diff --git a/northd/en-acl-ids.c b/northd/en-acl-ids.c > > new file mode 100644 > > index 000000000..80c8a80cc > > --- /dev/null > > +++ b/northd/en-acl-ids.c > > @@ -0,0 +1,104 @@ > > +/* > > + * Licensed under the Apache License, Version 2.0 (the "License"); > > + * you may not use this file except in compliance with the License. > > + * You may obtain a copy of the License at: > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > <http://www.apache.org/licenses/LICENSE-2.0> > > + * > > + * Unless required by applicable law or agreed to in writing, > software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > > implied. > > + * See the License for the specific language governing permissions > and > > + * limitations under the License. > > + */ > > + > > +#include <config.h> > > + > > +#include "en-acl-ids.h" > > +#include "lib/uuidset.h" > > +#include "lib/ovn-sb-idl.h" > > +#include "lib/ovn-nb-idl.h" > > +#include "lib/bitmap.h" > > + > > +#include "openvswitch/vlog.h" > > + > > +VLOG_DEFINE_THIS_MODULE(northd_acl_ids); > > + > > +#define MAX_ACL_ID 65535 > > + > > +void * > > +en_acl_id_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED) > > +{ > > + return NULL; > > +} > > + > > +static bool > > +should_sync_to_sb(const struct nbrec_acl *nb_acl) > > +{ > > + return !strcmp(nb_acl->action, "allow-related") && > > + smap_get_bool(&nb_acl->options, > > + "bypass_match_for_established", > > + false); > > +} > > + > > +void > > +en_acl_id_run(struct engine_node *node, void *data OVS_UNUSED) > > +{ > > + const struct engine_context *eng_ctx = engine_get_context(); > > + if (!eng_ctx->ovnnb_idl_txn || !eng_ctx->ovnsb_idl_txn) { > > + return; > > + } > > > > > > I'm pretty sure that the engine run can only happen if both idls are > > capable of writing. > > > > + > > + const struct nbrec_acl_table *nb_acl_table = > > + EN_OVSDB_GET(engine_get_input("NB_acl", node)); > > + const struct sbrec_acl_id_table *sb_acl_id_table = > > + EN_OVSDB_GET(engine_get_input("SB_acl_id", node)); > > + struct uuidset visited = UUIDSET_INITIALIZER(&visited); > > + unsigned long *id_bitmap = bitmap_allocate(MAX_ACL_ID); > > + > > + const struct nbrec_acl *nb_acl; > > + const struct sbrec_acl_id *sb_id; > > + SBREC_ACL_ID_TABLE_FOR_EACH_SAFE (sb_id, sb_acl_id_table) { > > + nb_acl = nbrec_acl_table_get_for_uuid(nb_acl_table, > > + &sb_id->header_.uuid); > > + if (nb_acl && should_sync_to_sb(nb_acl)) { > > + bitmap_set1(id_bitmap, sb_id->id); > > + uuidset_insert(&visited, &sb_id->header_.uuid); > > + } else { > > + sbrec_acl_id_delete(sb_id); > > + } > > + } > > + > > + size_t scan_start = 1; > > + size_t scan_end = MAX_ACL_ID; > > + NBREC_ACL_TABLE_FOR_EACH (nb_acl, nb_acl_table) { > > + if (uuidset_find_and_delete(&visited, > &nb_acl->header_.uuid)) { > > + continue; > > + } > > + if (!should_sync_to_sb(nb_acl)) { > > + continue; > > + } > > + int64_t new_id = bitmap_scan(id_bitmap, 0, > > + scan_start, scan_end + 1); > > + if (new_id == scan_end + 1) { > > + static struct vlog_rate_limit rl = > > VLOG_RATE_LIMIT_INIT(1, 1); > > + VLOG_WARN_RL(&rl, "Exhausted all ACL IDs"); > > + break; > > + } > > + sb_id = > > sbrec_acl_id_insert_persist_uuid(eng_ctx->ovnsb_idl_txn, > > + > > &nb_acl->header_.uuid); > > + sbrec_acl_id_set_id(sb_id, new_id); > > + bitmap_set1(id_bitmap, new_id); > > + scan_start = new_id + 1; > > + } > > + > > + engine_set_node_state(node, EN_UPDATED); > > + uuidset_destroy(&visited); > > + bitmap_free(id_bitmap); > > +} > > + > > +void > > +en_acl_id_cleanup(void *data OVS_UNUSED) > > +{ > > +} > > diff --git a/northd/en-acl-ids.h b/northd/en-acl-ids.h > > new file mode 100644 > > index 000000000..f878f55d9 > > --- /dev/null > > +++ b/northd/en-acl-ids.h > > @@ -0,0 +1,13 @@ > > +#ifndef EN_ACL_IDS_H > > +#define EN_ACL_IDS_H > > + > > +#include <config.h> > > +#include <stdbool.h> > > + > > +#include "lib/inc-proc-eng.h" > > + > > +bool northd_acl_id_handler(struct engine_node *node, void *data); > > +void *en_acl_id_init(struct engine_node *, struct engine_arg *); > > +void en_acl_id_run(struct engine_node *, void *data); > > +void en_acl_id_cleanup(void *data); > > +#endif > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index 6e0aa04c4..8025f1c28 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -41,6 +41,7 @@ > > #include "en-sampling-app.h" > > #include "en-sync-sb.h" > > #include "en-sync-from-sb.h" > > +#include "en-acl-ids.h" > > #include "unixctl.h" > > #include "util.h" > > > > @@ -102,7 +103,8 @@ static unixctl_cb_func chassis_features_list; > > SB_NODE(fdb, "fdb") \ > > SB_NODE(static_mac_binding, "static_mac_binding") \ > > SB_NODE(chassis_template_var, "chassis_template_var") \ > > - SB_NODE(logical_dp_group, "logical_dp_group") > > + SB_NODE(logical_dp_group, "logical_dp_group") \ > > + SB_NODE(acl_id, "acl_id") > > > > enum sb_engine_node { > > #define SB_NODE(NAME, NAME_STR) SB_##NAME, > > @@ -161,6 +163,7 @@ static ENGINE_NODE(route_policies, > > "route_policies"); > > static ENGINE_NODE(routes, "routes"); > > static ENGINE_NODE(bfd, "bfd"); > > static ENGINE_NODE(bfd_sync, "bfd_sync"); > > +static ENGINE_NODE(acl_id, "acl_id"); > > > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > struct ovsdb_idl_loop *sb) > > @@ -186,6 +189,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop > *nb, > > global_config_sb_chassis_handler); > > engine_add_input(&en_global_config, &en_sampling_app, NULL); > > > > + engine_add_input(&en_acl_id, &en_nb_acl, NULL); > > + engine_add_input(&en_acl_id, &en_sb_acl_id, NULL); > > + > > engine_add_input(&en_northd, &en_nb_mirror, NULL); > > engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL); > > engine_add_input(&en_northd, &en_nb_chassis_template_var, > NULL); > > @@ -344,6 +350,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop > *nb, > > northd_output_mac_binding_aging_handler); > > engine_add_input(&en_northd_output, &en_fdb_aging, > > northd_output_fdb_aging_handler); > > + engine_add_input(&en_northd_output, &en_acl_id, NULL); > > > > This should follow the convention and add a handler that just returns > true. > > Just so I understand, is this convention specifically used for direct > inputs to the en_northd_output node? Is this to prevent unnecessary > recomputes since en_northd_output is essentially a "dummy" node that > everything eventually feeds into? > Yes to both questions. > If so, then it also makes sense why you suggested a similar change in > patch 4 for the new input to en_controller_output. > > > > > > > struct engine_arg engine_arg = { > > .nb_idl = nb->idl, > > @@ -364,6 +371,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop > *nb, > > = mac_binding_by_datapath_index_create(sb->idl); > > struct ovsdb_idl_index *fdb_by_dp_key = > > ovsdb_idl_index_create1(sb->idl, &sbrec_fdb_col_dp_key); > > + struct ovsdb_idl_index *sbrec_acl_id_by_id = > > + ovsdb_idl_index_create1(sb->idl, &sbrec_acl_id_col_id); > > > > > > I don't see this index being used anywhere. > > > > > > engine_init(&en_northd_output, &engine_arg); > > > > @@ -388,6 +397,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop > *nb, > > engine_ovsdb_node_add_index(&en_sb_fdb, > > "fdb_by_dp_key", > > fdb_by_dp_key); > > + engine_ovsdb_node_add_index(&en_sb_acl_id, > > + "sbrec_acl_id_by_id", > > + sbrec_acl_id_by_id); > > > > struct ovsdb_idl_index *sbrec_address_set_by_name > > = ovsdb_idl_index_create1(sb->idl, > > &sbrec_address_set_col_name); > > diff --git a/northd/northd.h b/northd/northd.h > > index d60c944df..bccb1c5d8 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -63,12 +63,16 @@ struct northd_input { > > struct eth_addr svc_monitor_mac_ea; > > const struct chassis_features *features; > > > > + /* ACL ID inputs. */ > > + const struct acl_id_data *acl_id_data; > > + > > /* Indexes */ > > struct ovsdb_idl_index *sbrec_chassis_by_name; > > struct ovsdb_idl_index *sbrec_chassis_by_hostname; > > struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name; > > struct ovsdb_idl_index *sbrec_ip_mcast_by_dp; > > struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port; > > + struct ovsdb_idl_index *sbrec_acl_id_by_id; > > }; > > > > /* A collection of datapaths. E.g. all logical switch datapaths, > > or all > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index fb29c3c21..bbb321248 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -896,6 +896,10 @@ main(int argc, char *argv[]) > > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > > &sbrec_logical_dp_group_columns[i]); > > } > > + for (size_t i = 0; i < SBREC_ACL_ID_N_COLUMNS; i++) { > > + ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > > + &sbrec_acl_id_columns[i]); > > + } > > > > unixctl_command_register("sb-connection-status", "", 0, 0, > > ovn_conn_show, ovnsb_idl_loop.idl); > > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > > index 73abf2c8d..ef7d28d82 100644 > > --- a/ovn-sb.ovsschema > > +++ b/ovn-sb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Southbound", > > - "version": "20.37.0", > > - "cksum": "1950136776 31493", > > + "version": "20.38.0", > > + "cksum": "3668645444 31756", > > "tables": { > > "SB_Global": { > > "columns": { > > @@ -617,6 +617,12 @@ > > "type": {"key": "string", "value": "string", > > "min": 0, "max": "unlimited"}}}, > > "indexes": [["chassis"]], > > - "isRoot": true} > > + "isRoot": true}, > > + "ACL_ID": { > > + "columns": { > > + "id": {"type": {"key": {"type": "integer", > > + "minInteger": 0, > > + "maxInteger": > 32767}}}}, > > + "isRoot": true} > > } > > } > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > index ea4adc1c3..b4759f72c 100644 > > --- a/ovn-sb.xml > > +++ b/ovn-sb.xml > > @@ -5217,4 +5217,17 @@ tcp.flags = RST; > > The set of variable values for a given chassis. > > </column> > > </table> > > + > > + <table name="ACL_ID"> > > + <p> > > + Each record represents an identifier that > > <code>ovn-northd</code> needs > > + to synchronize with instances of <code>ovn-controller</code>. > > The UUID > > + of each record corresponds directly with an <ref > > table="ACL"/> record > > + in the northbound database. > > + </p> > > + <column name="id"> > > + An identifier corresponding to a northbound > > + <code>allow-established</code> ACL. > > + </column> > > + </table> > > </database> > > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at < > http://ovn.at> > > index 2fdf1a88c..41524a2fc 100644 > > --- a/tests/ovn.at <http://ovn.at> > > +++ b/tests/ovn.at <http://ovn.at> > > @@ -39761,3 +39761,43 @@ OVN_CLEANUP([hv1]) > > > > AT_CLEANUP > > ]) > > + > > +AT_SETUP([ACL Conntrack ID propagation]) > > +ovn_start > > + > > +dnl In this test, we want to ensure that southbound ACL_ID > > +dnl entries are created for northbound ACLs of type "allow-related" > > +dnl when the "bypass_match_for_established" option is set to true. > > +dnl > > +dnl If an ACL is of a different type, does not have the option set > > +dnl to true, or is deleted, then there should be no soutbhound > ACL_ID. > > + > > +check ovn-nbctl ls-add sw > > +check ovn-nbctl --wait=sb acl-add sw from-lport 1000 1 allow-related > > +acl_uuid=$(fetch_column nb:ACL _uuid priority=1000) > > + > > +dnl The ACL is not have the bypass option set, so SBDB should have > > no rows. > > +wait_row_count ACL_ID 0 > > + > > +dnl When we set the option, the SBDB should pick it up. > > +check ovn-nbctl --wait=sb set ACL $acl_uuid > > options:bypass_match_for_established=true > > + > > +wait_row_count ACL_ID 1 > > + > > +dnl Setting to a new action should remove the row from the SBDB. > > +check ovn-nbctl --wait=sb set ACL $acl_uuid action=drop > > + > > +wait_row_count ACL_ID 0 > > + > > +dnl Set back to allow-related and make sure it shows up in the SBDB. > > +check ovn-nbctl --wait=sb set ACL $acl_uuid action=allow-related > > + > > +wait_row_count ACL_ID 1 > > + > > +dnl Delete the ACL and ensure the SBDB entry is deleted. > > +check ovn-nbctl --wait=sb acl-del sw from-lport 1000 1 > > + > > +wait_row_count ACL_ID 0 > > + > > +AT_CLEANUP > > +]) > > -- > > 2.45.2 > > > > _______________________________________________ > > dev mailing list > > [email protected] <mailto:[email protected]> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > > > > Thanks, > > Ales > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
