<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"><<a href="mailto:alexw@nicira.com" target="_blank">alexw@nicira.com</a>></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 <<a href="mailto:alexw@nicira.com">alexw@nicira.com</a>><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->status_changed = true;<br>
+}<br>
+<br>
/* Allocates a 'cfm' object called 'name'. 'cfm' 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(&cfm->ref_cnt);<br>
<br>
ovs_mutex_lock(&mutex);<br>
+ cfm_status_changed(cfm);<br>
cfm_generate_maid(cfm);<br>
hmap_insert(all_cfms, &cfm->hmap_node, hash_string(cfm->name, 0));<br>
ovs_mutex_unlock(&mutex);<br>
@@ -370,6 +379,7 @@ cfm_unref(struct cfm *cfm) OVS_EXCLUDED(mutex)<br>
}<br>
<br>
ovs_mutex_lock(&mutex);<br>
+ cfm_status_changed(cfm);<br>
hmap_remove(all_cfms, &cfm->hmap_node);<br>
ovs_mutex_unlock(&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->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 '/$2/p'],[0],<br>
+AT_CHECK([ovs-vsctl list interface $1 | sed -n '/$2 /p'],[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>