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?
>>>> }
>>>>
>>>> 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
