bridge: Resend status changes to database if previous transaction

was not successful.

Bridge, port and interface status changes are not sent to the
database if the connectivity and netdev sequence numbers have not
changed. However, if the previous database transaction fails, then
status changes will not be updated in the database until the
connectivity and netdev sequence numbers change again. This could
leave the database in an incorrect state for a long period of time.

This patch always sends status changes to the database if the last
transaction was not successful.

Bug #1256577
Signed-off-by: Ryan Wilson <wryan@nicira.com>
Acked-by: Alex Wang <alexw@nicira.com>
Signed-off-by: Alex Wang <alexw@nicira.com>

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index fdb4cd6..7f59b94 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1826,14 +1826,14 @@ out:
 }
 
 static int
-get_cfm_status(const struct ofport *ofport_,
+get_cfm_status(const struct ofport *ofport_, bool force,
                struct ofproto_cfm_status *status)
 {
     struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
     int ret = 0;
 
     if (ofport->cfm) {
-        if (cfm_check_status_change(ofport->cfm)) {
+        if (cfm_check_status_change(ofport->cfm) || force) {
             status->faults = cfm_get_fault(ofport->cfm);
             status->flap_count = cfm_get_flap_count(ofport->cfm);
             status->remote_opstate = cfm_get_opup(ofport->cfm);
@@ -1868,13 +1868,13 @@ set_bfd(struct ofport *ofport_, const struct smap *cfg)
 }
 
 static int
-get_bfd_status(struct ofport *ofport_, struct smap *smap)
+get_bfd_status(struct ofport *ofport_, bool force, struct smap *smap)
 {
     struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
     int ret = 0;
 
     if (ofport->bfd) {
-        if (bfd_check_status_change(ofport->bfd)) {
+        if (bfd_check_status_change(ofport->bfd) || force) {
             bfd_get_status(ofport->bfd, smap);
         } else {
             ret = NO_STATUS_CHANGE;

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 141adec..639b501 100644 (file)
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1435,10 +1435,13 @@ struct ofproto_class {
      * support CFM, as does a null pointer. */
     int (*set_cfm)(struct ofport *ofport, const struct cfm_settings *s);
 
-    /* Checks the status of CFM configured on 'ofport'.  Returns 0 if the
-     * port's CFM status was successfully stored into '*status'.  Returns
-     * negative number if there is no status change since last update.
-     * Returns positive errno otherwise.
+    /* Checks the status of CFM configured on 'ofport' and stores port's CFM
+     * status in '*status'.  If 'force' is set to true, status will be returned
+     * even if there is no status change since last update.
+     *
+     * Returns 0 on success.  Returns a negative number if there is no status
+     * change since last update and 'force' is set to false.  Returns positive
+     * errno otherwise.
      *
      * EOPNOTSUPP as a return value indicates that this ofproto_class does not
      * support CFM, as does a null pointer.
@@ -1446,7 +1449,7 @@ struct ofproto_class {
      * The caller must provide and own '*status', and it must free the array
      * returned in 'status->rmps'.  '*status' is indeterminate if the return
      * value is non-zero. */
-    int (*get_cfm_status)(const struct ofport *ofport,
+    int (*get_cfm_status)(const struct ofport *ofport, bool force,
                           struct ofproto_cfm_status *status);
 
     /* Configures BFD on 'ofport'.
@@ -1459,13 +1462,17 @@ struct ofproto_class {
      * support BFD, as does a null pointer. */
     int (*set_bfd)(struct ofport *ofport, const struct smap *cfg);
 
-    /* Populates 'smap' with the status of BFD on 'ofport'.  Returns 0 on
-     * success.  Returns a negative number if there is no status change since
-     * last update.  Returns a positive errno otherwise.
+    /* Populates 'smap' with the status of BFD on 'ofport'.  If 'force' is set
+     * to true, status will be returned even if there is no status change since
+     * last update.
+     *
+     * Returns 0 on success.  Returns a negative number if there is no status
+     * change since last update and 'force' is set to false.  Returns a positive
+     * errno otherwise.
      *
      * EOPNOTSUPP as a return value indicates that this ofproto_class does not
      * support BFD, as does a null pointer. */
-    int (*get_bfd_status)(struct ofport *ofport, struct smap *smap);
+    int (*get_bfd_status)(struct ofport *ofport, bool force, struct smap *smap);
 
     /* Configures spanning tree protocol (STP) on 'ofproto' using the
      * settings defined in 's'.

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index b98e9e5..608ddad 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1015,19 +1015,22 @@ ofproto_port_set_bfd(struct ofproto *ofproto, ofp_port_t ofp_port,
     }
 }
 
-/* Populates 'status' with the status of BFD on 'ofport'.  Returns 0 on
- * success.  Returns a negative number if there is no status change since
- * last update.  Returns a positive errno otherwise.  Has no effect if
- * 'ofp_port' is not an OpenFlow port in 'ofproto'.
+/* Populates 'status' with the status of BFD on 'ofport'.  If 'force' is set to
+ * true, status will be returned even if there is no status change since last
+ * update.
+ *
+ * Returns 0 on success.  Returns a negative number if there is no status change
+ * since last update and 'force' is set to false.  Returns a positive errno
+ * otherwise.  Has no effect if 'ofp_port' is not an OpenFlow port in 'ofproto'.
  *
  * The caller must provide and own '*status'. */
 int
 ofproto_port_get_bfd_status(struct ofproto *ofproto, ofp_port_t ofp_port,
-                            struct smap *status)
+                            bool force, struct smap *status)
 {
     struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
     return (ofport && ofproto->ofproto_class->get_bfd_status
-            ? ofproto->ofproto_class->get_bfd_status(ofport, status)
+            ? ofproto->ofproto_class->get_bfd_status(ofport, force, status)
             : EOPNOTSUPP);
 }
 
@@ -3715,21 +3718,23 @@ ofproto_get_netflow_ids(const struct ofproto *ofproto,
     ofproto->ofproto_class->get_netflow_ids(ofproto, engine_type, engine_id);
 }
 
-/* Checks the status of CFM configured on 'ofp_port' within 'ofproto'.
- * Returns 0 if the port's CFM status was successfully stored into
- * '*status'.  Returns positive errno if the port did not have CFM
- * configured.  Returns negative number if there is no status change
- * since last update.
+/* Checks the status of CFM configured on 'ofp_port' within 'ofproto' and stores
+ * the port's CFM status in '*status'.  If 'force' is set to true, status will
+ * be returned even if there is no status change since last update.
+ *
+ * Returns 0 on success.  Returns a negative number if there is no status
+ * change since last update and 'force' is set to false.  Returns positive errno
+ * if the port did not have CFM configured.
  *
  * The caller must provide and own '*status', and must free 'status->rmps'.
  * '*status' is indeterminate if the return value is non-zero. */
 int
 ofproto_port_get_cfm_status(const struct ofproto *ofproto, ofp_port_t ofp_port,
-                            struct ofproto_cfm_status *status)
+                            bool force, struct ofproto_cfm_status *status)
 {
     struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
     return (ofport && ofproto->ofproto_class->get_cfm_status
-            ? ofproto->ofproto_class->get_cfm_status(ofport, status)
+            ? ofproto->ofproto_class->get_cfm_status(ofport, force, status)
             : EOPNOTSUPP);
 }
 

diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 9ba6354..9a06849 100644 (file)
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -265,7 +265,7 @@ void ofproto_port_set_cfm(struct ofproto *, ofp_port_t ofp_port,
 void ofproto_port_set_bfd(struct ofproto *, ofp_port_t ofp_port,
                           const struct smap *cfg);
 int ofproto_port_get_bfd_status(struct ofproto *, ofp_port_t ofp_port,
-                                struct smap *);
+                                bool force, struct smap *);
 int ofproto_port_is_lacp_current(struct ofproto *, ofp_port_t ofp_port);
 int ofproto_port_set_stp(struct ofproto *, ofp_port_t ofp_port,
                          const struct ofproto_port_stp_settings *);
@@ -420,7 +420,7 @@ struct ofproto_cfm_status {
 };
 
 int ofproto_port_get_cfm_status(const struct ofproto *,
-                                ofp_port_t ofp_port,
+                                ofp_port_t ofp_port, bool force,
                                 struct ofproto_cfm_status *);
 \f
 /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 30fce66..5dcd3ba 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -172,9 +172,15 @@ static uint64_t connectivity_seqno = LLONG_MIN;
  * we check the return status of each update transaction and do not start new
  * update if the previous transaction status is 'TXN_INCOMPLETE'.
  *
- * 'statux_txn' is NULL if there is no ongoing status update.
+ * 'status_txn' is NULL if there is no ongoing status update.
+ *
+ * If the previous database transaction was incomplete or failed (is not
+ * 'TXN_SUCCESS' or 'TXN_UNCHANGED'), 'force_status_commit' is set to true.
+ * This means that 'status_txn' must be committed next iteration of bridge_run()
+ * even if the connectivity or netdev sequence numbers do not change.
  */
 static struct ovsdb_idl_txn *status_txn;
+static bool force_status_commit = true;
 
 /* When the status update transaction returns 'TXN_INCOMPLETE', should register a
  * timeout in 'STATUS_CHECK_AGAIN_MSEC' to check again. */
@@ -1541,7 +1547,6 @@ iface_create(struct bridge *br, const struct ovsrec_interface *iface_cfg,
 
     /* Populate initial status in database. */
     iface_refresh_stats(iface);
-    iface_refresh_netdev_status(iface);
 
     /* Add bond fake iface if necessary. */
     if (port_is_bond_fake_iface(port)) {
@@ -1814,7 +1819,8 @@ iface_refresh_netdev_status(struct iface *iface)
         return;
     }
 
-    if (iface->change_seq == netdev_get_change_seq(iface->netdev)) {
+    if (iface->change_seq == netdev_get_change_seq(iface->netdev)
+        && !force_status_commit) {
         return;
     }
 
@@ -1907,7 +1913,8 @@ iface_refresh_ofproto_status(struct iface *iface)
 
     smap_init(&smap);
     error = ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
-                                        iface->ofp_port, &smap);
+                                        iface->ofp_port, force_status_commit,
+                                        &smap);
     if (error >= 0) {
         ovsrec_interface_set_bfd_status(iface->cfg, &smap);
     }
@@ -1924,7 +1931,8 @@ iface_refresh_cfm_stats(struct iface *iface)
     int error;
 
     error = ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
-                                        iface->ofp_port, &status);
+                                        iface->ofp_port, force_status_commit,
+                                        &status);
     if (error < 0) {
         /* Do nothing if there is no status change since last update. */
     } else if (error > 0) {
@@ -2414,7 +2422,7 @@ bridge_run(void)
 
         /* Check the need to update status. */
         seq = seq_read(connectivity_seq_get());
-        if (seq != connectivity_seqno) {
+        if (seq != connectivity_seqno || force_status_commit) {
             connectivity_seqno = seq;
             status_txn = ovsdb_idl_txn_create(idl);
             HMAP_FOR_EACH (br, node, &all_bridges) {
@@ -2438,6 +2446,17 @@ bridge_run(void)
         enum ovsdb_idl_txn_status status;
 
         status = ovsdb_idl_txn_commit(status_txn);
+
+        /* If the transaction is incomplete or fails, 'status_txn'
+         * needs to be committed next iteration of bridge_run() even if
+         * connectivity or netdev sequence numbers do not change. */
+        if (status == TXN_SUCCESS || status == TXN_UNCHANGED)
+        {
+            force_status_commit = false;
+        } else {
+            force_status_commit = true;
+        }
+
         /* Do not destroy "status_txn" if the transaction is
          * "TXN_INCOMPLETE". */
         if (status != TXN_INCOMPLETE) {
@@ -2477,11 +2496,11 @@ bridge_wait(void)
         poll_timer_wait_until(stats_timer);
     }
 
-    /* If the status database transaction is 'TXN_INCOMPLETE' in this run,
-     * register a timeout in 'STATUS_CHECK_AGAIN_MSEC'.  Else, wait on the
-     * global connectivity sequence number.  Note, this also helps batch
-     * multiple status changes into one transaction. */
-    if (status_txn) {
+    /* If the status database transaction is 'TXN_INCOMPLETE' or is
+     * unsuccessful, register a timeout in 'STATUS_CHECK_AGAIN_MSEC'.  Else,
+     * wait on the global connectivity sequence number.  Note, this also helps
+     * batch multiple status changes into one transaction. */
+    if (force_status_commit) {
         poll_timer_wait_until(time_msec() + STATUS_CHECK_AGAIN_MSEC);
     } else {
         seq_wait(connectivity_seq_get(), connectivity_seqno);