On 28 Jun 2024, at 15:53, Adrián Moreno wrote:
> 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. My bad, this is what I was suggesting to see if it would make sense. I miss quoted n_local_sample_group, which caused the confusion. > Considering the amount of rows we expect to have is low, I think a > double loop yields cleaner code in this case. ACK. >>>>> + 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
