On Wed, Jun 26, 2024 at 02:11:00PM GMT, Eelco Chaudron wrote:
> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>
> > Add as new column in the Flow_Sample_Collector_Set table named
> > "local_sample_group" which enables this feature.
> >
> > Signed-off-by: Adrian Moreno <amore...@redhat.com>
> > ---
> > NEWS | 4 ++
> > vswitchd/bridge.c | 78 +++++++++++++++++++++++++++++++++++---
> > vswitchd/vswitch.ovsschema | 9 ++++-
> > vswitchd/vswitch.xml | 39 +++++++++++++++++--
> > 4 files changed, 119 insertions(+), 11 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index b92cec532..1c05a7120 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -7,6 +7,10 @@ Post-v3.3.0
> > - The primary development branch has been renamed from 'master' to
> > 'main'.
> > The OVS tree remains hosted on GitHub.
> > https://github.com/openvswitch/ovs.git
> > + - Local sampling is introduced. It reuses the OpenFlow sample action and
> > + allows samples to be emitted locally (instead of via IPFIX) in a
> > + datapath-specific manner via the new datapath action called
> > "emit_sample".
> > + Linux kernel datapath is the first to support this feature.
> >
> >
> > v3.3.0 - 16 Feb 2024
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 95a65fcdc..cd7dc6646 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -288,6 +288,7 @@ static void bridge_configure_mac_table(struct bridge *);
> > static void bridge_configure_mcast_snooping(struct bridge *);
> > static void bridge_configure_sflow(struct bridge *, int
> > *sflow_bridge_number);
> > static void bridge_configure_ipfix(struct bridge *);
> > +static void bridge_configure_lsample(struct bridge *);
> > static void bridge_configure_spanning_tree(struct bridge *);
> > static void bridge_configure_tables(struct bridge *);
> > static void bridge_configure_dp_desc(struct bridge *);
> > @@ -989,6 +990,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> > *ovs_cfg)
> > bridge_configure_netflow(br);
> > bridge_configure_sflow(br, &sflow_bridge_number);
> > bridge_configure_ipfix(br);
> > + bridge_configure_lsample(br);
> > bridge_configure_spanning_tree(br);
> > bridge_configure_tables(br);
> > bridge_configure_dp_desc(br);
> > @@ -1537,10 +1539,11 @@ ovsrec_ipfix_is_valid(const struct ovsrec_ipfix
> > *ipfix)
> > return ipfix && ipfix->n_targets > 0;
> > }
> >
> > -/* Returns whether a Flow_Sample_Collector_Set row is valid. */
> > +/* Returns whether a Flow_Sample_Collector_Set row constains valid IPFIX
>
> constains -> 'contains a'
>
Ack.
> > + * configuration. */
> > static bool
> > -ovsrec_fscs_is_valid(const struct ovsrec_flow_sample_collector_set *fscs,
> > - const struct bridge *br)
> > +ovsrec_fscs_is_valid_ipfix(const struct ovsrec_flow_sample_collector_set
> > *fscs,
> > + const struct bridge *br)
> > {
> > return ovsrec_ipfix_is_valid(fscs->ipfix) && fscs->bridge == br->cfg;
> > }
> > @@ -1558,7 +1561,7 @@ bridge_configure_ipfix(struct bridge *br)
> > const char *virtual_obs_id;
> >
> > OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> > - if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> > + if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> > n_fe_opts++;
> > }
> > }
> > @@ -1621,7 +1624,7 @@ bridge_configure_ipfix(struct bridge *br)
> > fe_opts = xcalloc(n_fe_opts, sizeof *fe_opts);
> > opts = fe_opts;
> > OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> > - if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> > + if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> > opts->collector_set_id = fe_cfg->id;
> > sset_init(&opts->targets);
> > sset_add_array(&opts->targets, fe_cfg->ipfix->targets,
> > @@ -1667,6 +1670,71 @@ bridge_configure_ipfix(struct bridge *br)
> > }
> > }
> >
> > +/* Returns whether a Flow_Sample_Collector_Set row contains valid local
> > + * sampling configuraiton. */
>
> ...row contains a valid local...
> ++
>
> configuraiton -> configuration
Ack.
>
> > +static bool
> > +ovsrec_fscs_is_valid_local(const struct ovsrec_flow_sample_collector_set
> > *fscs,
> > + const struct bridge *br)
> > +{
> > + return fscs->local_sample_group && fscs->n_local_sample_group == 1 &&
> > + fscs->bridge == br->cfg;
> > +}
> > +
> > +/* Set local sample configuration on 'br'. */
> > +static void
> > +bridge_configure_lsample(struct bridge *br)
> > +{
> > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > + const struct ovsrec_flow_sample_collector_set *fscs;
> > + struct ofproto_lsample_options *opts_array, *opts;
> > + size_t n_opts = 0;
> > + int ret;
> > +
> > + /* Iterate the Flow_Sample_Collector_Set table twice.
> > + * First to get the number of valid configuration entries, then to
> > process
> > + * each of them and build an array of options. */
> > + OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH (fscs, idl) {
> > + if (ovsrec_fscs_is_valid_local(fscs, br)) {
> > + n_opts ++;
> > + }
> > + }
>
> Did you consider just allocating n_local_sample_group entries and only use
> the ones needed? Or are we assuming there is a large gap between the actual
> entries and the ones having a valid local entry?
n_local_sample_group is just the number of "local_sample_group" defined
in one particular FSCS row. It's optional so it can be 0 or 1.
Were allocating one configuration object for each valid row in the FSCS
table. This is done because FSCS rows can be configured with different
bridges and since this is a OVSDB relationship, the ofproto layer and
below cannot tell if the configuration entry is aplicable to that
ofproto or not.
>
> > + if (n_opts == 0) {
> > + ofproto_set_local_sample(br->ofproto, NULL, 0);
> > + return;
> > + }
> > +
> > + opts_array = xcalloc(n_opts, sizeof *opts_array);
> > + opts = opts_array;
> > +
> > + OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH (fscs, idl) {
> > + if (!ovsrec_fscs_is_valid_local(fscs, br)) {
> > + continue;
> > + }
> > + opts->collector_set_id = fscs->id;
> > + opts->group_id = *fscs->local_sample_group;
> > + opts++;
> > + }
> > +
> > + ret = ofproto_set_local_sample(br->ofproto, opts_array, n_opts);
> > +
> > + if (ret == ENOTSUP) {
> > + if (n_opts) {
> > + VLOG_WARN_RL(&rl,
> > + "bridge %s: ignoring local sampling
> > configuration: "
> > + "not supported by this datapath",
> > + br->name);
> > + }
> > + } else if (ret) {
> > + VLOG_ERR_RL(&rl, "bridge %s: error configuring local sampling: %s",
> > + br->name, ovs_strerror(ret));
> > + }
> > +
> > + if (n_opts > 0) {
> > + free(opts_array);
> > + }
> > +}
> > +
> > static void
> > port_configure_stp(const struct ofproto *ofproto, struct port *port,
> > struct ofproto_port_stp_settings *port_s,
> > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> > index e2d5e2e85..f945451cd 100644
> > --- a/vswitchd/vswitch.ovsschema
> > +++ b/vswitchd/vswitch.ovsschema
> > @@ -1,6 +1,6 @@
> > {"name": "Open_vSwitch",
> > - "version": "8.5.0",
> > - "cksum": "4040946650 27557",
> > + "version": "8.6.0",
> > + "cksum": "3864090052 27769",
> > "tables": {
> > "Open_vSwitch": {
> > "columns": {
> > @@ -562,6 +562,11 @@
> > "type": {"key": {"type": "uuid",
> > "refTable": "IPFIX"},
> > "min": 0, "max": 1}},
> > + "local_sample_group": {
> > + "type": {"key": {"type": "integer",
> > + "minInteger": 0,
> > + "maxInteger": 4294967295},
> > + "min": 0, "max": 1}},
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > "min": 0, "max": "unlimited"}}},
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 8a1b607d7..2a5223203 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -7008,10 +7008,37 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> > type=patch options:peer=p1 \
> >
> > <table name="Flow_Sample_Collector_Set">
> > <p>
> > - A set of IPFIX collectors of packet samples generated by OpenFlow
> > - <code>sample</code> actions. This table is used only for IPFIX
> > - flow-based sampling, not for per-bridge sampling (see the <ref
> > - table="IPFIX"/> table for a description of the two forms).
> > + A set of IPFIX or local sampling collectors of packet samples
> > generated
> > + by OpenFlow <code>sample</code> actions.
> > + </p>
> > +
> > + <p>
> > + If the column <code>ipfix</code> contains a reference to a
> > + valid IPFIX entry, samples will be emitted via IPFIX. This mechanism
> > + is known as flow-based IPFIX sampling, as opposed to bridge-based
> > + sampling (see the <ref table="IPFIX"/> table for a description of the
> > + two forms).
> > + </p>
> > +
> > + <p>
> > + If the column <code>local_sample_group</code> contains an integer
> > and the
>
> Can we call this local_group instead (or local_group_id)? This will avoid the
> long name, as this option is already in the Flow_Sample_Collector_Set table.
>
Sure, local group is fine.
> > + running datapath supports local sample emission, packets will be sent
> > + to some local sample collector. Samples will contain the group number
> > + specified by <code>local_sample_group</code> which helps identify
> > their
> > + source as well as a 64-bit cookie result from the concatenation of
> > the
> > + observation_domain_id an the observation_point_id.
>
> I guess this needs some clarification regarding endianness, etc., based on
> your discussion with Ilya.
>
Yep.
> > +
> > + The way the sample is emitted and made available for local collectors
> > + is datapath-specific.
> > +
> > + Currently only Linux kernel datapath supports local sampling which is
> > + implemented by sending the packet to the <code>psample</code> netlink
> > + multicast group.
> > + </p>
> > +
> > + <p>
> > + Note both <code>local_sample_group</code> and <code>ipfix</code> can
> > be
> > + configured simultaneously.
> > </p>
> >
> > <column name="id">
> > @@ -7030,6 +7057,10 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> > type=patch options:peer=p1 \
> > record per sampled packet to.
> > </column>
> >
> > + <column name="local_sample_group">
> > + Configuration of the sample group to be used in local sampling.
> > + </column>
> > +
>
> Not sure if we prefer putting the type here also, like other options
> do for documentation:
>
> type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967295}'>
>
> Or is having it in the ovsschema enough?
It doesn't harm to document it here as well.
>
> > <group title="Common Columns">
> > The overall purpose of these columns is described under <code>Common
> > Columns</code> at the beginning of this document.
> > --
> > 2.45.1
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
