diff mbox series

[ovs-dev,v3] northd: Add qos packet marking.

Message ID a3602ed02c7b443426785fe1b18b2afe8ebdcf4c.1706888205.git.lorenzo.bianconi@redhat.com
State Accepted
Headers show
Series [ovs-dev,v3] northd: Add qos packet marking. | expand

Checks

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

Commit Message

Lorenzo Bianconi Feb. 2, 2024, 3:41 p.m. UTC
Add the capbility to mark (through pkt.mark) incoming/outgoing packets
in logical_switch datapath according to user configured QoS rule.

Co-developed-by: Dumitru Ceara <dceara@redhat.com>
Reported-at: https://issues.redhat.com/browse/FDP-42
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v2:
- set max dscp/mark value in ovn-nb.ovsschema
- add unit-test for new dscp/mark boundaries
Changes since v1:
- move qos packet mark action in QOS_MARK ls {ingress/egress} stage
---
 NEWS                      |  2 +
 northd/northd.c           | 33 +++++++++++++---
 northd/ovn-northd.8.xml   |  6 +++
 ovn-nb.ovsschema          |  8 ++--
 ovn-nb.xml                | 12 +++++-
 tests/ovn-nbctl.at        |  8 +++-
 tests/ovn-northd.at       | 81 ++++++++++++++++++++++++++++++++++++++
 tests/ovn.at              | 83 +++++++++++++++++++++++++++++++++++++++
 utilities/ovn-nbctl.8.xml |  5 ++-
 utilities/ovn-nbctl.c     | 27 ++++++++++---
 10 files changed, 245 insertions(+), 20 deletions(-)

Comments

