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 <[email protected]>
> ---
> 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'
> + * 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
> +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?
> + 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.
> + 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.
> +
> + 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?
> <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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
