[ovs-dev] [PATCH ovn] ovn-nbctl: Enhance lr-policy-add to set the options.

Numan Siddique numans at ovn.org
Tue Jun 23 14:30:46 UTC 2020


On Tue, Jun 23, 2020 at 7:09 PM Mark Michelson <mmichels at redhat.com> wrote:

> Hi Numan,
>
> I have a few comments in-line down below.
>
> On 6/23/20 7:43 AM, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > The commit [1] added a new column - 'options' to Logical_Router_Policy
> NB DB
> > table. This patch enhances the lr-policy-add command to set the options
> > as key=value pairs.
> >
> > [1] - a123ef0fb8fd("Support packet metadata marking for logical router
> policies.")
> >
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> >   tests/ovn-nbctl.at        |  6 +++---
> >   tests/ovn.at              |  8 ++------
> >   utilities/ovn-nbctl.8.xml | 11 ++++++++++-
> >   utilities/ovn-nbctl.c     | 38 ++++++++++++++++++++++++++++++++------
> >   4 files changed, 47 insertions(+), 16 deletions(-)
> >
> > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> > index 14de1a8bf..8180ccf9c 100644
> > --- a/tests/ovn-nbctl.at
> > +++ b/tests/ovn-nbctl.at
> > @@ -1590,7 +1590,7 @@ AT_CHECK([ovn-nbctl lr-add lr0])
> >
> >   dnl Add policies with allow and drop actions
> >   AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24"
> drop])
> > -AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24"
> allow])
> > +AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24"
> allow pkt_mark=100,foo=bar])
> >   AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24"
> allow])
> >   AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24"
> drop])
> >   AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip6.src == 2002::/64" drop])
> > @@ -1612,14 +1612,14 @@ Routing Policies
> >          101                              ip4.src == 2.1.1.0/24
>    allow
> >          101                              ip4.src == 2.1.2.0/24
>     drop
> >          101                               ip6.src == 2002::/64
>   drop
> > -       100                              ip4.src == 1.1.2.0/24
>  allow
> > +       100                              ip4.src == 1.1.2.0/24
>  allow               pkt_mark=100,foo=bar
> >   ])
> >
> >   dnl Delete all policies for given priority
> >   AT_CHECK([ovn-nbctl lr-policy-del lr0 101])
> >   AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl
> >   Routing Policies
> > -       100                              ip4.src == 1.1.2.0/24
>  allow
> > +       100                              ip4.src == 1.1.2.0/24
>  allow               pkt_mark=100,foo=bar
> >   ])
> >
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 1ff795294..00ed875ff 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -19993,20 +19993,16 @@ static_routes @lrt
> >   ovn-nbctl --wait=hv sync
> >
> >   # Add logical router policy and set pkt_mark on it.
> > -ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 10.0.0.3" allow
> > +ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 10.0.0.3" allow
> pkt_mark=100
> >   ovn-nbctl lr-policy-add lr0 1000 "ip4.src == 10.0.0.4" allow
> > -ovn-nbctl lr-policy-add lr0 900 "ip4.src == 10.0.0.5" reroute
> 172.168.0.200
> > +ovn-nbctl lr-policy-add lr0 900 "ip4.src == 10.0.0.5" reroute
> 172.168.0.200 pkt_mark=3
> >   ovn-nbctl lr-policy-add lr0 2001 "ip6.dst == bef0::5" reroute bef0::6
> >   ovn-nbctl lr-policy-add lr0 1001 "ip6" allow
> >
> > -
> >   pol1=$(ovn-nbctl --bare --columns _uuid find logical_router_policy
> priority=2000)
> > -pol3=$(ovn-nbctl --bare --columns _uuid find logical_router_policy
> priority=900)
> >   pol4=$(ovn-nbctl --bare --columns _uuid find logical_router_policy
> priority=2001)
> >   pol5=$(ovn-nbctl --bare --columns _uuid find logical_router_policy
> priority=1001)
> >
> > -ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=100
> > -ovn-nbctl set logical_router_policy $pol3 options:pkt_mark=3
> >   ovn-nbctl set logical_router_policy $pol4 options:pkt_mark=4
> >   ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=5
> >   ovn-nbctl --wait=hv sync
> > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> > index d265c7fcc..de86b70e6 100644
> > --- a/utilities/ovn-nbctl.8.xml
> > +++ b/utilities/ovn-nbctl.8.xml
> > @@ -721,7 +721,8 @@
> >
> >       <dl>
> >         <dt><code>lr-policy-add</code> <var>router</var>
> <var>priority</var>
> > -          <var>match</var> <var>action</var> [<var>nexthop</var>]</dt>
> > +          <var>match</var> <var>action</var> [<var>nexthop</var>]
> > +          [<var>options key=value]</var>] </dt>
> >         <dd>
> >           <p>
> >             Add Policy to <var>router</var> which provides a way to
> configure
> > @@ -732,6 +733,8 @@
> >             only when <var>action</var> is <var>reroute</var>. A policy
> is
> >             uniquely identified by <var>priority</var> and
> <var>match</var>.
> >             Multiple policies can have the same <var>priority</var>.
> > +          <var>options</var> sets the router policy options as
> key-value pair.
> > +          The supported option is : <code>pkt_mark</code>.
> >           </p>
> >
> >             <p>
> > @@ -743,6 +746,12 @@
> >             <code>lr-policy-add lr1 100 ip4.src == 192.168.100.0/24
> drop</code>.
> >             </p>
> >
> > +          <p>
> > +          <code>
> > +            lr-policy-add lr1 100 ip4.src == 192.168.100.0/24 allow
> > +            pkt_mark=100
> > +          </code>.
> > +          </p>
> >         </dd>
> >
> >         <dt><code>lr-policy-del</code> <var>router</var> [<var>{priority
> | uuid}
> > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> > index 6ccc7025d..ec269f252 100644
> > --- a/utilities/ovn-nbctl.c
> > +++ b/utilities/ovn-nbctl.c
> > @@ -694,7 +694,7 @@ Route commands:\n\
> >     lr-route-list ROUTER      print routes for ROUTER\n\
> >   \n\
> >   Policy commands:\n\
> > -  lr-policy-add ROUTER PRIORITY MATCH ACTION [NEXTHOP]\n\
> > +  lr-policy-add ROUTER PRIORITY MATCH ACTION [NEXTHOP] [OPTIONS
> KEY=VALUE]\n\
> >                               add a policy to router\n\
> >     lr-policy-del ROUTER [{PRIORITY | UUID} [MATCH]]\n\
> >                               remove policies from ROUTER\n\
> > @@ -3552,6 +3552,7 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
> >       const char *action = ctx->argv[4];
> >       char *next_hop = NULL;
> >
> > +    bool reroute = false;
> >       /* Validate action. */
> >       if (strcmp(action, "allow") && strcmp(action, "drop")
> >           && strcmp(action, "reroute")) {
> > @@ -3562,6 +3563,7 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
> >           if (ctx->argc < 6) {
> >               ctl_error(ctx, "Nexthop is required when action is
> reroute.");
> >           }
> > +        reroute = true;
> >       }
> >
> >       /* Check if same routing policy already exists.
> > @@ -3574,7 +3576,7 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
> >                         "logical router %s.", ctx->argv[1]);
> >           }
> >       }
> > -    if (ctx->argc == 6) {
> > +    if (ctx->argc >= 6 && reroute) {
>
> This can be simplified to:
>
>      if (reroute)
>
>
Ack. Done in v2.


> The block of code that sets reroute to true ensures that ctx->argc >= 6.
>
> >           next_hop = normalize_prefix_str(ctx->argv[5]);
> >           if (!next_hop) {
> >               ctl_error(ctx, "bad next hop argument: %s", ctx->argv[5]);
> > @@ -3586,9 +3588,23 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
> >       nbrec_logical_router_policy_set_priority(policy, priority);
> >       nbrec_logical_router_policy_set_match(policy, ctx->argv[3]);
> >       nbrec_logical_router_policy_set_action(policy, action);
> > -    if (ctx->argc == 6) {
> > +    if (ctx->argc >= 6 && reroute) {
>
> This can also be simplified to
>

Ack. Done in v2.


>
>      if (reroute)
>
> >           nbrec_logical_router_policy_set_nexthop(policy, next_hop);
> >       }
> > +
> > +    /* Parse the options. */
> > +    struct smap options = SMAP_INITIALIZER(&options);
> > +    for (size_t i = reroute ? 6 : 5; i < ctx->argc; i++) {
> > +        char *key, *value;
> > +        value = xstrdup(ctx->argv[i]);
> > +        key = strsep(&value, "=");
> > +        if (value) {
> > +            smap_add(&options, key, value); > +        }
>
> If no value is specified, then should we print an error/warning? Should
> we exit without inserting the logical router policy?
>

Yeah. I agree. v2 exits if no value specified.


>
> > +        free(key);
> > +    }
> > +    nbrec_logical_router_policy_set_options(policy, &options);
>
> Call smap_destroy(&options) here.
>

Ack. Done in v2.



>
> > +
> >       nbrec_logical_router_verify_policies(lr);
> >       struct nbrec_logical_router_policy **new_policies
> >           = xmalloc(sizeof *new_policies * (lr->n_policies + 1));
> > @@ -3716,6 +3732,16 @@ print_routing_policy(const struct
> nbrec_logical_router_policy *policy,
> >           ds_put_format(s, "%10"PRId64" %50s %15s", policy->priority,
> >                         policy->match, policy->action);
> >       }
> > +
> > +    if (!smap_is_empty(&policy->options)) {
> > +        ds_put_format(s, "%15s", "");
> > +        struct smap_node *node;
> > +        SMAP_FOR_EACH (node, &policy->options) {
> > +            ds_put_format(s, "%s=%s,", node->key, node->value);
> > +        }
> > +        ds_chomp(s, ',');
> > +    }
> > +
> >       ds_put_char(s, '\n');
> >   }
> >
> > @@ -3731,7 +3757,7 @@ nbctl_lr_policy_list(struct ctl_context *ctx)
> >           return;
> >       }
> >       policies = xmalloc(sizeof *policies * lr->n_policies);
> > -     for (int i = 0; i < lr->n_policies; i++) {
> > +    for (int i = 0; i < lr->n_policies; i++) {
> >           const struct nbrec_logical_router_policy *policy
> >               = lr->policies[i];
> >           policies[n_policies].priority = policy->priority;
> > @@ -6273,8 +6299,8 @@ static const struct ctl_command_syntax
> nbctl_commands[] = {
> >         "", RO },
> >
> >       /* Policy commands */
> > -    { "lr-policy-add", 4, 5, "ROUTER PRIORITY MATCH ACTION [NEXTHOP]",
> NULL,
> > -        nbctl_lr_policy_add, NULL, "", RW },
> > +    { "lr-policy-add", 4, INT_MAX, "ROUTER PRIORITY MATCH ACTION
> [NEXTHOP]",
>
> Is there any particular reason you didn't include the new OPTIONS
> argument here?
>
I think I missed out here. Done in v2.

Thanks for the review.

I just submitted v2 addressing the comments.

Thanks
Numan


>
> > +     NULL, nbctl_lr_policy_add, NULL, "", RW },
> >       { "lr-policy-del", 1, 3, "ROUTER [{PRIORITY | UUID} [MATCH]]",
> NULL,
> >           nbctl_lr_policy_del, NULL, "", RW },
> >       { "lr-policy-list", 1, 1, "ROUTER", NULL, nbctl_lr_policy_list,
> NULL,
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list