| Message ID | f89f48e658a36a91b9392fd62a02aadc5137409d.1642674977.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ovn-nbctl: add the capability to specify CoPP UUID or CoPP name | 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 |
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 <lorenzo.bianconi@redhat.com> > --- 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}, >
> 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 <lorenzo.bianconi@redhat.com> > > --- > > 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}, > > >
On 1/20/22 15:16, Lorenzo Bianconi wrote: >> 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 <lorenzo.bianconi@redhat.com> >>> --- >> >> 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? This still doesn't cover the case when entries already existed in the database before the upgrade. I don't think there's a backwards compatible way of doing this and I don't really think it's useful to add a "name" field that's not used as index. However, for this feature in particular, because it's rather new and not used (as far as I know) by any CMS, I think we can probably break compatibility and add the missing index. I'm curious about the maintainers' opinion on this too. Regards, Dumitru
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"}, + "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},
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 <lorenzo.bianconi@redhat.com> --- 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(-)