Dumitru Ceara Feb. 2, 2024, 4:03 p.m. UTC | #1
On 2/2/24 16:41, Lorenzo Bianconi wrote:
> Add the capbility to mark (through pkt.mark) incoming/outgoing packets
> in logical_switch datapath according to user configured QoS rule.
> 
> Co-developed-by: Dumitru Ceara <dceara@redhat.com>
> Reported-at: https://issues.redhat.com/browse/FDP-42
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v2:
> - set max dscp/mark value in ovn-nb.ovsschema
> - add unit-test for new dscp/mark boundaries
> Changes since v1:
> - move qos packet mark action in QOS_MARK ls {ingress/egress} stage
> ---
>  NEWS                      |  2 +
>  northd/northd.c           | 33 +++++++++++++---
>  northd/ovn-northd.8.xml   |  6 +++
>  ovn-nb.ovsschema          |  8 ++--
>  ovn-nb.xml                | 12 +++++-
>  tests/ovn-nbctl.at        |  8 +++-
>  tests/ovn-northd.at       | 81 ++++++++++++++++++++++++++++++++++++++
>  tests/ovn.at              | 83 +++++++++++++++++++++++++++++++++++++++
>  utilities/ovn-nbctl.8.xml |  5 ++-
>  utilities/ovn-nbctl.c     | 27 ++++++++++---
>  10 files changed, 245 insertions(+), 20 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 6553bd078..a8beb09fb 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -18,6 +18,8 @@ Post v23.09.0
>    - Support selecting encapsulation IP based on the source/destination VIF's
>      settting. See ovn-controller(8) 'external_ids:ovn-encap-ip' for more
>      details.
> +  - Add the capability to mark (through pkt.mark) incoming/outgoing packets
> +    in the logical switch datapath according to user configured QoS rule.
>  
>  OVN v23.09.0 - 15 Sep 2023
>  --------------------------
> diff --git a/northd/northd.c b/northd/northd.c
> index d2091d4bc..a77919af3 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -8363,6 +8363,8 @@ build_acls(struct ovn_datapath *od, const struct chassis_features *features,
>      ds_destroy(&actions);
>  }
>  
> +#define QOS_MAX_DSCP 63
> +
>  static void
>  build_qos(struct ovn_datapath *od, struct hmap *lflows) {
>      struct ds action = DS_EMPTY_INITIALIZER;
> @@ -8376,21 +8378,40 @@ build_qos(struct ovn_datapath *od, struct hmap *lflows) {
>          struct nbrec_qos *qos = od->nbs->qos_rules[i];
>          bool ingress = !strcmp(qos->direction, "from-lport") ? true :false;
>          enum ovn_stage stage = ingress ? S_SWITCH_IN_QOS_MARK : S_SWITCH_OUT_QOS_MARK;
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>          int64_t rate = 0;
>          int64_t burst = 0;
>  
> +        ds_clear(&action);
>          for (size_t j = 0; j < qos->n_action; j++) {
> +            if (strcmp(qos->key_action[j], "dscp") &&
> +                strcmp(qos->key_action[j], "mark")) {
> +                continue;
> +            }
> +

This check seems redundant we recheck the qos->key_action[j] just below.
 Would it be possible to remove it when applying the patch to the main
branch (if the patch is accepted)?

Thanks,
Dumitru
Mark Michelson Feb. 2, 2024, 5:09 p.m. UTC | #2
Thanks a bunch Lorenzo.

Acked-by: Mark Michelson <mmichels@redhat.com>

I plan to merge this in a few hours, giving a bit of time in case others 
want to review this. When I merge, I'll fold in Dumitru's suggestion.

On 2/2/24 11:03, Dumitru Ceara wrote:
> On 2/2/24 16:41, Lorenzo Bianconi wrote:
>> Add the capbility to mark (through pkt.mark) incoming/outgoing packets
>> in logical_switch datapath according to user configured QoS rule.
>>
>> Co-developed-by: Dumitru Ceara <dceara@redhat.com>
>> Reported-at: https://issues.redhat.com/browse/FDP-42
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> ---
>> Changes since v2:
>> - set max dscp/mark value in ovn-nb.ovsschema
>> - add unit-test for new dscp/mark boundaries
>> Changes since v1:
>> - move qos packet mark action in QOS_MARK ls {ingress/egress} stage
>> ---
>>   NEWS                      |  2 +
>>   northd/northd.c           | 33 +++++++++++++---
>>   northd/ovn-northd.8.xml   |  6 +++
>>   ovn-nb.ovsschema          |  8 ++--
>>   ovn-nb.xml                | 12 +++++-
>>   tests/ovn-nbctl.at        |  8 +++-
>>   tests/ovn-northd.at       | 81 ++++++++++++++++++++++++++++++++++++++
>>   tests/ovn.at              | 83 +++++++++++++++++++++++++++++++++++++++
>>   utilities/ovn-nbctl.8.xml |  5 ++-
>>   utilities/ovn-nbctl.c     | 27 ++++++++++---
>>   10 files changed, 245 insertions(+), 20 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 6553bd078..a8beb09fb 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -18,6 +18,8 @@ Post v23.09.0
>>     - Support selecting encapsulation IP based on the source/destination VIF's
>>       settting. See ovn-controller(8) 'external_ids:ovn-encap-ip' for more
>>       details.
>> +  - Add the capability to mark (through pkt.mark) incoming/outgoing packets
>> +    in the logical switch datapath according to user configured QoS rule.
>>   
>>   OVN v23.09.0 - 15 Sep 2023
>>   --------------------------
>> diff --git a/northd/northd.c b/northd/northd.c
>> index d2091d4bc..a77919af3 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -8363,6 +8363,8 @@ build_acls(struct ovn_datapath *od, const struct chassis_features *features,
>>       ds_destroy(&actions);
>>   }
>>   
>> +#define QOS_MAX_DSCP 63
>> +
>>   static void
>>   build_qos(struct ovn_datapath *od, struct hmap *lflows) {
>>       struct ds action = DS_EMPTY_INITIALIZER;
>> @@ -8376,21 +8378,40 @@ build_qos(struct ovn_datapath *od, struct hmap *lflows) {
>>           struct nbrec_qos *qos = od->nbs->qos_rules[i];
>>           bool ingress = !strcmp(qos->direction, "from-lport") ? true :false;
>>           enum ovn_stage stage = ingress ? S_SWITCH_IN_QOS_MARK : S_SWITCH_OUT_QOS_MARK;
>> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>>           int64_t rate = 0;
>>           int64_t burst = 0;
>>   
>> +        ds_clear(&action);
>>           for (size_t j = 0; j < qos->n_action; j++) {
>> +            if (strcmp(qos->key_action[j], "dscp") &&
>> +                strcmp(qos->key_action[j], "mark")) {
>> +                continue;
>> +            }
>> +
> 
> This check seems redundant we recheck the qos->key_action[j] just below.
>   Would it be possible to remove it when applying the patch to the main
> branch (if the patch is accepted)?
> 
> Thanks,
> Dumitru
>
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 6553bd078..a8beb09fb 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,8 @@  Post v23.09.0
   - Support selecting encapsulation IP based on the source/destination VIF's
     settting. See ovn-controller(8) 'external_ids:ovn-encap-ip' for more
     details.
+  - Add the capability to mark (through pkt.mark) incoming/outgoing packets
+    in the logical switch datapath according to user configured QoS rule.
 
 OVN v23.09.0 - 15 Sep 2023
 --------------------------
diff --git a/northd/northd.c b/northd/northd.c
index d2091d4bc..a77919af3 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8363,6 +8363,8 @@  build_acls(struct ovn_datapath *od, const struct chassis_features *features,
     ds_destroy(&actions);
 }
 
+#define QOS_MAX_DSCP 63
+
 static void
 build_qos(struct ovn_datapath *od, struct hmap *lflows) {
     struct ds action = DS_EMPTY_INITIALIZER;
@@ -8376,21 +8378,40 @@  build_qos(struct ovn_datapath *od, struct hmap *lflows) {
         struct nbrec_qos *qos = od->nbs->qos_rules[i];
         bool ingress = !strcmp(qos->direction, "from-lport") ? true :false;
         enum ovn_stage stage = ingress ? S_SWITCH_IN_QOS_MARK : S_SWITCH_OUT_QOS_MARK;
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
         int64_t rate = 0;
         int64_t burst = 0;
 
+        ds_clear(&action);
         for (size_t j = 0; j < qos->n_action; j++) {
+            if (strcmp(qos->key_action[j], "dscp") &&
+                strcmp(qos->key_action[j], "mark")) {
+                continue;
+            }
+
             if (!strcmp(qos->key_action[j], "dscp")) {
-                ds_clear(&action);
-                ds_put_format(&action, "ip.dscp = %"PRId64"; next;",
+                if (qos->value_action[j] > QOS_MAX_DSCP) {
+                    VLOG_WARN_RL(&rl, "Bad 'dscp' value %"PRId64" in qos "
+                                      UUID_FMT, qos->value_action[j],
+                                      UUID_ARGS(&qos->header_.uuid));
+                    continue;
+                }
+
+                ds_put_format(&action, "ip.dscp = %"PRId64"; ",
+                              qos->value_action[j]);
+            } else if (!strcmp(qos->key_action[j], "mark")) {
+                ds_put_format(&action, "pkt.mark = %"PRId64"; ",
                               qos->value_action[j]);
-                ovn_lflow_add_with_hint(lflows, od, stage,
-                                        qos->priority,
-                                        qos->match, ds_cstr(&action),
-                                        &qos->header_);
             }
         }
 
+        if (action.length) {
+            ds_put_cstr(&action, "next;");
+            ovn_lflow_add_with_hint(lflows, od, stage, qos->priority,
+                                    qos->match, ds_cstr(&action),
+                                    &qos->header_);
+        }
+
         for (size_t n = 0; n < qos->n_bandwidth; n++) {
             if (!strcmp(qos->key_bandwidth[n], "rate")) {
                 rate = qos->value_bandwidth[n];
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 068d47e1a..9583abeff 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -915,6 +915,12 @@ 
         QoS table.
       </li>
 
+      <li>
+        For every qos_rules entry in a logical switch with packet marking
+        enabled, a flow will be added at the priority mentioned in the
+        QoS table.
+      </li>
+
       <li>
         One priority-0 fallback flow that matches all packets and advances to
         the next table.
diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
index b2e0993e0..12ddce3f6 100644
--- a/ovn-nb.ovsschema
+++ b/ovn-nb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Northbound",
-    "version": "7.2.0",
-    "cksum": "1069338687 34162",
+    "version": "7.3.0",
+    "cksum": "4237284067 34178",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -293,10 +293,10 @@ 
                                             "enum": ["set", ["from-lport", "to-lport"]]}}},
                 "match": {"type": "string"},
                 "action": {"type": {"key": {"type": "string",
-                                            "enum": ["set", ["dscp"]]},
+                                            "enum": ["set", ["dscp", "mark"]]},
                                     "value": {"type": "integer",
                                               "minInteger": 0,
-                                              "maxInteger": 63},
+                                              "maxInteger": 4294967295},
                                     "min": 0, "max": "unlimited"}},
                 "bandwidth": {"type": {"key": {"type": "string",
                                                "enum": ["set", ["rate",
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 765ffcf2e..5b593f6a7 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -2808,12 +2808,22 @@  or
     </column>
 
     <column name="action">
-      <p>When specified, matching flows will have DSCP marking applied.</p>
+      <p>
+        When <code>dscp</code> action is specified, matching flows will have
+        have DSCP marking applied.
+        When <code>mark</code> action is specified, matching flows will have
+        packet marking applied.
+      </p>
+
       <ul>
         <li>
           <code>dscp</code>: The value of this action should be in the
           range of 0 to 63 (inclusive).
         </li>
+        <li>
+           <code>mark</code>: The value of this action should be a positive
+           integer.
+        </li>
       </ul>
     </column>
 
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 7f37d7716..b11e4ec6b 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -316,6 +316,8 @@  AT_CHECK([ovn-nbctl qos-add ls0 from-lport 400 tcp dscp=0 rate=300 burst=3000])
 AT_CHECK([ovn-nbctl qos-add ls0 to-lport 300 tcp dscp=48])
 AT_CHECK([ovn-nbctl qos-add ls0 to-lport 200 ip rate=101])
 AT_CHECK([ovn-nbctl qos-add ls0 to-lport 100 ip4 dscp=13 rate=301 burst=30000])
+AT_CHECK([ovn-nbctl qos-add ls0 to-lport 101 ip4 mark=15])
+AT_CHECK([ovn-nbctl qos-add ls0 to-lport 102 ip4 dscp=16 mark=17])
 
 dnl Add duplicated qos
 AT_CHECK([ovn-nbctl qos-add ls0 to-lport 100 ip4 dscp=11 rate=302 burst=30002], [1], [], [stderr])
@@ -328,6 +330,8 @@  from-lport   500 (udp) rate=100 burst=1000
 from-lport   400 (tcp) rate=300 burst=3000 dscp=0
   to-lport   300 (tcp) dscp=48
   to-lport   200 (ip) rate=101
+  to-lport   102 (ip4) dscp=16 mark=17
+  to-lport   101 (ip4) mark=15
   to-lport   100 (ip4) rate=301 burst=30000 dscp=13
 ])
 
@@ -371,11 +375,11 @@  AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip dscp=-1], [1], [],
 ])
 
 AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip dscpa=-1], [1], [],
-[ovn-nbctl: dscpa=-1: supported arguments are "dscp=", "rate=", and "burst="
+[ovn-nbctl: dscpa=-1: supported arguments are "dscp=", "rate=", "burst=" and "mark="
 ])
 
 AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip burst=123], [1], [],
-[ovn-nbctl: Either "rate" and/or "dscp" must be specified
+[ovn-nbctl: Either "mark", "rate" and/or "dscp" must be specified
 ])])
 
 dnl ---------------------------------------------------------------------
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 67e81ddba..8f2d1d1b0 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -9860,6 +9860,41 @@  AT_CHECK([fetch_column sb:Port_Binding options logical_port=public |grep -q 'qos
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([check OVN QoS mark/meter logical flows])
+AT_KEYWORDS([OVN-QoS])
+ovn_start
+
+check ovn-nbctl ls-add ls
+check ovn-nbctl qos-add ls from-lport 100 ip4 rate=100 burst=1000
+check ovn-nbctl qos-add ls from-lport 101 ip4 mark=15
+check ovn-nbctl qos-add ls from-lport 102 ip4 dscp=16
+check ovn-nbctl qos-add ls from-lport 103 ip4 dscp=17 mark=18
+
+check ovn-nbctl qos-add ls to-lport 100 ip4 rate=100 burst=1000
+check ovn-nbctl qos-add ls to-lport 101 ip4 mark=15
+check ovn-nbctl qos-add ls to-lport 102 ip4 dscp=16
+check ovn-nbctl qos-add ls to-lport 103 ip4 dscp=17 mark=18
+check ovn-nbctl --wait=sb sync
+
+AT_CHECK([ovn-sbctl lflow-list ls | grep qos | sed 's/table=../table=??/'], [0], [dnl
+  table=??(ls_in_qos_mark     ), priority=103  , match=(ip4), action=(ip.dscp = 17; pkt.mark = 18; next;)
+  table=??(ls_in_qos_mark     ), priority=102  , match=(ip4), action=(ip.dscp = 16; next;)
+  table=??(ls_in_qos_mark     ), priority=101  , match=(ip4), action=(pkt.mark = 15; next;)
+  table=??(ls_in_qos_mark     ), priority=0    , match=(1), action=(next;)
+  table=??(ls_in_qos_meter    ), priority=100  , match=(ip4), action=(set_meter(100, 1000); next;)
+  table=??(ls_in_qos_meter    ), priority=0    , match=(1), action=(next;)
+  table=??(ls_out_qos_mark    ), priority=103  , match=(ip4), action=(ip.dscp = 17; pkt.mark = 18; next;)
+  table=??(ls_out_qos_mark    ), priority=102  , match=(ip4), action=(ip.dscp = 16; next;)
+  table=??(ls_out_qos_mark    ), priority=101  , match=(ip4), action=(pkt.mark = 15; next;)
+  table=??(ls_out_qos_mark    ), priority=0    , match=(1), action=(next;)
+  table=??(ls_out_qos_meter   ), priority=100  , match=(ip4), action=(set_meter(100, 1000); next;)
+  table=??(ls_out_qos_meter   ), priority=0    , match=(1), action=(next;)
+])
+
+AT_CLEANUP
+])
+
 AT_SETUP([Tiered ACL logical flows])
 AT_KEYWORDS([acl])
 
@@ -11495,3 +11530,49 @@  CHECK_NO_CHANGE_AFTER_RECOMPUTE
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([check QoS table configuration])
+ovn_start
+
+check ovn-nbctl ls-add lsw0
+check ovn-nbctl lsp-add lsw0 lp0
+check ovn-nbctl ls-add lsw1
+check ovn-nbctl --wait=hv lsp-add lsw1 lp1
+
+AT_CHECK([ovn-nbctl --wait=hv -- --id=@lp0-qos create QoS priority=100 action=dscp=48 match="inport\=\=\"lp0\"\ &&\ is_chassis_resident(\"lp0\")" direction="from-lport" -- set Logical_Switch lsw0 qos_rules=@lp0-qos],[0],[ignore])
+check_row_count nb:QoS 1
+
+AT_CHECK([ovn-nbctl --wait=hv -- --id=@lp1-qos create QoS priority=101 action=mark=49 match="inport\=\=\"lp1\"\ &&\ is_chassis_resident(\"lp1\")" direction="from-lport" -- set Logical_Switch lsw1 qos_rules=@lp1-qos],[0],[ignore])
+check_row_count nb:QoS 2
+
+AT_CHECK([ovn-nbctl list qos |grep -q dscp=48])
+AT_CHECK([ovn-nbctl list qos |grep -q mark=49])
+
+check ovn-nbctl qos-del lsw0
+check_row_count nb:QoS 1
+
+# check wrong dscp values
+AT_CHECK([ovn-nbctl --wait=hv -- --id=@lp0-qos create QoS priority=100 action=dscp=100 match="inport\=\=\"lp0\"\ &&\ is_chassis_resident(\"lp0\")" direction="from-lport" -- set Logical_Switch lsw0 qos_rules=@lp0-qos],[0],[ignore])
+AT_CHECK([ovn-sbctl dump-flows | grep -q dscp], [1])
+AT_CHECK([grep -q "Bad 'dscp' value 100" northd/ovn-northd.log])
+
+check ovn-nbctl qos-del lsw0
+check_row_count nb:QoS 1
+
+AT_CHECK([ovn-nbctl --wait=hv -- --id=@lp0-qos create QoS priority=100 action=dscp="-1" match="inport\=\=\"lp0\"\ &&\ is_chassis_resident(\"lp0\")" direction="from-lport" -- set Logical_Switch lsw0 qos_rules=@lp0-qos],[1],[ignore],[dnl
+ovn-nbctl: constraint violation: -1 is not in the valid range 0 to 4294967295 (inclusive)
+])
+check_row_count nb:QoS 1
+
+check ovn-nbctl qos-del lsw1
+check_row_count nb:QoS 0
+
+# check wrong mark value
+AT_CHECK([ovn-nbctl --wait=hv -- --id=@lp1-qos create QoS priority=101 action=mark="-2" match="inport\=\=\"lp1\"\ &&\ is_chassis_resident(\"lp1\")" direction="from-lport" -- set Logical_Switch lsw1 qos_rules=@lp1-qos],[1],[ignore],[dnl
+ovn-nbctl: constraint violation: -2 is not in the valid range 0 to 4294967295 (inclusive)
+])
+check_row_count nb:QoS 0
+
+AT_CLEANUP
+])
diff --git a/tests/ovn.at b/tests/ovn.at
index 28c6b6c34..ee7c8cb1e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -37929,3 +37929,86 @@  AT_CHECK([ovn-sbctl --db=ssl:127.0.0.1:$TCP_PORT \
 
 OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 AT_CLEANUP
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([QoS packet marking])
+AT_KEYWORDS([ovn-qos-pkt-marking])
+AT_SKIP_IF([test $HAVE_SCAPY = no])
+ovn_start
+
+check ovn-nbctl lr-add lr0
+check ovn-nbctl ls-add ls0
+check ovn-nbctl ls-add ls1
+
+check ovn-nbctl lrp-add lr0 lrp0 f0:00:00:00:00:10 10.0.0.254/24
+check ovn-nbctl lsp-add ls0 lrp-s0 -- \
+    set Logical_Switch_Port lrp-s0 type=router \
+    options:router-port=lrp0 addresses='"f0:00:00:00:00:10"'
+check ovn-nbctl lrp-add lr0 lrp1 f0:00:00:00:00:20 20.0.0.254/24
+check ovn-nbctl lsp-add ls1 lrp-s1 -- \
+    set Logical_Switch_Port lrp-s1 type=router \
+    options:router-port=lrp1 addresses='"f0:00:00:00:00:20"'
+
+check ovn-nbctl --wait=sb lsp-add ls0 lp0
+check ovn-nbctl lsp-set-addresses lp0 f0:00:00:00:00:01
+check ovn-nbctl --wait=sb lsp-add ls0 lp1
+check ovn-nbctl lsp-set-addresses lp1 f0:00:00:00:00:02
+
+check ovn-nbctl --wait=sb lsp-add ls1 lp2
+check ovn-nbctl lsp-set-addresses lp2 f0:00:00:00:00:11
+check ovn-nbctl --wait=sb sync
+
+net_add n1
+sim_add hv
+
+as hv
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+
+check ovs-vsctl add-port br-int vif0 -- \
+    set Interface vif0 external-ids:iface-id=lp0 \
+    options:tx_pcap=hv/vif0-tx.pcap \
+    options:rxq_pcap=hv/vif0-rx.pcap \
+    ofport-request=1
+check ovs-vsctl add-port br-int vif1 -- \
+    set Interface vif1 external-ids:iface-id=lp1 \
+    options:tx_pcap=hv/vif1-tx.pcap \
+    options:rxq_pcap=hv/vif1-rx.pcap \
+    ofport-request=2
+check ovs-vsctl add-port br-int vif2 -- \
+    set Interface vif2 external-ids:iface-id=lp2 \
+    options:tx_pcap=hv/vif2-tx.pcap \
+    options:rxq_pcap=hv/vif2-rx.pcap \
+    ofport-request=3
+
+# Create QoS rules for packet marking.
+check ovn-nbctl qos-add ls0 from-lport 100 "inport==\"lp0\" && udp" mark=48
+check ovn-nbctl qos-add ls0 to-lport 100 "outport==\"lrp-s0\" && udp" mark=49
+check_row_count nb:QoS 2
+
+# Create some flows to count pkt marking.
+check ovn-nbctl acl-add ls0 to-lport 1002 "outport==\"lp1\" && pkt.mark == 48" drop
+check ovn-nbctl lr-policy-add lr0 200 "pkt.mark == 49" drop
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+send_udp_packet() {
+    local inport=$1 eth_src=$2 eth_dst=$3 ipv4_src=$4 ipv4_dst=$5
+    local packet=$(fmt_pkt "Ether(dst='${eth_dst}', src='${eth_src}')/ \
+                            IP(src='${ipv4_src}', dst='${ipv4_dst}', ttl=0x40)/ \
+                            UDP(sport=4242, dport=4343)")
+    as hv ovs-appctl netdev-dummy/receive vif$inport $packet
+}
+
+ls0_dp_key=$(printf "0x%x" $(fetch_column Datapath_Binding tunnel_key external_ids:name=ls0))
+lr0_dp_key=$(printf "0x%x" $(fetch_column Datapath_Binding tunnel_key external_ids:name=lr0))
+
+send_udp_packet 0 f0:00:00:00:00:01 f0:00:00:00:00:02 10.0.0.1 10.0.0.2
+OVS_WAIT_UNTIL([test 1 = $(as hv ovs-ofctl dump-flows br-int | grep -E "pkt_mark=0x30.*metadata=$ls0_dp_key" | grep -c n_packets=1)])
+
+send_udp_packet 0 f0:00:00:00:00:01 f0:00:00:00:00:10 10.0.0.1 10.0.0.10
+OVS_WAIT_UNTIL([test 1 = $(as hv ovs-ofctl dump-flows br-int | grep -E "pkt_mark=0x31.*metadata=$lr0_dp_key" | grep -c n_packets=1)])
+
+OVN_CLEANUP([hv])
+AT_CLEANUP
+])
diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
index 6f74bd557..c1a280191 100644
--- a/utilities/ovn-nbctl.8.xml
+++ b/utilities/ovn-nbctl.8.xml
@@ -463,7 +463,7 @@ 
 
     <h2>Logical Switch QoS Rule Commands</h2>
     <dl>
