On 12/10/24 12:45 PM, Ales Musil wrote:
> On Tue, Nov 12, 2024 at 6:38 PM Alexandra Rukomoinikova
> <[email protected]> wrote:
>
>> Added ability to output multiple individual chassis by their
>> name, hostname and encap-ip.
>>
>> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
>> Tested-by: Ivan Burnin <[email protected]>
>> ---
>> v2: - removed all copy from ovs function
>> - removed hash for hostnames
>> ---
>>
>
> Hi Alexandra,
>
> thank you for the followup and sorry for the delay, I have a few minor
> comments that could be addressed during merge if maintainers agree.
>
Hi Alexandra, Ales,
First of all, sorry for the delay in replying.
>
>> tests/ovn-sbctl.at | 9 ++++
>> utilities/ovn-sbctl.c | 109 +++++++++++++++++++++++++++++++++---------
>> 2 files changed, 96 insertions(+), 22 deletions(-)
>>
>> diff --git a/tests/ovn-sbctl.at b/tests/ovn-sbctl.at
>> index 19ac55c80..9743dacb8 100644
>> --- a/tests/ovn-sbctl.at
>> +++ b/tests/ovn-sbctl.at
>> @@ -164,6 +164,15 @@ Chassis ch0
>> Port_Binding vif0
>> ])
>>
>> +AT_CHECK([ovn-sbctl set Chassis ch0 hostname="ch0_host"])
>> +AT_CHECK([ovn-sbctl chassis-add ch1 geneve 1.2.3.6])
>> +AT_CHECK([ovn-sbctl chassis-add ch2 geneve 1.2.3.7])
>> +AT_CHECK([ovn-sbctl show ch0_host ch1 1.2.3.7 | grep Chassis], [0], [dnl
>> +Chassis ch0
>> +Chassis ch1
>> +Chassis ch2
>> +])
>> +
>> uuid=$(ovn-sbctl --columns=_uuid list Chassis ch0 | cut -d ':' -f2 | tr
>> -d ' ')
>> AT_CHECK_UNQUOTED([ovn-sbctl --columns=logical_port,mac,chassis list
>> Port_Binding], [0], [dnl
>> logical_port : vif0
>> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
>> index f1f8c2b42..e2ef37fa3 100644
>> --- a/utilities/ovn-sbctl.c
>> +++ b/utilities/ovn-sbctl.c
>> @@ -91,7 +91,7 @@ sbctl_usage(void)
>> usage: %s [OPTIONS] COMMAND [ARG...]\n\
>> \n\
>> General commands:\n\
>> - show print overview of database contents\n\
>> + show [CHASSIS | HOSTNAME | ENCAP-IP ...] print overview of database
>> contents\n\
>>
>
> As pointed out by checkpatch this line is longer than 79 characters.
>
>
>> \n\
>> Chassis commands:\n\
>> chassis-add CHASSIS ENCAP-TYPE ENCAP-IP create a new chassis named\n\
>> @@ -355,12 +355,14 @@ static void
>> pre_get_info(struct ctl_context *ctx)
>> {
>> ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_col_name);
>> + ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_col_hostname);
>> ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_col_encaps);
>>
>> ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_private_col_name);
>>
>> ovsdb_idl_add_column(ctx->idl, &sbrec_encap_col_type);
>> ovsdb_idl_add_column(ctx->idl, &sbrec_encap_col_ip);
>> + ovsdb_idl_add_column(ctx->idl, &sbrec_encap_col_options);
>>
>> ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_logical_port);
>> ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_tunnel_key);
>> @@ -404,26 +406,6 @@ pre_get_info(struct ctl_context *ctx)
>> ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_protocol);
>> }
>>
>> -static struct cmd_show_table cmd_show_tables[] = {
>> - {&sbrec_table_chassis,
>> - &sbrec_chassis_col_name,
>> - {&sbrec_chassis_col_hostname,
>> - &sbrec_chassis_col_encaps,
>> - NULL},
>> - {&sbrec_table_port_binding,
>> - &sbrec_port_binding_col_logical_port,
>> - &sbrec_port_binding_col_chassis}},
>> -
>> - {&sbrec_table_encap,
>> - &sbrec_encap_col_type,
>> - {&sbrec_encap_col_ip,
>> - &sbrec_encap_col_options,
>> - NULL},
>> - {NULL, NULL, NULL}},
>> -
>> - {NULL, NULL, {NULL, NULL, NULL}, {NULL, NULL, NULL}},
>> -};
>> -
I'm not sure I like this too much unfortunately. It means we will have
to write boilerplate code every time we want to add a new table to the
"show" command.
Isn't it a better option to enhance the OVS cmd_show() implementation
[0] to accept an optional list of arguments to filter on?
CC Ilya, in case he has ideas too.
Thanks,
Dumitru
[0]
https://github.com/openvswitch/ovs/blob/77ac0b28c868c724675b4004d554f5938ce13693/lib/db-ctl-base.c#L2548-L2554
>> static void
>> sbctl_init(struct ctl_context *ctx OVS_UNUSED)
>> {
>> @@ -564,6 +546,86 @@ cmd_lsp_unbind(struct ctl_context *ctx)
>> }
>> }
>>
>> +static void
>> +show_encap(struct ctl_context *ctx,
>> + const struct sbrec_encap *encap)
>> +{
>> + ds_put_format(&ctx->output, " Encap %s\n", encap->type);
>> + ds_put_format(&ctx->output, " ip: \"%s\"\n", encap->ip);
>> + ds_put_format(&ctx->output, " options: {csum=\"%s\"}\n",
>> + smap_get_def(&encap->options, "csum", " "));
>> +};
>> +
>> +static void
>> +show_chassis(struct ctl_context *ctx,
>> + const struct sbrec_chassis *chassis)
>> +{
>> + const struct sbrec_port_binding *pb;
>> +
>> + ds_put_format(&ctx->output, "Chassis %s\n", chassis->name);
>> +
>> + if (chassis->hostname && strlen(chassis->hostname)) {
>>
>
> No need for strlen, simple "chassis->hostname[0]" is sufficient.
>
>
>> + ds_put_format(&ctx->output, " hostname: %s\n",
>> chassis->hostname);
>> + }
>> +
>> + for (size_t i = 0; i < chassis->n_encaps; i++) {
>> + show_encap(ctx, chassis->encaps[i]);
>> + }
>> +
>> + SBREC_PORT_BINDING_FOR_EACH (pb, ctx->idl) {
>> + if (pb->chassis == chassis) {
>> + ds_put_format(&ctx->output, " Port_Binding %s\n",
>> + pb->logical_port);
>> + }
>> + }
>> +}
>> +
>> +static const struct sbrec_chassis *
>> +sbctl_find_chassis(struct ctl_context *ctx, const char *name)
>> +{
>> + const struct sbctl_chassis *chassis = NULL;
>> + const struct sbrec_chassis *chassis_rec = NULL;
>> +
>> + /* Find chassis by his name. */
>> + chassis = find_chassis(ctx, name, false);
>> + if (chassis) {
>> + return chassis->ch_cfg;
>> + }
>> +
>> + /* Find chassis by his hostname or encap ip. */
>> + SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->idl) {
>> + if (chassis_rec->hostname && !strcmp(chassis_rec->hostname,
>> name)) {
>> + return chassis_rec;
>> + }
>> + for (size_t i = 0; i < chassis_rec->n_encaps; i++) {
>> + if (!strcmp(chassis_rec->encaps[i]->ip, name)) {
>> + return chassis_rec;
>> + }
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static void
>> +cmd_show(struct ctl_context *ctx)
>> +{
>> + const struct sbrec_chassis *chassis;
>> +
>> + if (ctx->argc > 1) {
>> + for (int i = 1; i <= ctx->argc - 1; i++) {
>> + chassis = sbctl_find_chassis(ctx, ctx->argv[i]);
>> + if (chassis) {
>> + show_chassis(ctx, chassis);
>> + }
>> + }
>> + } else {
>> + SBREC_CHASSIS_FOR_EACH (chassis, ctx->idl) {
>> + show_chassis(ctx, chassis);
>> + }
>> + }
>> +}
>> +
>> enum {
>> PL_INGRESS,
>> PL_EGRESS,
>> @@ -1554,6 +1616,9 @@ static const struct ctl_table_class
>> tables[SBREC_N_TABLES] = {
>> static const struct ctl_command_syntax sbctl_commands[] = {
>> { "init", 0, 0, "", NULL, sbctl_init, NULL, "", RW },
>>
>> + { "show", 0, INT_MAX, "[CHASSIS | HOSTNAME | ENCAP-IP ...]",
>> + pre_get_info, cmd_show, NULL, "", RO },
>> +
>> /* Chassis commands. */
>> {"chassis-add", 3, 3, "CHASSIS ENCAP-TYPE ENCAP-IP", pre_get_info,
>> cmd_chassis_add, NULL, "--may-exist", RW},
>> @@ -1610,7 +1675,7 @@ main(int argc, char *argv[])
>>
>> .idl_class = &sbrec_idl_class,
>> .tables = tables,
>> - .cmd_show_table = cmd_show_tables,
>> + .cmd_show_table = NULL,
>> .commands = sbctl_commands,
>>
>> .usage = sbctl_usage,
>> --
>> 2.34.1
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> With those addressed:
>
> Acked-by: Ales Musil <[email protected]>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
