[ovs-dev] [PATCH 1/1] ovn: Add column enabled to table Logical_Router

Na Zhu nazhu at cn.ibm.com
Tue Apr 5 13:07:14 UTC 2016


Hi Russell,


Really appreciate your comments. Will update soon.


Regards,
Juno Zhu
IBM China Development Labs (CDL) Cloud IaaS Lab
Email: nazhu at cn.ibm.com
5F, Building 10, 399 Keyuan Road, Zhangjiang Hi-Tech Park, Pudong New 
District, Shanghai, China (201203)



From:   Russell Bryant <russell at ovn.org>
To:     Na Zhu/China/IBM at IBMCN
Cc:     ovs dev <dev at openvswitch.org>
Date:   2016/04/05 20:35
Subject:        Re: [ovs-dev] [PATCH 1/1] ovn: Add column enabled to table 
Logical_Router





On Tue, Apr 5, 2016 at 6:54 AM, Na Zhu <nazhu at cn.ibm.com> wrote:
This patch add column "enabled" to table Logical_Router for setting router
administrative state.

The type of "enabled" is bool.

If the administrative state is false, delete all the flows relevant to the

logical router from table Logical_Flow.

You have an extra blank line in the commit message above.
 

Signed-off-by: Na Zhu <nazhu at cn.ibm.com>
Reported-by: Na Zhu <nazhu at cn.ibm.com>
Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1563175
---
 ovn/northd/ovn-northd.8.xml |  4 ++++
 ovn/northd/ovn-northd.c     | 46
+++++++++++++++++++++++++++++++++++++++++++++
 ovn/ovn-nb.ovsschema        |  3 ++-
 ovn/ovn-nb.xml              |  7 +++++++
 4 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index da776e1..fed996c 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -397,6 +397,10 @@ output;

     <h2>Logical Router Datapaths</h2>

+    <p>
+      This is only enabled logical router.
+    </p>
+

I suggest this alternative wording:

"Logical router datapaths will only exist for <ref table="Logical_Router" 
db="OVN_Northbound"/> rows in the <ref db="OVN_Northbound"/> database that 
do not have <ref column="enabled" table="Logical_Router" 
db="OVN_Northbound"/> set to <code>false</code>."
 
     <h3>Ingress Table 0: L2 Admission Control</h3>

     <p>
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 4b1d611..ec1c6af 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1312,6 +1312,12 @@ lport_is_up(const struct nbrec_logical_port *lport)
 }

 static bool
+lrouter_is_enabled(const struct nbrec_logical_router *lrouter)
+{
+    return !lrouter->enabled || *lrouter->enabled;
+}
+
+static bool
 has_stateful_acl(struct ovn_datapath *od)
 {
     for (size_t i = 0; i < od->nbs->n_acls; i++) {
@@ -1793,6 +1799,10 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
             continue;
         }

+        if (!lrouter_is_enabled(od->nbr)) {
+            continue;
+        }
+

This patch has to check this in several places.  I think it could be 
simplified to only check it in one place.

ovn-northd only iterates the Logical_Router table in OVN_Northbound in a 
single place.  Could you just ignore the Logical_Router row in that one 
place instead?
 diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 40a7a97..e878ac8 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Northbound",
     "version": "2.0.2",
-    "cksum": "4289495412 4436",
+    "cksum": "1227843805 4513",
     "tables": {
         "Logical_Switch": {
             "columns": {

You should update the schema version, as well.  I would make it "2.1.0".
 
-- 
Russell Bryant





More information about the dev mailing list