-      <dt>[<code>--may-exist</code>] <code>qos-add</code> <var>switch</var> <var>direction</var> <var>priority</var> <var>match</var> [<code>dscp=</code><var>dscp</var>] [<code>rate=</code><var>rate</var> [<code>burst=</code><var>burst</var>]]</dt>
+        <dt>[<code>--may-exist</code>] <code>qos-add</code> <var>switch</var> <var>direction</var> <var>priority</var> <var>match</var> [<code>mark=</code><var>mark</var>] [<code>dscp=</code><var>dscp</var>] [<code>rate=</code><var>rate</var> [<code>burst=</code><var>burst</var>]]</dt>
       <dd>
         <p>
           Adds QoS marking and metering rules to <var>switch</var>.
@@ -482,6 +482,9 @@ 
           <code>burst=</code><var>burst</var> specifies the burst rate
           limit in kilobits.  <code>dscp</code> and/or <code>rate</code>
           are required arguments.
+          If <code>mark=</code><var>mark</var> is specified, then matching
+          packets will be marked (through <code>pkt.mark</code>).
+          <var>mark</var> must be a positive integer.
         </p>
 
         <p>
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 526369b68..14460991f 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -283,7 +283,7 @@  ACL commands:\n\
                             print ACLs for SWITCH\n\
 \n\
 QoS commands:\n\
