diff mbox series

[ovs-dev,1/6] connmgr: Suppress duplicate port status notifications.

Message ID 20180824173521.19922-1-blp@ovn.org
State Accepted
Headers show
Series [ovs-dev,1/6] connmgr: Suppress duplicate port status notifications. | expand

Commit Message

Ben Pfaff Aug. 24, 2018, 5:35 p.m. UTC 
When the status of a port changes, ofproto calls into connmgr to notify
controllers.  Sometimes, particular changes are only visible to controllers
running specific versions of OpenFlow.  Until now, OVS would send those
controllers duplicate port status notifications.  This is unnecessary and
somewhat confusing.  This commit eliminates it.

This commit updates one of the tests not to expect duplicate notifications.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ofproto/connmgr.c | 38 ++++++++++++++++++++++++++++----------
 ofproto/connmgr.h |  4 +++-
 ofproto/ofproto.c | 18 ++++++++++--------
 tests/lacp.at     |  3 ---
 4 files changed, 41 insertions(+), 22 deletions(-)

Comments

Ben Pfaff Oct. 12, 2018, 3:56 p.m. UTC | #1 
On Fri, Aug 24, 2018 at 10:35:16AM -0700, Ben Pfaff wrote:
> When the status of a port changes, ofproto calls into connmgr to notify
> controllers.  Sometimes, particular changes are only visible to controllers
> running specific versions of OpenFlow.  Until now, OVS would send those
> controllers duplicate port status notifications.  This is unnecessary and
> somewhat confusing.  This commit eliminates it.
> 
> This commit updates one of the tests not to expect duplicate notifications.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>

This series still needs a review.
Numan Siddique Oct. 17, 2018, 12:09 p.m. UTC | #2 
On Fri, Oct 12, 2018 at 9:52 PM Ben Pfaff <blp@ovn.org> wrote:

> On Fri, Aug 24, 2018 at 10:35:16AM -0700, Ben Pfaff wrote:
> > When the status of a port changes, ofproto calls into connmgr to notify
> > controllers.  Sometimes, particular changes are only visible to
> controllers
> > running specific versions of OpenFlow.  Until now, OVS would send those
> > controllers duplicate port status notifications.  This is unnecessary and
> > somewhat confusing.  This commit eliminates it.
> >
> > This commit updates one of the tests not to expect duplicate
> notifications.
> >
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
>
> This series still needs a review.
>

I will review this series and get back to you with comments.

Thanks
Numan


> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Numan Siddique Oct. 18, 2018, 9:24 a.m. UTC | #3 
On Wed, Oct 17, 2018 at 5:39 PM Numan Siddique <nusiddiq@redhat.com> wrote:

>
>
>
> On Fri, Oct 12, 2018 at 9:52 PM Ben Pfaff <blp@ovn.org> wrote:
>
>> On Fri, Aug 24, 2018 at 10:35:16AM -0700, Ben Pfaff wrote:
>> > When the status of a port changes, ofproto calls into connmgr to notify
>> > controllers.  Sometimes, particular changes are only visible to
>> controllers
>> > running specific versions of OpenFlow.  Until now, OVS would send those
>> > controllers duplicate port status notifications.  This is unnecessary
>> and
>> > somewhat confusing.  This commit eliminates it.
>> >
>> > This commit updates one of the tests not to expect duplicate
>> notifications.
>> >
>> > Signed-off-by: Ben Pfaff <blp@ovn.org>
>>
>> This series still needs a review.
>>
>
> I will review this series and get back to you with comments.
>

Acked-by: Numan Siddique <nusididq@redhat.com> for all the patches in this
series.



>
> Thanks
> Numan
>
>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
Ben Pfaff Oct. 18, 2018, 3:24 p.m. UTC | #4 
On Thu, Oct 18, 2018 at 02:54:44PM +0530, Numan Siddique wrote:
> On Wed, Oct 17, 2018 at 5:39 PM Numan Siddique <nusiddiq@redhat.com> wrote:
> 
> >
> >
> >
> > On Fri, Oct 12, 2018 at 9:52 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> >> On Fri, Aug 24, 2018 at 10:35:16AM -0700, Ben Pfaff wrote:
> >> > When the status of a port changes, ofproto calls into connmgr to notify
> >> > controllers.  Sometimes, particular changes are only visible to
> >> controllers
> >> > running specific versions of OpenFlow.  Until now, OVS would send those
> >> > controllers duplicate port status notifications.  This is unnecessary
> >> and
> >> > somewhat confusing.  This commit eliminates it.
> >> >
> >> > This commit updates one of the tests not to expect duplicate
> >> notifications.
> >> >
> >> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> >>
> >> This series still needs a review.
> >>
> >
> > I will review this series and get back to you with comments.
> >
> 
> Acked-by: Numan Siddique <nusididq@redhat.com> for all the patches in this
> series.

