On 29.05.2025 22:21, Ilya Maximets wrote:
> On 5/29/25 7:39 PM, Rukomoinikova Aleksandra wrote:
>> Ilya, hi! thanks for the comments, I fixed the storage of filters on the
>> sset structure, but I have a question below
>>
>> On 29.05.2025 19:10, Ilya Maximets wrote:
>>> On 5/29/25 6:02 PM, Ilya Maximets wrote:
>>>> On 5/20/25 7:16 PM, Alexandra Rukomoinikova wrote:
>>>>> Added the ability to filter the output of the show command
>>>>> using the filter option.
>>>>>
>>>>> Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud>
>>>>> ---
>>>>> v1 --> v2 : did as Ilya said.
>>>>> ---
>>>>> lib/db-ctl-base.c | 75 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>> tests/ovs-vsctl.at | 28 +++++++++++++++++
>>>>> 2 files changed, 102 insertions(+), 1 deletion(-)
>>>> Hi, Alexandra. Thanks for the new version!
>>>>
>>>> nit: The 'lib:' in the patch name doesn't make a lot of sense as vast
>>>> majority of OVS code is in lib. It should be 'db-ctl-base:' instead.
>>>>
>>>> Since this patch adds a new option, it needs to be documented in the
>>>> man page (utilities/ovs-vsctl.8.in) and a NEWS entry should be added.
>>>>
>>>> Some more comments below.
>>>>
>>>>> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
>>>>> index 1f157e46c..45d2d51c0 100644
>>>>> --- a/lib/db-ctl-base.c
>>>>> +++ b/lib/db-ctl-base.c
>>>>> @@ -2243,19 +2243,92 @@ cmd_show_row(struct ctl_context *ctx, const
>>>>> struct ovsdb_idl_row *row,
>>>>> sset_find_and_delete_assert(shown, show->table->name);
>>>>> }
>>>>>
>>>>> +struct output_filter {
>>>>> + char **filters;
>>>>> + size_t n_filters;
>>>>> +};
>>>>> +
>>>>> +static void
>>>>> +filter_row(struct ctl_context *ctx,
>>>>> + struct output_filter *filter,
>>>>> + size_t old_lenght)
>>>> s/lenght/length/
>>>>
>>>> here and in a few other places in the code.
>>>>
>>>>> +{
>>>>> + bool filter_match = false;
>>>>> +
>>>>> + for (size_t i = 0; i < filter->n_filters; i++) {
>>>>> + if (strstr(&ctx->output.string[old_lenght], filter->filters[i]))
>>>>> {
>>>>> + filter_match = true;
>>>> break here. There is no need to continue the loop.
>>>>
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + if (!filter_match) {
>>>>> + ds_truncate(&ctx->output, old_lenght);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static struct output_filter *
>>>>> +init_filters(struct ctl_context *ctx)
>>>>> +{
>>>>> + char *filter_str = shash_find_data(&ctx->options, "--filter");
>>>>> + if (!filter_str || !*filter_str) {
>>>>> + return NULL;
>>>>> + }
>>>>> +
>>>>> + size_t count = 1;
>>>>> + for (const char *p = filter_str; *p; p++) {
>>>>> + if (*p == '|') {
>>>>> + count++;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + struct output_filter *filter = xmalloc(sizeof(struct output_filter));
>>>>> + filter->filters = xmalloc(count * (sizeof(char *)));
>>>>> + filter->n_filters = count;
>>>>> +
>>>>> + char *ptr = NULL;
>>>>> + char *token = strtok_r(filter_str, "|", &ptr);
>>>>> + size_t idx = 0;
>>>>> +
>>>>> + while (token && idx < count) {
>>>>> + filter->filters[idx++] = xstrdup(token);
>>>>> + token = strtok_r(NULL, "|", &ptr);
>>>>> + }
>>>>> +
>>>>> + return filter;
>>>>> +}
>>>> If we use sset as a data structure for the filters, the code above can
>>>> be mostly replaced with an sset_from_delimited_string() call.
>>>> The sset is slightly less memory-efficient, but I don't think we care
>>>> in this particular case.
>>>>
>>>>> +
>>>>> +static void
>>>>> +free_filters(struct output_filter *filter)
>>>>> +{
>>>>> + if (!filter) {
>>>>> + return;
>>>>> + }
>>>>> + for (size_t i = 0; i < filter->n_filters; i++) {
>>>>> + free(filter->filters[i]);
>>>>> + }
>>>>> + free(filter->filters);
>>>>> + free(filter);
>>>>> +}
>>>>> +
>>>>> static void
>>>>> cmd_show(struct ctl_context *ctx)
>>>>> {
>>>>> const struct ovsdb_idl_row *row;
>>>>> + struct output_filter *filter = init_filters(ctx);
>>>>> struct sset shown = SSET_INITIALIZER(&shown);
>>>>>
>>>>> for (row = ovsdb_idl_first_row(ctx->idl, cmd_show_tables[0].table);
>>>>> row; row = ovsdb_idl_next_row(row)) {
>>>>> + size_t lenght_before = ctx->output.length;
>>>>> cmd_show_row(ctx, row, 0, &shown);
>>>>> + if (filter) {
>>>>> + filter_row(ctx, filter, lenght_before);
>>>>> + }
>>>> This only covers the top-level elements, but the idea was to perform the
>>>> filtering recursively for every level. You should move the filtering into
>>>> the cmd_show_row() itself, i.e. save the length at the top of
>>>> cmd_show_row()
>>>> and run the filtering at the bottom. This will ensure that we're filtering
>>>> all the way through.
>>>>
>>>> For example, if we have a setup like this:
>>>>
>>>> $ ovs-vsctl add-br br1
>>>> $ ovs-vsctl add-br br0
>>>> $ ovs-vsctl add-port br0 p1
>>>> $ ovs-vsctl add-port br0 tunnel_port \
>>>> -- set Interface tunnel_port type=geneve \
>>>> options:remote_ip=flow options:key=123
>>>> $ ovs-vsctl add-port br0 tunnel_port2 \
>>>> -- set Interface tunnel_port2 type=geneve \
>>>> options:remote_ip=flow options:key=125
>>>>
>>>> Then I can get a list of bridges only with:
>>>>
>>>> $ ovs-vsctl --filter="Bridge" show
>>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0
>>>> Bridge br1
>>>> Bridge br0
>>>>
>>>> Since the 'Bridge' match happens at the higher level, all the port
>>>> and interface information is filtered out.
>>>>
>>>> Or a list of ports per bridge:
>>>>
>>>> $ ovs-vsctl --filter="Port" show
>>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0
>>>> Bridge br1
>>>> Port br1
>>>> Bridge br0
>>>> Port p1
>>>> Port tunnel_port
>>>> Port tunnel_port2
>>>> Port br0
>>>>
>>>> Here only the interface information is filtered out since once we match
>>>> on the 'Port' string, all the higher-level nodes will match as well.
>>>>
>>>> Or if I want to only show geneve interfaces:
>>>>
>>>> $ ovs-vsctl --filter="geneve" show
>>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0
>>>> Bridge br0
>>>> Port tunnel_port
>>>> Interface tunnel_port
>>>> type: geneve
>>>> options: {key="123", remote_ip=flow}
>>>> Port tunnel_port2
>>>> Interface tunnel_port2
>>>> type: geneve
>>>> options: {key="125", remote_ip=flow}
>>>>
>>>>
>>>> Or if I want to show all tunnels with the tunnel key 123:
>>>>
>>>> $ ovs-vsctl --filter='key="123"' show
>>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0
>>>> Bridge br0
>>>> Port tunnel_port
>>>> Interface tunnel_port
>>>> type: geneve
>>>> options: {key="123", remote_ip=flow}
>>>>
>>>>
>>>> Or I want the previous one, but also names of all the ports per bridge:
>>>>
>>>> $ ovs-vsctl --filter='key="123"|Port' show
>>>> d55f21cd-a59a-4548-b1bc-fc165b0ec0a0
>>>> Bridge br1
>>>> Port br1
>>>> Bridge br0
>>>> Port p1
>>>> Port tunnel_port
>>>> Interface tunnel_port
>>>> type: geneve
>>>> options: {key="123", remote_ip=flow}
>>>> Port tunnel_port2
>>>> Port br0
>>>>
>>>>
>>>> What do you think?
>> my idea was in many ways for the sbctl command to be able to filter by
>> chassis, it seems that in your approach this will not be available, and
>> if I filter by chassis name, I will only see its chassis name and will
>> not see all the information about enсap-ips and port bindings.
> Hmm, good point. The recursive logic works nicely for the OVS schema,
> but not so much for the OVN_Southbound. So, we probably want is an
> option to apply filters at the level of specific tables. I spent some
> time thinking what would be a good syntax for this that would not limit
> the filtering options too much, and I came up with something like this:
>
> --filter=TABLE(FILTER[|FILTER...]),...
>
> So, for filtering on chassis names, but keeping all the encap and the
> port binding informations, it will look like:
>
> ovn-sbctl --filter="chassis(ch0|ch1)" show
>
> This will mean that we only apply this filter in cmd_show_row() when
> the current show->table is "chassis".
>
> If we'll want to only show specific port bindings in the found chassis,
> then we'll be able to do something like this:
>
> ovn-sbctl --filter="chassis(ch0|ch1),port-binding(sw0-p)" show
>
> This will still print all the encap-ips for all the found chassis, but
> will only print port bindings that contain 'sw0-p' substring.
>
> And we should still allow the simple case without specifying the table
> that should do the filtering on all levels, e.g.
>
> ovs-vsctl --filter='Port' show
>
> That will still only print port names inside all the bridges.
> And something like this:
>
> ovs-vsctl --filter='bridge(key=123)' show
>
> would print every port and interface of a bridge if at least one of the
> interfaces in it is a tunnel interface with a key=123.
>
> So, the solution would look very similar, but parsing will be a bit more
> involved... First we'll need to split by a comma, then parse each chunk
> by finding the first parenthesis and treating everything inside as a
> list of filters for the table. As a data structure to store all that,
> may use an shash with sset as a value, I guess. If the name of a current
> table is not in the shash, then use the filter that has no table specified.
>
> What do you think?
I think this approach is not very user-friendly, I would honestly get
tired of writing this in the show command=) maybe I'll make 2 flags?
just a filter that will implement your approach, and the second
filter-all (or filter-table, but i like filter-all more), when we can
filter the table completely ?
>>>>> }
>>>>>
>>>>> ovs_assert(sset_is_empty(&shown));
>>>>> sset_destroy(&shown);
>>>>> + free_filters(filter);
>>>>> }
>>>>>
>>>>>
>>>>> @@ -2548,7 +2621,7 @@ ctl_init__(const struct ovsdb_idl_class *idl_class_,
>>>>> cmd_show_tables = cmd_show_tables_;
>>>>> if (cmd_show_tables) {
>>>>> static const struct ctl_command_syntax show =
>>>>> - {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL, "", RO};
>>>>> + {"show", 0, 0, "", pre_cmd_show, cmd_show, NULL,
>>>>> "--filter=", RO};
>>>>> ctl_register_command(&show);
>>>>> }
>>>>> }
>>>>> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
>>>>> index e488e292d..010889789 100644
>>>>> --- a/tests/ovs-vsctl.at
>>>>> +++ b/tests/ovs-vsctl.at
>>>>> @@ -1831,3 +1831,31 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns
>>>>> _uuid,name list bridge tst1], [0]
>>>>>
>>>>> OVS_VSCTL_CLEANUP
>>>>> AT_CLEANUP
>>>>> +
>>>>> +AT_SETUP([ovs-vsctl filter option usage])
>>>> nit: I'd name it 'ovs-vsctl show filtered' or something like that.
>>>>
>>>>> +AT_KEYWORDS([ovs-vsctl filter option])
>>>> nit :I'd replace the 'option' with 'show'. 'option' is a not very specific
>>>> keyword to use.
>>>>
>>>>> +
>>>>> +OVS_VSWITCHD_START([dnl
>>>>> + add-port br0 p1 -- set Interface p1 type=internal ofport_request=1 --
>>>>> \
>>>>> + add-port br0 p2 -- set Interface p2 type=internal ofport_request=2 --
>>>>> \
>>>>> + add-port br0 p3 -- set Interface p3 type=internal ofport_request=3
>>>>> +])
>>>>> +
>>>>> +AT_CHECK([ovs-vsctl --filter='p1' show | grep Port | sort], [0], [dnl
>>>>> + Port br0
>>>>> + Port p1
>>>>> + Port p2
>>>>> + Port p3
>>>>> +])
>>>>> +
>>>>> +AT_CHECK([ovs-vsctl --filter='test' show | grep Port | sort], [0], [])
>>>>> +
>>>>> +AT_CHECK([ovs-vsctl --filter='Bridge' show | grep Port | sort], [0], [dnl
>>>>> + Port br0
>>>>> + Port p1
>>>>> + Port p2
>>>>> + Port p3
>>>>> +])
>>>> We should print the whole thing here. It will not change from run to run,
>>>> because database structures are sorted and our top-level table only has one
>>>> row. The only thing that will change is the datapath uuid at the very top.
>>>> You may just cut out the first line with tail -n +2 or feed the output into
>>>> the uuidfilt.
>>> Ignore that part, we do not control the UUIDs of referenced rows, so the
>>> output
>>> order will change between runs, unfortunately.
>>>
>>>> Maybe add a bit more variety to the tests. Different interface types.
>>>>
>>>>> +
>>>>> +OVS_VSCTL_CLEANUP
>>>>> +AT_CLEANUP
>>>>> \ No newline at end of file
>>>> Add the new line here!
>>>>
>>>> Best regards, Ilya Maximets.
>>>> _______________________________________________
>>>> dev mailing list
>>>> d...@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>
--
regards,
Alexandra.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
