On 27 Jun 2024, at 13:37, Adrián Moreno wrote:
> 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. I understand that part, was just wondering if there is a big discrepancy between the actual n_local_sample_group number and the amount local entries we are interested in, ovsrec_fscs_is_valid_local(), in a typical configuration. This to avoid two loops through all of them. But this is more of a nit than an important part. >>> + 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
