> 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,
Hi Dumitru,
thx for the review.
>
> > 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?
if name is used as index we will need to make it mandatory. One possible
solution
would be to create a random string when not provided. Any downside with this
approach?
Regards,
Lorenzo
>
> 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
