diff mbox

[ovs-dev,v2,03/11] ofp-actions: Add clone action.

Message ID 20161216222537.2221-4-blp@ovn.org
State Superseded
Headers show

Commit Message

Ben Pfaff Dec. 16, 2016, 10:25 p.m. UTC
From: William Tu <u9012063@gmail.com>

This patch adds OpenFlow clone action with syntax as below:
"clone([action][,action...])".  The clone() action makes a copy of the
current packet and executes the list of actions against the packet,
without affecting the packet after the "clone(...)" action.  In other
word, the packet before the clone() and after the clone() is the same,
no matter what actions executed inside the clone().

Use case 1:
Set different fields and output to different ports without unset
actions=
  clone(mod_dl_src:<mac1>, output:1), clone(mod_dl_dst:<mac2>, output:2), output:3
Since each clone() has independent packet, output:1 has only dl_src modified,
output:2 has only dl_dst modified, output:3 has original packet.

Similar to case1
actions=
  push_vlan(...), output:2, pop_vlan, push_vlan(...), output:3
can be changed to
actions=
  clone(push_vlan(...), output:2),clone(push_vlan(...), output:3)
without having to add pop_vlan.

case 2: resubmit to another table without worrying packet being modified
  actions=clone(resubmit(1,2)), ...

Signed-off-by: William Tu <u9012063@gmail.com>
[blp@ovn.org revised this to omit the "sample" action]
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 include/openvswitch/ofp-actions.h |  5 ++-
 lib/ofp-actions.c                 | 91 ++++++++++++++++++++++++++++++++++++++-
 ofproto/ofproto-dpif-xlate.c      | 14 ++++++
 tests/ofp-actions.at              |  5 +++
 tests/ofproto-dpif.at             | 19 ++++++++
 tests/system-traffic.at           | 29 +++++++++++++
 6 files changed, 160 insertions(+), 3 deletions(-)

Comments

William Tu Dec. 17, 2016, 4:19 p.m. UTC | #1
Thanks for the patch, I have some comments.

> +static char * OVS_WARN_UNUSED_RESULT
> +parse_CLONE(char *arg, struct ofpbuf *ofpacts,
> +             enum ofputil_protocol *usable_protocols)
> +{
> +
> +    const size_t ct_offset = ofpacts_pull(ofpacts);
I will s/ct_offset/clone_offset/g

> +    struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts);
> +    char *error;
> +
> +    ofpbuf_pull(ofpacts, sizeof *clone);
> +    error = ofpacts_parse_copy(arg, ofpacts, usable_protocols,
> +                               false, OFPACT_CLONE);
should we set to "true" to allow instructions?

> @@ -7279,6 +7363,10 @@ ofpacts_verify_nested(const struct ofpact *a, enum ofpact_type outer_action)
>                  return OFPERR_OFPBAC_BAD_ARGUMENT;
>              }
>          }
> +        if (outer_action == OFPACT_CLONE) {
> +                /* add some constraints. */
> +                return 0;
> +        }
I will remove the above, since we no longer use sample.

> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6431,6 +6431,25 @@ AT_CHECK([ovs-vsctl destroy Flow_Sample_Collector_Set 1], [0], [ignore])
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> +dnl OpenFlow Clone action using Sample datapath action
I will remove "Sample"

> +AT_SETUP([ofproto-dpif - clone action])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 3 4
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1, actions=clone(set_field:192.168.3.3->ip_src),clone(set_field:192.168.4.4->ip_dst,output:2),clone(mod_dl_src:80:81:81:81:81:81,set_field:192.168.5.5->ip_dst,output:3),output:4
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
> +
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
> +])
LGTM

Thanks for the patch. I will submit v3.

