| Message ID | 20221018155936.1394396-2-amorenoz@redhat.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | ACL Sampling using per-flow IPFIX | expand |
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 <amorenoz@redhat.com> > --- > 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? 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. An external_ids column might also make sense. [0] https://github.com/openvswitch/ovs/commit/04e5adfedd2a2e4ceac62136671057542a7cf875 > + }, Nit: By default "isRoot" is false. Don't we want this as root table? > "Copp": { > "columns": { > "name": {"type": "string"}, > @@ -267,6 +284,10 @@ > "label": {"type": {"key": {"type": "integer", > "minInteger": 0, > "maxInteger": 4294967295}}}, > + "sample": {"type": {"key": {"type": "uuid", > + "refTable": "Sample", > + "refType": "strong"}, > + "min": 0, "max": 1}}, > "options": { > "type": {"key": "string", > "value": "string", > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 9581aef7c..e818fd8d1 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -387,6 +387,31 @@ > > </table> > > + <table name="Sample" title="Sample"> > + <p> > + This table describes an IPFIX Sampling Point. Entries in other tables > + might be associated with Sample entries to indicate how the sample > + should be generated. > + > + For an example, see <ref table="ACL"/>. > + </p> > + <column name="probability"> > + Sampling probability. It must be an integer number between 0 and 65535. > + </column> > + <column name="collector_set_id"> > + The 32-bit integer identifier of the set of of collectors to send > + packets to. See Flow_Sample_Collector_Set Table in ovs-vswitchd's > + database schema. > + </column> > + <column name="obs_domain_id"> > + The 8 most significant bits of the Observation Domain ID that will be > + added to evvery IPFIX sample. The 24 LSB will be the datapath key. > + </column> > + <column name="obs_point_id"> > + If set, it'll be use as Observation Point ID in every IPFIX sample. > + Otherwise the Logical Flow's coockie will be used. > + </column> > + </table> > <table name="Copp" title="Control plane protection"> > <p> > This table is used to define control plane protection policies, i.e., > @@ -2191,6 +2216,12 @@ > </column> > </group> > > + <column name="sample"> > + <p> > + The entry in the <ref table="Sample"/> table to use for sampling. > + </p> > + </column> > + > <group title="Common Columns"> > <column name="options"> > This column provides general key/value settings. The supported
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 <amorenoz@redhat.com> >> --- >> 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. > > 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"? 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 >> "Copp": { >> "columns": { >> "name": {"type": "string"}, >> @@ -267,6 +284,10 @@ >> "label": {"type": {"key": {"type": "integer", >> "minInteger": 0, >> "maxInteger": 4294967295}}}, >> + "sample": {"type": {"key": {"type": "uuid", >> + "refTable": "Sample", >> + "refType": "strong"}, >> + "min": 0, "max": 1}}, >> "options": { >> "type": {"key": "string", >> "value": "string", >> diff --git a/ovn-nb.xml b/ovn-nb.xml >> index 9581aef7c..e818fd8d1 100644 >> --- a/ovn-nb.xml >> +++ b/ovn-nb.xml >> @@ -387,6 +387,31 @@ >> >> </table> >> >> + <table name="Sample" title="Sample"> >> + <p> >> + This table describes an IPFIX Sampling Point. Entries in other tables >> + might be associated with Sample entries to indicate how the sample >> + should be generated. >> + >> + For an example, see <ref table="ACL"/>. >> + </p> >> + <column name="probability"> >> + Sampling probability. It must be an integer number between 0 and 65535. >> + </column> >> + <column name="collector_set_id"> >> + The 32-bit integer identifier of the set of of collectors to send >> + packets to. See Flow_Sample_Collector_Set Table in ovs-vswitchd's >> + database schema. >> + </column> >> + <column name="obs_domain_id"> >> + The 8 most significant bits of the Observation Domain ID that will be >> + added to evvery IPFIX sample. The 24 LSB will be the datapath key. >> + </column> >> + <column name="obs_point_id"> >> + If set, it'll be use as Observation Point ID in every IPFIX sample. >> + Otherwise the Logical Flow's coockie will be used. >> + </column> >> + </table> >> <table name="Copp" title="Control plane protection"> >> <p> >> This table is used to define control plane protection policies, i.e., >> @@ -2191,6 +2216,12 @@ >> </column> >> </group> >> >> + <column name="sample"> >> + <p> >> + The entry in the <ref table="Sample"/> table to use for sampling. >> + </p> >> + </column> >> + >> <group title="Common Columns"> >> <column name="options"> >> This column provides general key/value settings. The supported >
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 <amorenoz@redhat.com> >>> --- >>> 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.
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}} + } + }, "Copp": { "columns": { "name": {"type": "string"}, @@ -267,6 +284,10 @@ "label": {"type": {"key": {"type": "integer", "minInteger": 0, "maxInteger": 4294967295}}}, + "sample": {"type": {"key": {"type": "uuid", + "refTable": "Sample", + "refType": "strong"}, + "min": 0, "max": 1}}, "options": { "type": {"key": "string", "value": "string", diff --git a/ovn-nb.xml b/ovn-nb.xml index 9581aef7c..e818fd8d1 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -387,6 +387,31 @@ </table> + <table name="Sample" title="Sample"> + <p> + This table describes an IPFIX Sampling Point. Entries in other tables + might be associated with Sample entries to indicate how the sample + should be generated. + + For an example, see <ref table="ACL"/>. + </p> + <column name="probability"> + Sampling probability. It must be an integer number between 0 and 65535. + </column> + <column name="collector_set_id"> + The 32-bit integer identifier of the set of of collectors to send + packets to. See Flow_Sample_Collector_Set Table in ovs-vswitchd's + database schema. + </column> + <column name="obs_domain_id"> + The 8 most significant bits of the Observation Domain ID that will be + added to evvery IPFIX sample. The 24 LSB will be the datapath key. + </column> + <column name="obs_point_id"> + If set, it'll be use as Observation Point ID in every IPFIX sample. + Otherwise the Logical Flow's coockie will be used. + </column> + </table> <table name="Copp" title="Control plane protection"> <p> This table is used to define control plane protection policies, i.e., @@ -2191,6 +2216,12 @@ </column> </group> + <column name="sample"> + <p> + The entry in the <ref table="Sample"/> table to use for sampling. + </p> + </column> + <group title="Common Columns"> <column name="options"> This column provides general key/value settings. The supported
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 <amorenoz@redhat.com> --- northd/northd.c | 31 ++++++++++++++++++++++++++++++- ovn-nb.ovsschema | 23 ++++++++++++++++++++++- ovn-nb.xml | 31 +++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-)