Thanks for the reviews.  I applied this series to master.
diff mbox series

Patch

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index f78b4c5ff411..50e0b8f991b5 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1609,24 +1609,28 @@  ofconn_send(const struct ofconn *ofconn, struct ofpbuf *msg,
 
 /* Sending asynchronous messages. */
 
-/* Sends an OFPT_PORT_STATUS message with 'opp' and 'reason' to appropriate
+/* Sends an OFPT_PORT_STATUS message with 'new_pp' and 'reason' to appropriate
  * controllers managed by 'mgr'.  For messages caused by a controller
  * OFPT_PORT_MOD, specify 'source' as the controller connection that sent the
- * request; otherwise, specify 'source' as NULL. */
+ * request; otherwise, specify 'source' as NULL.
+ *
+ * If 'reason' is OFPPR_MODIFY and 'old_pp' is nonnull, then messages are
+ * suppressed in the case where the change would not be visible to a particular
+ * controller.  For example, OpenFlow 1.0 does not have the OFPPS_LIVE flag, so
+ * this would suppress a change solely to that flag from being sent to an
+ * OpenFlow 1.0 controller. */
 void
 connmgr_send_port_status(struct connmgr *mgr, struct ofconn *source,
-                         const struct ofputil_phy_port *pp, uint8_t reason)
+                         const struct ofputil_phy_port *old_pp,
+                         const struct ofputil_phy_port *new_pp,
+                         uint8_t reason)
 {
     /* XXX Should limit the number of queued port status change messages. */
-    struct ofputil_port_status ps;
-    struct ofconn *ofconn;
+    struct ofputil_port_status new_ps = { reason, *new_pp };
 
-    ps.reason = reason;
-    ps.desc = *pp;
+    struct ofconn *ofconn;
     LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
         if (ofconn_receives_async_msg(ofconn, OAM_PORT_STATUS, reason)) {
-            struct ofpbuf *msg;
-
             /* Before 1.5, OpenFlow specified that OFPT_PORT_MOD should not
              * generate OFPT_PORT_STATUS messages.  That requirement was a
              * relic of how OpenFlow originally supported a single controller,
@@ -1651,7 +1655,21 @@  connmgr_send_port_status(struct connmgr *mgr, struct ofconn *source,
                 continue;
             }
 
-            msg = ofputil_encode_port_status(&ps, ofconn_get_protocol(ofconn));
+            enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
+            struct ofpbuf *msg = ofputil_encode_port_status(&new_ps, protocol);
+            if (reason == OFPPR_MODIFY && old_pp) {
+                struct ofputil_port_status old_ps = { reason, *old_pp };
+                struct ofpbuf *old_msg = ofputil_encode_port_status(&old_ps,
+                                                                    protocol);
+                bool suppress = ofpbuf_equal(msg, old_msg);
+                ofpbuf_delete(old_msg);
+
+                if (suppress) {
+                    ofpbuf_delete(msg);
+                    continue;
+                }
+            }
+
             ofconn_send(ofconn, msg, NULL);
         }
     }
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index eb3be16686c5..4a22f1c26611 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -147,7 +147,9 @@  void ofconn_report_flow_mod(struct ofconn *, enum ofp_flow_mod_command);
 /* Sending asynchronous messages. */
 bool connmgr_wants_packet_in_on_miss(struct connmgr *mgr);
 void connmgr_send_port_status(struct connmgr *, struct ofconn *source,
-                              const struct ofputil_phy_port *, uint8_t reason);
+                              const struct ofputil_phy_port *old_pp,
+                              const struct ofputil_phy_port *new_pp,
+                              uint8_t reason);
 void connmgr_send_flow_removed(struct connmgr *,
                                const struct ofputil_flow_removed *)
     OVS_REQUIRES(ofproto_mutex);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 9552a585d096..95235f21232c 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2417,7 +2417,7 @@  ofport_install(struct ofproto *p,
     if (error) {
         goto error;
     }
-    connmgr_send_port_status(p->connmgr, NULL, pp, OFPPR_ADD);
+    connmgr_send_port_status(p->connmgr, NULL, NULL, pp, OFPPR_ADD);
     return 0;
 
 error:
@@ -2438,7 +2438,7 @@  ofport_remove(struct ofport *ofport)
     struct ofproto *p = ofport->ofproto;
     bool is_mtu_overridden = ofport_is_mtu_overridden(p, ofport);
 
-    connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp,
+    connmgr_send_port_status(ofport->ofproto->connmgr, NULL, NULL, &ofport->pp,
                              OFPPR_DELETE);
     ofport_destroy(ofport, true);
     if (!is_mtu_overridden) {
@@ -2483,8 +2483,9 @@  void
 ofproto_port_set_state(struct ofport *port, enum ofputil_port_state state)
 {
     if (port->pp.state != state) {
+        struct ofputil_phy_port old_pp = port->pp;
         port->pp.state = state;
-        connmgr_send_port_status(port->ofproto->connmgr, NULL,
+        connmgr_send_port_status(port->ofproto->connmgr, NULL, &old_pp,
                                  &port->pp, OFPPR_MODIFY);
     }
 }
@@ -2630,6 +2631,7 @@  update_port(struct ofproto *ofproto, const char *name)
         port = ofproto_get_port(ofproto, ofproto_port.ofp_port);
         if (port && !strcmp(netdev_get_name(port->netdev), name)) {
             struct netdev *old_netdev = port->netdev;
+            struct ofputil_phy_port old_pp = port->pp;
 
             /* 'name' hasn't changed location.  Any properties changed? */
             bool port_changed = !ofport_equal(&port->pp, &pp);
@@ -2652,7 +2654,7 @@  update_port(struct ofproto *ofproto, const char *name)
             /* Send status update, if any port property changed */
             if (port_changed) {
                 connmgr_send_port_status(port->ofproto->connmgr, NULL,
-                                         &port->pp, OFPPR_MODIFY);
+                                         &old_pp, &port->pp, OFPPR_MODIFY);
             }
 
             netdev_close(old_netdev);
@@ -3648,11 +3650,11 @@  update_port_config(struct ofconn *ofconn, struct ofport *port,
     }
 
     if (toggle) {
-        enum ofputil_port_config old_config = port->pp.config;
+        struct ofputil_phy_port old_pp = port->pp;
         port->pp.config ^= toggle;
-        port->ofproto->ofproto_class->port_reconfigured(port, old_config);
-        connmgr_send_port_status(port->ofproto->connmgr, ofconn, &port->pp,
-                                 OFPPR_MODIFY);
+        port->ofproto->ofproto_class->port_reconfigured(port, old_pp.config);
+        connmgr_send_port_status(port->ofproto->connmgr, ofconn, &old_pp,
+                                 &port->pp, OFPPR_MODIFY);
     }
 }
 
diff --git a/tests/lacp.at b/tests/lacp.at
index a7f75ac67acb..7b460d7be35e 100644
--- a/tests/lacp.at
+++ b/tests/lacp.at
@@ -805,7 +805,6 @@  check_liveness 1 LIVE
 AT_CHECK([ovs-vsctl \
 -- add-port br0 null0 -- set int null0 type=patch options:peer=p2 -- set int p2 options:peer=null0 \
 -- add-port br1 null1 -- set int null1 type=patch options:peer=p0 -- set int p0 options:peer=null1])
-check_liveness 2 LIVE
 
 # Wait 4 more simulated seconds.  The LACP state should become "defaulted" for p0 and p2.
 ovs-appctl time/warp 4100 100
@@ -901,7 +900,6 @@  check_liveness 1 LIVE
 AT_CHECK([ovs-vsctl \
 -- add-port br0 null0 -- set int null0 type=patch options:peer=p2 -- set int p2 options:peer=null0 \
 -- add-port br1 null1 -- set int null1 type=patch options:peer=p0 -- set int p0 options:peer=null1])
-check_liveness 2 LIVE
 
 # Wait 4 more simulated seconds.  The LACP state should become "defaulted" for p0 and p2.
 ovs-appctl time/warp 4100 100
@@ -997,7 +995,6 @@  check_liveness 1 LIVE
 AT_CHECK([ovs-vsctl \
 -- add-port br0 null0 -- set int null0 type=patch options:peer=p2 -- set int p2 options:peer=null0 \
 -- add-port br1 null1 -- set int null1 type=patch options:peer=p0 -- set int p0 options:peer=null1])
-check_liveness 2 LIVE
 
 # Wait 4 more simulated seconds.  The LACP state should become "defaulted" for p0 and p2.
 ovs-appctl time/warp 4100 100