-  qos-add SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] [dscp=DSCP]\n\
+  qos-add SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] [dscp=DSCP] [mark=MARK]\n\
                             add an QoS rule to SWITCH\n\
   qos-del SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]\n\
                             remove QoS rules from SWITCH\n\
@@ -2614,6 +2614,9 @@  nbctl_qos_list(struct ctl_context *ctx)
             if (!strcmp(qos_rule->key_action[j], "dscp")) {
                 ds_put_format(&ctx->output, " dscp=%"PRId64"",
                               qos_rule->value_action[j]);
+            } else if (!strcmp(qos_rule->key_action[j], "mark")) {
+                ds_put_format(&ctx->output, " mark=%"PRId64"",
+                              qos_rule->value_action[j]);
             }
         }
         ds_put_cstr(&ctx->output, "\n");
@@ -2631,6 +2634,7 @@  nbctl_pre_qos_add(struct ctl_context *ctx)
     ovsdb_idl_add_column(ctx->idl, &nbrec_qos_col_direction);
     ovsdb_idl_add_column(ctx->idl, &nbrec_qos_col_priority);
     ovsdb_idl_add_column(ctx->idl, &nbrec_qos_col_match);
+    ovsdb_idl_add_column(ctx->idl, &nbrec_qos_col_action);
 }
 
 static void
