On 1/20/22 11:38, Lorenzo Bianconi wrote:
> Introduce the capability to specify CoPP UUID or CoPP name in order to
> reuse the same CoPP reference on multiple datapaths.
> Introduce logical_router and logical_switches columns in CoPP table in
> order to specify datapaths where CoPP is installed.
>
> Reported-ad: https://bugzilla.redhat.com/show_bug.cgi?id=2040852
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
Hi Lorenzo,
> ovn-nb.ovsschema | 15 +++++-
> ovn-nb.xml | 9 ++++
> tests/ovn-northd.at | 27 ++++++++++
> utilities/ovn-nbctl.8.xml | 16 ++++--
> utilities/ovn-nbctl.c | 103 ++++++++++++++++++++++++++++++++------
> 5 files changed, 150 insertions(+), 20 deletions(-)
>
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 55977339a..cf2947d93 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Northbound",
> - "version": "5.34.1",
> - "cksum": "2177334725 30782",
> + "version": "5.35.0",
> + "cksum": "2039436985 31434",
> "tables": {
> "NB_Global": {
> "columns": {
> @@ -32,6 +32,17 @@
> "isRoot": true},
> "Copp": {
> "columns": {
> + "name": {"type": "string"},
I think name should be an index too. That would be a backwards
incompatible change but, AFAIK, there are no users of CoPP yet.
What do you think?
Thanks,
Dumitru
> + "logical_switch": {"type": {"key": {"type": "uuid",
> + "refTable": "Logical_Switch",
> + "refType": "strong"},
> + "min": 0,
> + "max": "unlimited"}},
> + "logical_router": {"type": {"key": {"type": "uuid",
> + "refTable": "Logical_Router",
> + "refType": "strong"},
> + "min": 0,
> + "max": "unlimited"}},
> "meters": {
> "type": {"key": "string",
> "value": "string",
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 6a6972856..4d319267f 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -360,6 +360,15 @@
> associate entries from table <ref table="Meter"/> to control protocol
> names.
> </p>
> + <column name="name">
> + CoPP name.
> + </column>
> + <column name="logical_switch">
> + Reference to <ref table="Logical_Switch"/> where the CoPP is installed.
> + </column>
> + <column name="logical_router">
> + Reference to <ref table="Logical_Router"/> where the CoPP is installed.
> + </column>
> <column name="meters" key="arp">
> Rate limiting meter for ARP packets (request/reply) used for learning
> neighbors.
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 652903761..bd284c915 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3403,6 +3403,33 @@ check ovn-nbctl lr-copp-del r0
> AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> ])
>
> +check ovn-nbctl ls-copp-del sw1
> +AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
> +])
> +
> +check ovn-nbctl --wait=hv lr-copp-add copp0 r0 arp meter0
> +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> +arp: meter0
> +])
> +
> +AT_CHECK([fetch_column nb:CoPP name], [0], [dnl
> +copp0
> +])
> +
> +lr_uuid=$(fetch_column nb:Logical_Router _uuid)
> +copp_lr_uuid=$(fetch_column nb:CoPP logical_router)
> +AT_CHECK([test "$lr_uuid" = "$copp_lr_uuid"])
> +
> +copp_uuid=$(fetch_column nb:CoPP _uuid)
> +check ovn-nbctl --wait=hv ls-copp-add $copp_uuid sw1 arp meter0
> +
> +ls_uuid=$(fetch_column nb:Logical_Switch _uuid)
> +copp_ls_uuid=$(fetch_column nb:CoPP logical_switch)
> +AT_CHECK([test "$ls_uuid" = "$copp_ls_uuid"])
> +
> +ls_copp_uuid=$(fetch_column nb:Logical_Switch copp)
> +AT_CHECK([test "$ls_copp_uuid" = "$copp_uuid"])
> +
> AT_CLEANUP
> ])
>
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index 80a564660..98326dcc2 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -1474,13 +1474,17 @@
> </p>
>
> <dl>
> - <dt><code>ls-copp-add</code> <var>switch</var> <var>proto</var>
> - <var>meter</var></dt>
> + <dt><code>ls-copp-add</code> [<var>UUID</var>|<var>name</var>]
> + <var>switch</var> <var>proto</var> <var>meter</var></dt>
> <dd>
> Adds the control <code>proto</code> to <code>meter</code> mapping
> to the <code>switch</code> control plane protection policy. If no
> policy exists yet, it creates one. If a mapping already existed for
> <code>proto</code>, this will overwrite it.
> + If <var>UUID</var> is provided, the already installed will be reused
> + (if not found and error will be reported).
> + If <var>name</var> is provided, CoPP name can be used for CoPP
> + table lookup.
> </dd>
>
> <dt><code>ls-copp-del</code> <var>switch</var> [<var>proto</var>]</dt>
> @@ -1497,13 +1501,17 @@
> <code>switch</code>.
> </dd>
>
> - <dt><code>lr-copp-add</code> <var>router</var> <var>proto</var>
> - <var>meter</var></dt>
> + <dt><code>lr-copp-add</code> [<var>UUID</var>|<var>name</var>]
> + <var>router</var> <var>proto</var> <var>meter</var></dt>
> <dd>
> Adds the control <code>proto</code> to <code>meter</code> mapping
> to the <code>router</code> control plane protection policy. If no
> policy exists yet, it creates one. If a mapping already existed for
> <code>proto</code>, this will overwrite it.
> + If <var>UUID</var> is provided, the already installed will be reused
> + (if not found and error will be reported).
> + If <var>name</var> is provided, CoPP name can be used for CoPP
> + table lookup.
> </dd>
>
> <dt><code>lr-copp-del</code> <var>router</var> [<var>proto</var>]</dt>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index d67d2db65..8889f1c6b 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -437,7 +437,7 @@ chassis with mandatory PRIORITY to the HA chassis group
> GRP\n\
> CHASSIS from the HA chassis group GRP\n\
> \n\
> Control Plane Protection Policy commands:\n\
> - ls-copp-add SWITCH PROTO METER\n\
> + ls-copp-add [UUID|NAME] SWITCH PROTO METER\n\
> Add a copp policy for PROTO packets on SWITCH\n\
> based on an existing METER.\n\
> ls-copp-del SWITCH [PROTO]\n\
> @@ -447,7 +447,7 @@ Control Plane Protection Policy commands:\n\
> ls-copp-list SWITCH\n\
> List all copp policies defined for control\n\
> protocols on SWITCH.\n\
> - lr-copp-add ROUTER PROTO METER\n\
> + lr-copp-add [UUID|NAME] ROUTER PROTO METER\n\
> Add a copp policy for PROTO packets on ROUTER\n\
> based on an existing METER.\n\
> lr-copp-del ROUTER [PROTO]\n\
> @@ -6278,6 +6278,9 @@ nbctl_pre_copp(struct ctl_context *ctx)
> {
> nbctl_pre_context(ctx);
> ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_meters);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_logical_switch);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_logical_router);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_name);
> ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_col_copp);
> ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_copp);
> }
> @@ -6285,9 +6288,31 @@ nbctl_pre_copp(struct ctl_context *ctx)
> static void
> nbctl_ls_copp_add(struct ctl_context *ctx)
> {
> - const char *ls_name = ctx->argv[1];
> - const char *proto_name = ctx->argv[2];
> - const char *meter = ctx->argv[3];
> + const struct nbrec_copp *copp = NULL;
> + const char *copp_name = NULL;
> + const char *proto_name;
> + const char *ls_name;
> + const char *meter;
> +
> + if (ctx->argc == 5) {
> + struct uuid uuid;
> + if (uuid_from_string(&uuid, ctx->argv[1])) {
> + copp = nbrec_copp_get_for_uuid(ctx->idl, &uuid);
> + if (!copp) {
> + ctx->error = xasprintf("copp %s not found.", ctx->argv[1]);
> + return;
> + }
> + } else {
> + copp_name = ctx->argv[1];
> + }
> + ls_name = ctx->argv[2];
> + proto_name = ctx->argv[3];
> + meter = ctx->argv[4];
> + } else {
> + ls_name = ctx->argv[1];
> + proto_name = ctx->argv[2];
> + meter = ctx->argv[3];
> + }
>
> char *error = copp_proto_validate(proto_name);
> if (error) {
> @@ -6302,9 +6327,23 @@ nbctl_ls_copp_add(struct ctl_context *ctx)
> return;
> }
>
> - const struct nbrec_copp *copp =
> - copp_meter_add(ctx, ls->copp, proto_name, meter);
> + if (!copp) {
> + copp = copp_meter_add(ctx, ls->copp, proto_name, meter);
> + }
> + if (copp_name) {
> + nbrec_copp_set_name(copp, copp_name);
> + }
> nbrec_logical_switch_set_copp(ls, copp);
> +
> + size_t n_logical_switch = copp->n_logical_switch + 1;
> + struct nbrec_logical_switch **ls_list =
> + xmalloc(n_logical_switch * sizeof *ls_list);
> + for (int i = 0; i < copp->n_logical_switch; i++) {
> + ls_list[i] = copp->logical_switch[i];
> + }
> + ls_list[copp->n_logical_switch] = (struct nbrec_logical_switch *)ls;
> + nbrec_copp_set_logical_switch(copp, ls_list, n_logical_switch);
> + free(ls_list);
> }
>
> static void
> @@ -6351,9 +6390,31 @@ nbctl_ls_copp_list(struct ctl_context *ctx)
> static void
> nbctl_lr_copp_add(struct ctl_context *ctx)
> {
> - const char *lr_name = ctx->argv[1];
> - const char *proto_name = ctx->argv[2];
> - const char *meter = ctx->argv[3];
> + const struct nbrec_copp *copp = NULL;
> + const char *copp_name = NULL;
> + const char *proto_name;
> + const char *lr_name;
> + const char *meter;
> +
> + if (ctx->argc == 5) {
> + struct uuid uuid;
> + if (uuid_from_string(&uuid, ctx->argv[1])) {
> + copp = nbrec_copp_get_for_uuid(ctx->idl, &uuid);
> + if (!copp) {
> + ctx->error = xasprintf("copp %s not found.", ctx->argv[1]);
> + return;
> + }
> + } else {
> + copp_name = ctx->argv[1];
> + }
> + lr_name = ctx->argv[2];
> + proto_name = ctx->argv[3];
> + meter = ctx->argv[4];
> + } else {
> + lr_name = ctx->argv[1];
> + proto_name = ctx->argv[2];
> + meter = ctx->argv[3];
> + }
>
> char *error = copp_proto_validate(proto_name);
> if (error) {
> @@ -6368,9 +6429,23 @@ nbctl_lr_copp_add(struct ctl_context *ctx)
> return;
> }
>
> - const struct nbrec_copp *copp =
> - copp_meter_add(ctx, lr->copp, proto_name, meter);
> + if (!copp) {
> + copp = copp_meter_add(ctx, lr->copp, proto_name, meter);
> + }
> + if (copp_name) {
> + nbrec_copp_set_name(copp, copp_name);
> + }
> nbrec_logical_router_set_copp(lr, copp);
> +
> + size_t n_logical_router = copp->n_logical_router + 1;
> + struct nbrec_logical_router **lr_list =
> + xmalloc(n_logical_router * sizeof *lr_list);
> + for (int i = 0; i < copp->n_logical_router; i++) {
> + lr_list[i] = copp->logical_router[i];
> + }
> + lr_list[copp->n_logical_router] = (struct nbrec_logical_router *)lr;
> + nbrec_copp_set_logical_router(copp, lr_list, n_logical_router);
> + free(lr_list);
> }
>
> static void
> @@ -7177,13 +7252,13 @@ static const struct ctl_command_syntax
> nbctl_commands[] = {
> NULL, "", RO },
>
> /* Control plane protection commands */
> - {"ls-copp-add", 3, 3, "SWITCH PROTO METER", nbctl_pre_copp,
> + {"ls-copp-add", 3, 4, "SWITCH PROTO METER", nbctl_pre_copp,
> nbctl_ls_copp_add, NULL, "", RW},
> {"ls-copp-del", 1, 2, "SWITCH [PROTO]", nbctl_pre_copp,
> nbctl_ls_copp_del, NULL, "", RW},
> {"ls-copp-list", 1, 1, "SWITCH", nbctl_pre_copp, nbctl_ls_copp_list,
> NULL, "", RO},
> - {"lr-copp-add", 3, 3, "ROUTER PROTO METER", nbctl_pre_copp,
> + {"lr-copp-add", 3, 4, "ROUTER PROTO METER", nbctl_pre_copp,
> nbctl_lr_copp_add, NULL, "", RW},
> {"lr-copp-del", 1, 2, "ROUTER [PROTO]", nbctl_pre_copp,
> nbctl_lr_copp_del, NULL, "", RW},
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