Regards,
William
Ben Pfaff Dec. 18, 2016, 8:01 a.m. UTC | #2
On Sat, Dec 17, 2016 at 08:19:34AM -0800, William Tu wrote:
> Thanks for the patch, I have some comments.
> 
> > +static char * OVS_WARN_UNUSED_RESULT
> > +parse_CLONE(char *arg, struct ofpbuf *ofpacts,
> > +             enum ofputil_protocol *usable_protocols)
> > +{
> > +
> > +    const size_t ct_offset = ofpacts_pull(ofpacts);
> I will s/ct_offset/clone_offset/g
> 
> > +    struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts);
> > +    char *error;
> > +
> > +    ofpbuf_pull(ofpacts, sizeof *clone);
> > +    error = ofpacts_parse_copy(arg, ofpacts, usable_protocols,
> > +                               false, OFPACT_CLONE);
> should we set to "true" to allow instructions?
> 
> > @@ -7279,6 +7363,10 @@ ofpacts_verify_nested(const struct ofpact *a, enum ofpact_type outer_action)
> >                  return OFPERR_OFPBAC_BAD_ARGUMENT;
> >              }
> >          }
> > +        if (outer_action == OFPACT_CLONE) {
> > +                /* add some constraints. */
> > +                return 0;
> > +        }
> I will remove the above, since we no longer use sample.
> 
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -6431,6 +6431,25 @@ AT_CHECK([ovs-vsctl destroy Flow_Sample_Collector_Set 1], [0], [ignore])
> >  OVS_VSWITCHD_STOP
> >  AT_CLEANUP
> >
> > +dnl OpenFlow Clone action using Sample datapath action
> I will remove "Sample"
> 
> > +AT_SETUP([ofproto-dpif - clone action])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2 3 4
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1, actions=clone(set_field:192.168.3.3->ip_src),clone(set_field:192.168.4.4->ip_dst,output:2),clone(mod_dl_src:80:81:81:81:81:81,set_field:192.168.5.5->ip_dst,output:3),output:4
> > +])
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
> > +
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
> > +])
> LGTM
> 
> Thanks for the patch. I will submit v3.

Thanks for the comments.  No need to submit a v3, I'll apply these
directly to the version in my series.
diff mbox

Patch

diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index ecafbea..870e16f 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -109,6 +109,7 @@ 
     OFPACT(CT,              ofpact_conntrack,   ofpact, "ct")           \
     OFPACT(NAT,             ofpact_nat,         ofpact, "nat")          \
     OFPACT(OUTPUT_TRUNC,    ofpact_output_trunc,ofpact, "output_trunc") \
+    OFPACT(CLONE,           ofpact_nest,        actions, "clone")       \
                                                                         \
     /* Debugging actions.                                               \
      *                                                                  \
@@ -532,9 +533,9 @@  struct ofpact_meter {
     uint32_t meter_id;
 };
 
-/* OFPACT_WRITE_ACTIONS.
+/* OFPACT_WRITE_ACTIONS, OFPACT_CLONE.
  *
- * Used for OFPIT11_WRITE_ACTIONS. */
+ * Used for OFPIT11_WRITE_ACTIONS, NXAST_CLONE. */
 struct ofpact_nest {
     OFPACT_PADDED_MEMBERS(struct ofpact ofpact;);
     struct ofpact actions[];
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 09bd8c2..26bb3b4 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -325,6 +325,9 @@  enum ofp_raw_action_type {
     /* NX1.0+(39): struct nx_action_output_trunc. */
     NXAST_RAW_OUTPUT_TRUNC,
 
+    /* NX1.0+(42): struct ext_action_header, ... */
+    NXAST_RAW_CLONE,
+
 /* ## ------------------ ## */
 /* ## Debugging actions. ## */
 /* ## ------------------ ## */
@@ -405,6 +408,7 @@  ofpact_next_flattened(const struct ofpact *ofpact)
     switch (ofpact->type) {
     case OFPACT_OUTPUT:
     case OFPACT_GROUP:
+    case OFPACT_CLONE:
     case OFPACT_CONTROLLER:
     case OFPACT_ENQUEUE:
     case OFPACT_OUTPUT_REG:
@@ -4798,6 +4802,80 @@  format_UNROLL_XLATE(const struct ofpact_unroll_xlate *a, struct ds *s)
                   colors.paren,   colors.end);
 }
 
+/* The NXAST_CLONE action is "struct ext_action_header", followed by zero or
+ * more embedded OpenFlow actions. */
+
+static enum ofperr
+decode_NXAST_RAW_CLONE(const struct ext_action_header *eah,
+                        enum ofp_version ofp_version,
+                        struct ofpbuf *out)
+{
+    int error;
+    struct ofpbuf openflow;
+    const size_t clone_offset = ofpacts_pull(out);
+    struct ofpact_nest *clone = ofpact_put_CLONE(out);
+
+    /* decode action list */
+    ofpbuf_pull(out, sizeof(*clone));
+    openflow = ofpbuf_const_initializer(
+                    eah + 1, ntohs(eah->len) - sizeof *eah);
+    error = ofpacts_pull_openflow_actions__(&openflow, openflow.size,
+                                            ofp_version,
+                                            1u << OVSINST_OFPIT11_APPLY_ACTIONS,
+                                            out, OFPACT_CLONE);
+    clone = ofpbuf_push_uninit(out, sizeof *clone);
+    out->header = &clone->ofpact;
+    ofpact_finish_CLONE(out, &clone);
+    ofpbuf_push_uninit(out, clone_offset);
+    return error;
+}
+
+static void
+encode_CLONE(const struct ofpact_nest *clone,
+              enum ofp_version ofp_version, struct ofpbuf *out)
+{
+    size_t len;
+    const size_t ofs = out->size;
+    struct ext_action_header *eah;
+
+    eah = put_NXAST_CLONE(out);
+    len = ofpacts_put_openflow_actions(clone->actions,
+                                       ofpact_nest_get_action_len(clone),
+                                       out, ofp_version);
+    len += sizeof *eah;
+    eah = ofpbuf_at(out, ofs, sizeof *eah);
+    eah->len = htons(len);
+}
+
+static char * OVS_WARN_UNUSED_RESULT
+parse_CLONE(char *arg, struct ofpbuf *ofpacts,
+             enum ofputil_protocol *usable_protocols)
+{
+
+    const size_t ct_offset = ofpacts_pull(ofpacts);
+    struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts);
+    char *error;
+
+    ofpbuf_pull(ofpacts, sizeof *clone);
+    error = ofpacts_parse_copy(arg, ofpacts, usable_protocols,
+                               false, OFPACT_CLONE);
+    /* header points to the action list */
+    ofpacts->header = ofpbuf_push_uninit(ofpacts, sizeof *clone);
+    clone = ofpacts->header;
+
+    ofpact_finish_CLONE(ofpacts, &clone);
+    ofpbuf_push_uninit(ofpacts, ct_offset);
+    return error;
+}
+
+static void
+format_CLONE(const struct ofpact_nest *a, struct ds *s)
+{
+    ds_put_format(s, "%sclone(%s", colors.paren, colors.end);
+    ofpacts_format(a->actions, ofpact_nest_get_action_len(a), s);
+    ds_put_format(s, "%s)%s", colors.paren, colors.end);
+}
+
 /* Action structure for NXAST_SAMPLE.
  *
  * Samples matching packets with the given probability and sends them
@@ -6209,6 +6287,7 @@  ofpact_is_set_or_move_action(const struct ofpact *a)
     case OFPACT_BUNDLE:
     case OFPACT_CLEAR_ACTIONS:
     case OFPACT_CT:
+    case OFPACT_CLONE:
     case OFPACT_NAT:
     case OFPACT_CONTROLLER:
     case OFPACT_DEC_MPLS_TTL:
@@ -6285,6 +6364,7 @@  ofpact_is_allowed_in_actions_set(const struct ofpact *a)
      * the specification.  Thus the order in which they should be applied
      * in the action set is undefined. */
     case OFPACT_BUNDLE:
+    case OFPACT_CLONE:
     case OFPACT_CONTROLLER:
     case OFPACT_CT:
     case OFPACT_NAT:
@@ -6477,6 +6557,7 @@  ovs_instruction_type_from_ofpact_type(enum ofpact_type type)
         return OVSINST_OFPIT11_GOTO_TABLE;
     case OFPACT_OUTPUT:
     case OFPACT_GROUP:
+    case OFPACT_CLONE:
     case OFPACT_CONTROLLER:
     case OFPACT_ENQUEUE:
     case OFPACT_OUTPUT_REG:
@@ -7077,6 +7158,9 @@  ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
     case OFPACT_SAMPLE:
         return 0;
 
+    case OFPACT_CLONE:
+        return 0;
+
     case OFPACT_CT: {
         struct ofpact_conntrack *oc = ofpact_get_CT(a);
 
@@ -7268,7 +7352,7 @@  ofpacts_verify_nested(const struct ofpact *a, enum ofpact_type outer_action)
 
     if (outer_action) {
         ovs_assert(outer_action == OFPACT_WRITE_ACTIONS
-                   || outer_action == OFPACT_CT);
+                   || outer_action == OFPACT_CT || outer_action == OFPACT_CLONE);
 
         if (outer_action == OFPACT_CT) {
             if (!field) {
@@ -7279,6 +7363,10 @@  ofpacts_verify_nested(const struct ofpact *a, enum ofpact_type outer_action)
                 return OFPERR_OFPBAC_BAD_ARGUMENT;
             }
         }
+        if (outer_action == OFPACT_CLONE) {
+                /* add some constraints. */
+                return 0;
+        }
     }
 
     return 0;
@@ -7632,6 +7720,7 @@  ofpact_outputs_to_port(const struct ofpact *ofpact, ofp_port_t port)
     case OFPACT_POP_MPLS:
     case OFPACT_SAMPLE:
     case OFPACT_CLEAR_ACTIONS:
