On 4/5/23 09:20, Adrian Moreno wrote: > Thanks for the comments Dumitru! > > On 3/30/23 12:02, Dumitru Ceara wrote: >> On 10/18/22 17:59, Adrian Moreno wrote: >>> Introduce a new table called Sample where per-flow IPFIX configuration >>> can be specified. >>> Also, reference rows from such table from the ACL table to enable the >>> configuration of ACL sampling. If enabled, northd will add a sample >>> action to each ACL-related logical flow. >>> >>> Signed-off-by: Adrian Moreno <[email protected]> >>> --- >>> northd/northd.c | 31 ++++++++++++++++++++++++++++++- >>> ovn-nb.ovsschema | 23 ++++++++++++++++++++++- >>> ovn-nb.xml | 31 +++++++++++++++++++++++++++++++ >>> 3 files changed, 83 insertions(+), 2 deletions(-) >>> >>> diff --git a/northd/northd.c b/northd/northd.c >>> index 61d474840..3e09e3a0f 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -6194,6 +6194,27 @@ build_acl_log(struct ds *actions, const struct >>> nbrec_acl *acl, >>> ds_put_cstr(actions, "); "); >>> } >>> +static void >>> +build_acl_sample(struct ds *actions, const struct nbrec_acl *acl) >>> +{ >>> + if (!acl->sample) { >>> + return; >>> + } >>> + ds_put_format(actions, "sample(probability=%"PRId16"," >>> + "collector_set=%d," >>> + "obs_domain=%hd,", >>> + (uint16_t) acl->sample->probability, >>> + (uint32_t) acl->sample->collector_set_id, >>> + (uint8_t) acl->sample->obs_domain_id); >>> + >>> + if (acl->sample->obs_point_id) { >>> + ds_put_format(actions, "obs_point=%"PRId32");", >>> + (uint32_t) *acl->sample->obs_point_id); >>> + } else { >>> + ds_put_cstr(actions, "obs_point=$cookie);"); >>> + } >>> +} >>> + >>> static void >>> build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, >>> enum ovn_stage stage, struct nbrec_acl *acl, >>> @@ -6260,6 +6281,7 @@ consider_acl(struct hmap *lflows, struct >>> ovn_datapath *od, >>> if (!strcmp(acl->action, "allow-stateless")) { >>> ds_clear(actions); >>> build_acl_log(actions, acl, meter_groups); >>> + build_acl_sample(actions, acl); >>> ds_put_cstr(actions, "next;"); >>> ovn_lflow_add_with_hint(lflows, od, stage, >>> acl->priority + OVN_ACL_PRI_OFFSET, >>> @@ -6275,6 +6297,7 @@ consider_acl(struct hmap *lflows, struct >>> ovn_datapath *od, >>> if (!has_stateful) { >>> ds_clear(actions); >>> build_acl_log(actions, acl, meter_groups); >>> + build_acl_sample(actions, acl); >>> ds_put_cstr(actions, "next;"); >>> ovn_lflow_add_with_hint(lflows, od, stage, >>> acl->priority + >>> OVN_ACL_PRI_OFFSET, >>> @@ -6304,6 +6327,7 @@ consider_acl(struct hmap *lflows, struct >>> ovn_datapath *od, >>> REG_LABEL" = %"PRId64"; ", acl->label); >>> } >>> build_acl_log(actions, acl, meter_groups); >>> + build_acl_sample(actions, acl); >>> ds_put_cstr(actions, "next;"); >>> ovn_lflow_add_with_hint(lflows, od, stage, >>> acl->priority + >>> OVN_ACL_PRI_OFFSET, >>> @@ -6329,6 +6353,7 @@ consider_acl(struct hmap *lflows, struct >>> ovn_datapath *od, >>> REG_LABEL" = %"PRId64"; ", acl->label); >>> } >>> build_acl_log(actions, acl, meter_groups); >>> + build_acl_sample(actions, acl); >>> ds_put_cstr(actions, "next;"); >>> ovn_lflow_add_with_hint(lflows, od, stage, >>> acl->priority + >>> OVN_ACL_PRI_OFFSET, >>> @@ -6349,7 +6374,7 @@ consider_acl(struct hmap *lflows, struct >>> ovn_datapath *od, >>> */ >>> bool log_related = smap_get_bool(&acl->options, >>> "log-related", >>> false); >>> - if (acl->log && acl->label && log_related) { >>> + if ((acl->log || acl->sample) && acl->label && >>> log_related) { >>> /* Related/reply flows need to be set on the >>> opposite pipeline >>> * from where the ACL itself is set. >>> */ >>> @@ -6365,6 +6390,7 @@ consider_acl(struct hmap *lflows, struct >>> ovn_datapath *od, >>> use_ct_inv_match ? " && !ct.inv" : "", >>> ct_blocked_match, acl->label); >>> build_acl_log(actions, acl, meter_groups); >>> + build_acl_sample(actions, acl); >>> ds_put_cstr(actions, "next;"); >>> ovn_lflow_add_with_hint(lflows, od, log_related_stage, >>> UINT16_MAX - 2, >>> @@ -6402,6 +6428,7 @@ consider_acl(struct hmap *lflows, struct >>> ovn_datapath *od, >>> } else { >>> ds_put_format(match, " && (%s)", acl->match); >>> build_acl_log(actions, acl, meter_groups); >>> + build_acl_sample(actions, acl); >>> ds_put_cstr(actions, debug_implicit_drop_action()); >>> ovn_lflow_add_with_hint(lflows, od, stage, >>> acl->priority + >>> OVN_ACL_PRI_OFFSET, >>> @@ -6430,6 +6457,7 @@ consider_acl(struct hmap *lflows, struct >>> ovn_datapath *od, >>> } else { >>> ds_put_format(match, " && (%s)", acl->match); >>> build_acl_log(actions, acl, meter_groups); >>> + build_acl_sample(actions, acl); >>> ds_put_cstr(actions, debug_implicit_drop_action()); >>> ovn_lflow_add_with_hint(lflows, od, stage, >>> acl->priority + >>> OVN_ACL_PRI_OFFSET, >>> @@ -6447,6 +6475,7 @@ consider_acl(struct hmap *lflows, struct >>> ovn_datapath *od, >>> actions, &acl->header_, >>> meter_groups); >>> } else { >>> build_acl_log(actions, acl, meter_groups); >>> + build_acl_sample(actions, acl); >>> ds_put_cstr(actions, debug_implicit_drop_action()); >>> ovn_lflow_add_with_hint(lflows, od, stage, >>> acl->priority + >>> OVN_ACL_PRI_OFFSET, >>> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema >>> index 174364c8b..6178b532e 100644 >>> --- a/ovn-nb.ovsschema >>> +++ b/ovn-nb.ovsschema >>> @@ -1,7 +1,7 @@ >>> { >>> "name": "OVN_Northbound", >>> "version": "6.3.0", >>> - "cksum": "4042813038 31869", >>> + "cksum": "3795038812 33116", >>> "tables": { >>> "NB_Global": { >>> "columns": { >>> @@ -30,6 +30,23 @@ >>> "ipsec": {"type": "boolean"}}, >>> "maxRows": 1, >>> "isRoot": true}, >>> + "Sample": { >>> + "columns": { >>> + "probability": {"type": {"key": {"type": "integer", >>> + "minInteger": 0, >>> + "maxInteger": >>> 65535}}}, >>> + "collector_set_id": {"type": {"key": {"type": >>> "integer", >>> + "minInteger": 0, >>> + "maxInteger": >>> 4294967295}}}, >>> + "obs_domain_id": {"type": {"key": {"type": "integer", >>> + "minInteger": 0, >>> + "maxInteger": >>> 255}}}, >>> + "obs_point_id": {"type": {"key": {"type": "integer", >>> + "minInteger": 0, >>> + "maxInteger": >>> 4294967295}, >>> + "min": 0, "max":1}} >>> + } >> >> This seems to match 1:1 the arguments of the OVS SAMPLE action. That's >> fine but do we expect more arguments to be added in the future? > > I can't think of any right now. >
Ok. >> >> Please add a column that can be used as index and also define a unique >> index. That will save us from trouble like "spurious duplicates" [0] >> when the DB is in RAFT. It also makes it easier for the CMS to track >> what it installed or not. > > Technically, the only unique element would be (collector_set_id, > obs_domain_id, obs_point_id) tuple. I can add that as index. Would that > work or are you referring to a column called "id"? > If the tuple (collector_set_id, obs_domain_id, obs_point_id) is expected to be unique then let's add it as index. Otherwise, yes, please add an "id" or "name" column and mark that as index. > An external_ids column might also make sense. >> > > Yes it does. > > >> [0] >> https://github.com/openvswitch/ovs/commit/04e5adfedd2a2e4ceac62136671057542a7cf875 >> >>> + }, >> >> Nit: By default "isRoot" is false. Don't we want this as root table? >> > > Hmm... I don't know. Maybe we could delete > "isRoot: false" means the records are garbage collected when not referred to anymore. ACLs, for example, are like that. It's probably OK to leave this as non-root. I just wanted to make sure we considered both options. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
