On 5/29/25 10:03 PM, Rukomoinikova Aleksandra wrote:
> 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=)
Fair. :) Also, OVN doesn't make things easy by having a lot of tables with
the same prefixes like 'logical-' or 'chassis', defeating the partial match
mechanism.
> 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 ?
A counteroffer: We have just one option, but if the table is not specified, then
the filtering happens only at the top level as in your current implementation.
But if the table is specified, then the filtering happens at the level of that
particular table. i.e. --filter='ch0' in ovn-sbctl will be the same as
--filter='chassis(ch0)', but I would still be able to use ovs-vsctl with
--filter='interface(geneve)' to filter out only the geneve interfaces or use
--filter='port(Port),int(key=123)' to print all the port names, but only print
tunnel interfaces with the key=123 in them.
Does that sound reasonable?
>>>>>> }
>>>>>>
>>>>>> 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
>>>>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
