On Thu, Jun 27, 2024 at 03:57:03PM GMT, Eelco Chaudron wrote:
>
>
> 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.
>
Sorry, my head is not working very well today and I'm not understanding
your point.
n_local_sample_group might 1 or 0 indicating whether there is local
sampling or not in this collector. The actual value can be anything
between 1 and 2^32 -1 and it is completely up to the user to configure
something that will be unique in their system (e.g: do not collide
with other act_sample actions they might have installed).
I don't expect many FSCS rows to be configured. Let's take en example.
Let's say we have 2 bridges, 10 FSCS entries, 5 for each bridge. From
them, only 3 have local sampling (the other 2 have only IPFIX
configuration). So, n_local_sample_group is 1 in 6 of the entries and 0
in the rest. The actual number can be on 10001-10006 for instance.
Here we are configuring one ofproto (i.e: one bridge). Since we're
storing the configuration in an array, we need to iterate twice. First
time we realize that, out of the 10 rows, only 3 of them return true in
ovsrec_fscs_is_valid_local(). So we allocate an array of size 3, create
configuration objects for those and send them down to ofproto-dpif.
The only other way I can see of avoiding the double loop is to allocate
the total number of rows, 10 in the previous example, and add a boolean
in the configuration that tells the ofproto layer that the option must
be ignored because, it either didn't have local sampling
(n_local_sample_group=0) or it belongs to another bridge.
Considering the amount of rows we expect to have is low, I think a
double loop yields cleaner code in this case.
> >>> + 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