@@ -2640,6 +2644,7 @@  nbctl_qos_add(struct ctl_context *ctx)
     const char *direction;
     int64_t priority;
     int64_t dscp = -1;
+    int64_t mark = 0;
     int64_t rate = 0;
     int64_t burst = 0;
     char *error;
@@ -2669,6 +2674,13 @@  nbctl_qos_add(struct ctl_context *ctx)
                 return;
             }
         }
+        else if (!strncmp(ctx->argv[i], "mark=", 5)) {
+            if (!ovs_scan(ctx->argv[i] + 5, "%"SCNd64, &mark) || mark < 0) {
+                ctl_error(ctx, "%s: mark must be a positive integer",
+                          ctx->argv[i] + 5);
+                return;
+            }
+        }
         else if (!strncmp(ctx->argv[i], "rate=", 5)) {
             if (!ovs_scan(ctx->argv[i] + 5, "%"SCNd64, &rate)
                 || rate < 1 || rate > UINT32_MAX) {
@@ -2686,14 +2698,15 @@  nbctl_qos_add(struct ctl_context *ctx)
             }
         } else {
             ctl_error(ctx, "%s: supported arguments are \"dscp=\", \"rate=\", "
-                      "and \"burst=\"", ctx->argv[i]);
+                      "\"burst=\" and \"mark=\"", ctx->argv[i]);
             return;
         }
     }
 
     /* Validate rate and dscp. */
-    if (-1 == dscp && !rate) {
-        ctl_error(ctx, "Either \"rate\" and/or \"dscp\" must be specified");
+    if (-1 == dscp && !rate && !mark) {
+        ctl_error(ctx, "Either \"mark\", \"rate\" and/or \"dscp\" must be "
+                       "specified");
         return;
     }
 
@@ -2702,9 +2715,11 @@  nbctl_qos_add(struct ctl_context *ctx)
     nbrec_qos_set_priority(qos, priority);
     nbrec_qos_set_direction(qos, direction);
     nbrec_qos_set_match(qos, ctx->argv[4]);
+    if (mark) {
+        nbrec_qos_update_action_setkey(qos, "mark", mark);
+    }
     if (-1 != dscp) {
-        const char *dscp_key = "dscp";
-        nbrec_qos_set_action(qos, &dscp_key, &dscp, 1);
+        nbrec_qos_update_action_setkey(qos, "dscp", dscp);
     }
     if (rate) {
         const char *bandwidth_key[2] = {"rate", "burst"};