+    case OFPACT_CLONE:
     case OFPACT_WRITE_ACTIONS:
     case OFPACT_GOTO_TABLE:
     case OFPACT_METER:
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d0f9a33..eec1dae 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4290,6 +4290,14 @@  xlate_sample_action(struct xlate_ctx *ctx,
                           tunnel_out_port, false);
 }
 
+static void
+compose_clone_action(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
+{
+    struct flow old_flow = ctx->xin->flow;
+    do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
+    ctx->xin->flow = old_flow;
+}
+
 static bool
 may_receive(const struct xport *xport, struct xlate_ctx *ctx)
 {
@@ -4448,6 +4456,7 @@  freeze_unroll_actions(const struct ofpact *a, const struct ofpact *end,
         case OFPACT_WRITE_ACTIONS:
         case OFPACT_METER:
         case OFPACT_SAMPLE:
+        case OFPACT_CLONE:
         case OFPACT_DEBUG_RECIRC:
         case OFPACT_CT:
         case OFPACT_NAT:
@@ -4696,6 +4705,7 @@  recirc_for_mpls(const struct ofpact *a, struct xlate_ctx *ctx)
     case OFPACT_NOTE:
     case OFPACT_EXIT:
     case OFPACT_SAMPLE:
+    case OFPACT_CLONE:
     case OFPACT_UNROLL_XLATE:
     case OFPACT_CT:
     case OFPACT_NAT:
@@ -5055,6 +5065,10 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             xlate_sample_action(ctx, ofpact_get_SAMPLE(a));
             break;
 
+        case OFPACT_CLONE:
+            compose_clone_action(ctx, ofpact_get_CLONE(a));
+            break;
+
         case OFPACT_CT:
             compose_conntrack_action(ctx, ofpact_get_CT(a));
             break;
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 25ee06c..db73854 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -247,6 +247,11 @@  fe800000 00000000 020c 29ff fe88 a18b dnl
 # actions=output(port=1,max_len=100)
 ffff 0010 00002320 0027 0001 00000064
 
+# actions=clone(mod_vlan_vid:5,output:10)
+ffff 0020 00002320 002a 000000000000 dnl
+0001 0008 0005 0000 dnl
+0000 0008 000a 0000
+
 # actions=group:5
 ffff 0010 00002320 0028 0000 00000005
 
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 07a1675..caa1e84 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6431,6 +6431,25 @@  AT_CHECK([ovs-vsctl destroy Flow_Sample_Collector_Set 1], [0], [ignore])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+dnl OpenFlow Clone action using Sample datapath action
+AT_SETUP([ofproto-dpif - clone action])
+OVS_VSWITCHD_START
+add_of_ports br0 1 2 3 4
+
+AT_DATA([flows.txt], [dnl
+in_port=1, actions=clone(set_field:192.168.3.3->ip_src),clone(set_field:192.168.4.4->ip_dst,output:2),clone(mod_dl_src:80:81:81:81:81:81,set_field:192.168.5.5->ip_dst,output:3),output:4
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
+
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 dnl Flow based IPFIX statistics check
 AT_SETUP([ofproto-dpif - Flow IPFIX statistics check])
 OVS_VSWITCHD_START
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 801dfe3..31e5a16 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -337,6 +337,35 @@  NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.1.1.100 | FORMAT_PI
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([datapath - clone action])
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1, at_ns2)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24")
+
+AT_CHECK([ovs-vsctl -- set interface ovs-p0 ofport_request=1])
+AT_CHECK([ovs-vsctl -- set interface ovs-p1 ofport_request=2])
+AT_CHECK([ovs-vsctl -- set interface ovs-p2 ofport_request=3])
+
+dnl verify that the clone(...) won't affect the original packet, so ping still works OK
+dnl without 'output' in 'clone()'
+AT_CHECK([ovs-ofctl add-flow br0 "in_port=1,actions=clone(mod_dl_dst(50:54:00:00:00:0a),set_field:192.168.3.3->ip_dst), output:2"])
+dnl with 'output' in 'clone()'
+AT_CHECK([ovs-ofctl add-flow br0 "in_port=2,actions=clone(mod_dl_dst(50:54:00:00:00:0b),set_field:192.168.4.4->ip_dst, output:3), output:1"])
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], [0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py]], [http0.pid])
+NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o wget0.log])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([datapath - basic truncate action])
 OVS_TRAFFIC_VSWITCHD_START()
 AT_CHECK([ovs-ofctl del-flows br0])