| Message ID | 20241115163552.1109388-1-mj.ponsonby@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] Allow creation of a LRP without ipv4 | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi MJ, thanks for enabling LRPs without explicit IP address. The code looks good to me, but I think that the documentation needs updates in couple more places: * manpage for ovn-nbctl [0] * possibly manpage for ovn-nb [1]. While the Summary and the short description of the field will get automatically generated based on the schema, from "set of 1 or more strings" to "set of strings", I believe that there's no harm in explicitly mentioning that the IP addresses are optional. Also, since this is a user visible change to the NB database, I believe that a NEWS entry [2] is also warranted. [0] https://github.com/ovn-org/ovn/blob/3e177f726635c509f4f11e8112d8c31527d0c2c7/utilities/ovn-nbctl.8.xml#L932 [1] https://github.com/ovn-org/ovn/blob/3e177f726635c509f4f11e8112d8c31527d0c2c7/ovn-nb.xml#L3222-L3235 [2] https://docs.ovn.org/en/latest/internals/contributing/submitting-patches.html#before-you-start Thanks, Martin. On Fri, 2024-11-15 at 16:35 +0000, MJ Ponsonby wrote: > This makes it so that networks are an optional argument for lrp-add, > and > in the case where no networks are provided it creates the logical > router > port but doesn't add the routes that require ipv4. > > This is useful as part of an effort to get OVN+BGP unnumbered working > with > minimal configuration. > > Signed-off-by: MJ Ponsonby <mj.ponsonby@canonical.com> > --- > ovn-nb.ovsschema | 6 +++--- > tests/ovn-nbctl.at | 13 +++++++++++++ > utilities/ovn-nbctl.c | 11 ++--------- > 3 files changed, 18 insertions(+), 12 deletions(-) > > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > index c4a48183d..bda387242 100644 > --- a/ovn-nb.ovsschema > +++ b/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > - "version": "7.7.0", > - "cksum": "116357561 38626", > + "version": "7.7.1", > + "cksum": "3555498067 38626", > "tables": { > "NB_Global": { > "columns": { > @@ -468,7 +468,7 @@ > "min": 0, > "max": "unlimited"}}, > "networks": {"type": {"key": "string", > - "min": 1, > + "min": 0, > "max": "unlimited"}}, > "mac": {"type": "string"}, > "peer": {"type": {"key": "string", "min": 0, "max": > 1}}, > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 2efa13b93..ec233a6d0 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -1697,6 +1697,19 @@ AT_CHECK([ovn-nbctl lrp-list lr0 | uuidfilt], > [0], [dnl > <1> (lrp1) > ]) > > +AT_CHECK([ovn-nbctl lrp-add lr0 lrp2 00:00:00:01:02:03]) > +AT_CHECK([ovn-nbctl lrp-list lr0 | uuidfilt], [0], [dnl > +<0> (lrp0) > +<1> (lrp1) > +<2> (lrp2) > +]) > + > +AT_CHECK([ovn-nbctl lrp-del lrp2]) > +AT_CHECK([ovn-nbctl lrp-list lr0 | uuidfilt], [0], [dnl > +<0> (lrp0) > +<1> (lrp1) > +]) > + > AT_CHECK([ovn-nbctl lr-add lr1]) > AT_CHECK([ovn-nbctl lrp-add lr0 lrp1 00:00:00:01:02:03 > 192.168.1.1/24], [1], [], > [ovn-nbctl: lrp1: a port with this name already exists > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index d45be75c7..6513324a9 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -6005,13 +6005,6 @@ nbctl_lrp_add(struct ctl_context *ctx) > break; > } > } > - > - if (!n_networks) { > - ctl_error(ctx, "%s: router port requires specifying a > network", > - lrp_name); > - return; > - } > - > char **settings = (char **) &ctx->argv[n_networks + 4]; > int n_settings = ctx->argc - 4 - n_networks; > > @@ -8057,8 +8050,8 @@ static const struct ctl_command_syntax > nbctl_commands[] = { > { "lr-list", 0, 0, "", nbctl_pre_lr_list, nbctl_lr_list, NULL, > "", RO }, > > /* logical router port commands. */ > - { "lrp-add", 4, INT_MAX, > - "ROUTER PORT MAC NETWORK... [COLUMN[:KEY]=VALUE]...", > + { "lrp-add", 3, INT_MAX, > + "ROUTER PORT MAC [NETWORK]... [COLUMN[:KEY]=VALUE]...", > nbctl_pre_lrp_add, nbctl_lrp_add, NULL, "--may-exist", RW }, > { "lrp-set-gateway-chassis", 2, 3, > "PORT CHASSIS [PRIORITY]",
diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema index c4a48183d..bda387242 100644 --- a/ovn-nb.ovsschema +++ b/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", - "version": "7.7.0", - "cksum": "116357561 38626", + "version": "7.7.1", + "cksum": "3555498067 38626", "tables": { "NB_Global": { "columns": { @@ -468,7 +468,7 @@ "min": 0, "max": "unlimited"}}, "networks": {"type": {"key": "string", - "min": 1, + "min": 0, "max": "unlimited"}}, "mac": {"type": "string"}, "peer": {"type": {"key": "string", "min": 0, "max": 1}}, diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 2efa13b93..ec233a6d0 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -1697,6 +1697,19 @@ AT_CHECK([ovn-nbctl lrp-list lr0 | uuidfilt], [0], [dnl <1> (lrp1) ]) +AT_CHECK([ovn-nbctl lrp-add lr0 lrp2 00:00:00:01:02:03]) +AT_CHECK([ovn-nbctl lrp-list lr0 | uuidfilt], [0], [dnl +<0> (lrp0) +<1> (lrp1) +<2> (lrp2) +]) + +AT_CHECK([ovn-nbctl lrp-del lrp2]) +AT_CHECK([ovn-nbctl lrp-list lr0 | uuidfilt], [0], [dnl +<0> (lrp0) +<1> (lrp1) +]) + AT_CHECK([ovn-nbctl lr-add lr1]) AT_CHECK([ovn-nbctl lrp-add lr0 lrp1 00:00:00:01:02:03 192.168.1.1/24], [1], [], [ovn-nbctl: lrp1: a port with this name already exists diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index d45be75c7..6513324a9 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -6005,13 +6005,6 @@ nbctl_lrp_add(struct ctl_context *ctx) break; } } - - if (!n_networks) { - ctl_error(ctx, "%s: router port requires specifying a network", - lrp_name); - return; - } - char **settings = (char **) &ctx->argv[n_networks + 4]; int n_settings = ctx->argc - 4 - n_networks; @@ -8057,8 +8050,8 @@ static const struct ctl_command_syntax nbctl_commands[] = { { "lr-list", 0, 0, "", nbctl_pre_lr_list, nbctl_lr_list, NULL, "", RO }, /* logical router port commands. */ - { "lrp-add", 4, INT_MAX, - "ROUTER PORT MAC NETWORK... [COLUMN[:KEY]=VALUE]...", + { "lrp-add", 3, INT_MAX, + "ROUTER PORT MAC [NETWORK]... [COLUMN[:KEY]=VALUE]...", nbctl_pre_lrp_add, nbctl_lrp_add, NULL, "--may-exist", RW }, { "lrp-set-gateway-chassis", 2, 3, "PORT CHASSIS [PRIORITY]",
This makes it so that networks are an optional argument for lrp-add, and in the case where no networks are provided it creates the logical router port but doesn't add the routes that require ipv4. This is useful as part of an effort to get OVN+BGP unnumbered working with minimal configuration. Signed-off-by: MJ Ponsonby <mj.ponsonby@canonical.com> --- ovn-nb.ovsschema | 6 +++--- tests/ovn-nbctl.at | 13 +++++++++++++ utilities/ovn-nbctl.c | 11 ++--------- 3 files changed, 18 insertions(+), 12 deletions(-)