<div dir="ltr">Hey Ethan,<div><br></div><div>Could you review these two patches?  Should be straightforward,</div><div><br></div><div>Thanks,</div><div>Alex Wang,</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">

On Wed, May 7, 2014 at 12:01 AM, Alex Wang <span dir="ltr">&lt;<a href="mailto:alexw@nicira.com" target="_blank">alexw@nicira.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Commit 88bf179aa3 (bfd/cfm: Check status change before update<br>
status to database.) used a boolean flag to trigger cfm status<br>
update.  However, the flag is not set on cfm creation and deletion.<br>
And this causes stale status in database which may confuse users.<br>
<br>
This commit fixes the issue by making cfm module trigger status<br>
update on creation and deletion.<br>
<br>
Bug #1245800<br>
<br>
Signed-off-by: Alex Wang &lt;<a href="mailto:alexw@nicira.com">alexw@nicira.com</a>&gt;<br>
---<br>
 lib/cfm.c    |   18 +++++++++--------<br>
 tests/<a href="http://cfm.at" target="_blank">cfm.at</a> |   62 ++++++++++++++++++++++++++++++++++++++++++++++------------<br>
 2 files changed, 59 insertions(+), 21 deletions(-)<br>
<br>
diff --git a/lib/cfm.c b/lib/cfm.c<br>
index 1b32625..c271e1e 100644<br>
--- a/lib/cfm.c<br>
+++ b/lib/cfm.c<br>
@@ -328,6 +328,14 @@ cfm_init(void)<br>
                              1, 2, cfm_unixctl_set_fault, NULL);<br>
 }<br>
<br>
+/* Records the status change and changes the global connectivity seq. */<br>
+static void<br>
+cfm_status_changed(struct cfm *cfm) OVS_REQUIRES(mutex)<br>
+{<br>
+    seq_change(connectivity_seq_get());<br>
+    cfm-&gt;status_changed = true;<br>
+}<br>
+<br>
 /* Allocates a &#39;cfm&#39; object called &#39;name&#39;.  &#39;cfm&#39; should be initialized by<br>
  * cfm_configure() before use. */<br>
 struct cfm *<br>
@@ -349,6 +357,7 @@ cfm_create(const struct netdev *netdev) OVS_EXCLUDED(mutex)<br>
     ovs_refcount_init(&amp;cfm-&gt;ref_cnt);<br>
<br>
     ovs_mutex_lock(&amp;mutex);<br>
+    cfm_status_changed(cfm);<br>
     cfm_generate_maid(cfm);<br>
     hmap_insert(all_cfms, &amp;cfm-&gt;hmap_node, hash_string(cfm-&gt;name, 0));<br>
     ovs_mutex_unlock(&amp;mutex);<br>
@@ -370,6 +379,7 @@ cfm_unref(struct cfm *cfm) OVS_EXCLUDED(mutex)<br>
     }<br>
<br>
     ovs_mutex_lock(&amp;mutex);<br>
+    cfm_status_changed(cfm);<br>
     hmap_remove(all_cfms, &amp;cfm-&gt;hmap_node);<br>
     ovs_mutex_unlock(&amp;mutex);<br>
<br>
@@ -395,14 +405,6 @@ cfm_ref(const struct cfm *cfm_)<br>
     return cfm;<br>
 }<br>
<br>
-/* Records the status change and changes the global connectivity seq. */<br>
-static void<br>
-cfm_status_changed(struct cfm *cfm) OVS_REQUIRES(mutex)<br>
-{<br>
-    seq_change(connectivity_seq_get());<br>
-    cfm-&gt;status_changed = true;<br>
-}<br>
-<br>
 /* Should be run periodically to update fault statistics messages. */<br>
 void<br>
 cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)<br>
diff --git a/tests/<a href="http://cfm.at" target="_blank">cfm.at</a> b/tests/<a href="http://cfm.at" target="_blank">cfm.at</a><br>
index fdca4ac..06cab90 100644<br>
--- a/tests/<a href="http://cfm.at" target="_blank">cfm.at</a><br>
+++ b/tests/<a href="http://cfm.at" target="_blank">cfm.at</a><br>
@@ -28,23 +28,54 @@ MPID $2: extended<br>
 ])<br>
<br>
 m4_define([CFM_VSCTL_LIST_IFACE], [<br>
-AT_CHECK([ovs-vsctl list interface $1 | sed -n &#39;/$2/p&#39;],[0],<br>
+AT_CHECK([ovs-vsctl list interface $1 | sed -n &#39;/$2 /p&#39;],[0],<br>
 [dnl<br>
 $3<br>
 ])<br>
 ])<br>
<br>
 m4_define([CFM_CHECK_DB], [<br>
-CFM_VSCTL_LIST_IFACE([$1], [cfm_fault_status], [cfm_fault_status    : [[$2]]])<br>
-CFM_VSCTL_LIST_IFACE([$1], [cfm_flap_count], [cfm_flap_count      : $3])<br>
-CFM_VSCTL_LIST_IFACE([$1], [cfm_health], [cfm_health          : [[$4]]])<br>
-CFM_VSCTL_LIST_IFACE([$1], [cfm_remote_mpids], [cfm_remote_mpids    : [[$5]]])<br>
-CFM_VSCTL_LIST_IFACE([$1], [cfm_remote_opstate], [cfm_remote_opstate  : $6])<br>
+CFM_VSCTL_LIST_IFACE([$1], [cfm_fault], [cfm_fault           : $2])<br>
+CFM_VSCTL_LIST_IFACE([$1], [cfm_fault_status], [cfm_fault_status    : [[$3]]])<br>
+CFM_VSCTL_LIST_IFACE([$1], [cfm_flap_count], [cfm_flap_count      : $4])<br>
+CFM_VSCTL_LIST_IFACE([$1], [cfm_health], [cfm_health          : [[$5]]])<br>
+CFM_VSCTL_LIST_IFACE([$1], [cfm_remote_mpids], [cfm_remote_mpids    : [[$6]]])<br>
+CFM_VSCTL_LIST_IFACE([$1], [cfm_remote_opstate], [cfm_remote_opstate  : $7])<br>
 ])<br>
<br>
-# This test checks the update of cfm status to OVSDB at startup.<br>
-# The cfm status should be updated to OVSDB within 3.5 * cfm_interval.<br>
-AT_SETUP([cfm - check update ovsdb])<br>
+# These two tests check the update of cfm status at different scenarios.<br>
+<br>
+# Test cfm status update at startup and removal.<br>
+AT_SETUP([cfm - check update ovsdb 1])<br>
+OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=gre \<br>
+                    options:remote_ip=1.2.3.4 -- \<br>
+                    set Interface p0 other_config:cfm_interval=300 other_config:cfm_extended=true])<br>
+<br>
+ovs-appctl time/stop<br>
+<br>
+AT_CHECK([ovs-vsctl set Interface p0 cfm_mpid=1])<br>
+# at beginning, since the first fault check timeout is not reached<br>
+# cfm_fault should be false.<br>
+for i in `seq 0 4`; do<br>
+    ovs-appctl time/warp 100<br>
+    CFM_CHECK_DB([p0], [false], [], [0], [], [], [up])<br>
+done<br>
+<br>
+# advance clock to pass the fault check timeout and check cfm<br>
+# status update in OVSDB.<br>
+for i in `seq 0 14`; do ovs-appctl time/warp 100; done<br>
+CFM_CHECK_DB([p0], [true], [recv], [1], [], [], [up])<br>
+<br>
+# remove the cfm on p0 and status should be all empty.<br>
+AT_CHECK([ovs-vsctl remove int p0 cfm_mpid 1])<br>
+for i in `seq 0 4`; do ovs-appctl time/warp 100; done<br>
+CFM_CHECK_DB([p0], [[[]]], [], [[[]]], [], [], [[[]]])<br>
+<br>
+OVS_VSWITCHD_STOP<br>
+AT_CLEANUP<br>
+<br>
+# Test cfm status update in normal case.<br>
+AT_SETUP([cfm - check update ovsdb 2])<br>
 #Create 2 bridges connected by patch ports and enable cfm<br>
 OVS_VSWITCHD_START([add-br br1 -- \<br>
                     set bridge br1 datapath-type=dummy \<br>
@@ -61,13 +92,18 @@ ovs-appctl time/stop<br>
 AT_CHECK([ovs-vsctl set Interface p0 cfm_mpid=1])<br>
 # check cfm status update in OVSDB.<br>
 for i in `seq 0 14`; do ovs-appctl time/warp 100; done<br>
-CFM_CHECK_DB([p0], [recv], [1], [], [], [up])<br>
+CFM_CHECK_DB([p0], [true], [recv], [1], [], [], [up])<br>
<br>
-# turn cfm on p1 off, should increment the cfm_flap_count on p0.<br>
+# turn cfm on p1 on, cfm status of p0 and p1 should all go up.<br>
 AT_CHECK([ovs-vsctl set interface p1 cfm_mpid=2])<br>
 for i in `seq 0 14`; do ovs-appctl time/warp 100; done<br>
-CFM_CHECK_DB([p0], [], [2], [], [2], [up])<br>
-CFM_CHECK_DB([p1], [], [0], [], [1], [up])<br>
+CFM_CHECK_DB([p0], [false], [], [2], [], [2], [up])<br>
+CFM_CHECK_DB([p1], [false], [], [0], [], [1], [up])<br>
+<br>
+# turn cfm on p1 off, cfm status of p0 should go down again.<br>
+AT_CHECK([ovs-vsctl remove int p1 cfm_mpid 2])<br>
+for i in `seq 0 14`; do ovs-appctl time/warp 100; done<br>
+CFM_CHECK_DB([p0], [true], [recv], [3], [], [], [up])<br>
<br>
 OVS_VSWITCHD_STOP<br>
 AT_CLEANUP<br>
<span class="HOEnZb"><font color="#888888">--<br>
1.7.9.5<br>
<br>
</font></span></blockquote></div><br></div>