Merge lp:~cr3/checkbox/sleep_test into lp:checkbox

Proposed by Marc Tardif
Status: Merged
Merged at revision: 1212
Proposed branch: lp:~cr3/checkbox/sleep_test
Merge into: lp:checkbox
Diff against target: 169 lines (+36/-40)
5 files modified
data/whitelists/default.whitelist (+1/-0)
debian/changelog (+1/-0)
jobs/suspend.txt.in (+10/-4)
scripts/network_wait (+20/-0)
scripts/sleep_test (+4/-36)
To merge this branch: bzr merge lp:~cr3/checkbox/sleep_test
Reviewer Review Type Date Requested Status
Daniel Manrique (community) Approve
Marc Tardif (community) Needs Resubmitting
Brendan Donegan (community) Needs Information
Review via email: mp+89471@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

Not sure why this code is being removed? In particular, why are you removing the check_network code? Sorry if it should be obvious.

review: Needs Information
Revision history for this message
Marc Tardif (cr3) wrote :

The check_network code is not doing anything:

1. Even though the function is being called and the return value assigned to the network_is_live variable, the value is never used so the outcome doesn't achieve anything:

  network_is_live = check_network()

2. Just in case, I checked whether the function had any side-effects, such as perhaps enabling the network in which case the name of the function would have to be changed, but it really does what the name of the function implies. So, it waits up to 60 seconds to check the network, returns True on success and False on failure. But, as stated above, that outcome never changes the outcome.

So, either the script should use the outcome for the check_network() function, assuming it is an atomic part of the sleep_test, or the function should be removed.

review: Needs Resubmitting
Revision history for this message
Daniel Manrique (roadmr) wrote :

I'd be all for removing this code if indeed it's not needed. But I think the stated purpose is useful:

"Be reasonably sure the network is up before returning, we want the network to be up for possible following tests."

If after one minute the network is not back up, then we give up as there's probably something wrong with it which we can't fix, and the network tests will catch it anyway.

In that case maybe the function should be renamed to make this clearer (e.g. wait_for_network_to_be_up_before_continuing()). Also, since the return value is not used for anything, it shouldn't be assigned to a variable to avoid confusion.

An alternative would be to just

 sleep(60) #Wait for network to come back up

though then we would waste some time if the connection comes back up within less than 60 seconds.

Revision history for this message
Marc Tardif (cr3) wrote :

What if the network connection doesn't come back up, is there any reason to ignore that situation?

Revision history for this message
Daniel Manrique (roadmr) wrote :

OK, so we talked about it for a bit, and we realized that the sleep_test shouldn't really concern itself with networking. The "did network come back up?" concern belongs in the network-after-suspend job; and all jobs that need a working connection *after* suspend should depend on that one (isn't it wonderful that dependencies are working?).

Expect some more work on that, and once that's done, this merge request can be accepted, as the post-suspend verification will be done elsewhere, where it makes a bit more semantic sense.

lp:~cr3/checkbox/sleep_test updated
1187. By Marc Tardif

Merged from trunk.

1188. By Marc Tardif

Added network_wait as standalone script.

Revision history for this message
Marc Tardif (cr3) wrote :

So, there is now a suspend/network_before_suspend to persist the state of the network before suspend, as should be obvious from the name of the job, and the previous suspend/network_after_suspend makes sure that the state after suspend is still the same. As discussed, the sleep_test script shouldn't concern itself with the state of the network so the logic to wait for the network to come up was extracted into the network_wait standalone script. This script is called in the suspend/network_after_suspend command.

Note that these changes will require adding suspend/network_before_suspend to the appropriate checkbox-certification whitelists, so expect another merge request.

review: Needs Resubmitting
Revision history for this message
Daniel Manrique (roadmr) wrote :

Awesome, so this simplifies things while still allowing for the network to take its time coming back up.

One question though: if the system is not connected to the internet, internet_test will fail, thus preventing even the suspend_advanced test from running. Would it be desirable to change it so that networkless systems will still be able to pass the test?

Revision history for this message
Marc Tardif (cr3) wrote :

Absolutely, the important thing is that the outcome of running internet_test is the same before and after suspending. I changed the redirect to use the tee command instead in suspend/network_before_suspend which always returns true.

review: Needs Resubmitting
lp:~cr3/checkbox/sleep_test updated
1189. By Marc Tardif

Modified suspend/network_before_suspend to always pass.

Revision history for this message
Daniel Manrique (roadmr) wrote :

Excellent, that was about the only remaining concern. So things are now where they belong. Thanks! I'll merge this and then we can deploy and try these changes.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/whitelists/default.whitelist'
2--- data/whitelists/default.whitelist 2012-01-31 19:35:41 +0000
3+++ data/whitelists/default.whitelist 2012-02-01 14:45:47 +0000
4@@ -125,6 +125,7 @@
5 suspend/bluetooth_detect_after_suspend
6 suspend/memory_before_suspend
7 suspend/memory_after_suspend
8+suspend/network_before_suspend
9 suspend/network_after_suspend
10 suspend/resolution_after_suspend
11 suspend/resolution_before_suspend
12
13=== modified file 'debian/changelog'
14--- debian/changelog 2012-01-31 20:04:09 +0000
15+++ debian/changelog 2012-02-01 14:45:47 +0000
16@@ -37,6 +37,7 @@
17 and context info (LP: #912546)
18 * Added support for skipping tests when the depends don't pass
19 (LP: #509598)
20+ * Removed extraneous code from the sleep_test.
21
22 [Daniel Manrique]
23 * Update control files under debian/ to eliminate (most) lintian warnings
24
25=== modified file 'jobs/suspend.txt.in'
26--- jobs/suspend.txt.in 2012-01-31 14:26:03 +0000
27+++ jobs/suspend.txt.in 2012-02-01 14:45:47 +0000
28@@ -1,4 +1,10 @@
29 plugin: shell
30+name: suspend/network_before_suspend
31+depends: networking/detect
32+_description: Record the current network before suspending.
33+command: internet_test | tee $CHECKBOX_DATA/network_before_suspend.txt
34+
35+plugin: shell
36 name: suspend/resolution_before_suspend
37 _description: Record the current resolution before suspending.
38 command: xrandr -q |grep '*'| awk '{print $1}' > $CHECKBOX_DATA/resolution_before_suspend.txt
39@@ -70,7 +76,7 @@
40
41 plugin: manual
42 name: suspend/suspend_advanced
43-depends: power-management/rtc networking/detect suspend/resolution_before_suspend suspend/wireless_before_suspend bluetooth/detect-output suspend/cpu_before_suspend suspend/memory_before_suspend
44+depends: power-management/rtc suspend/network_before_suspend suspend/resolution_before_suspend suspend/wireless_before_suspend bluetooth/detect-output suspend/cpu_before_suspend suspend/memory_before_suspend
45 requires: package.name == 'pm-utils'
46 user: root
47 command: sleep_test -d
48@@ -86,9 +92,9 @@
49
50 plugin: shell
51 name: suspend/network_after_suspend
52-depends: suspend/suspend_advanced networking/detect
53+depends: suspend/suspend_advanced networking/network_before_suspend
54 _description: Test the network after resuming.
55-command: internet_test
56+command: network_wait; internet_test | diff $CHECKBOX_DATA/network_before_suspend.txt -
57
58 plugin: shell
59 name: suspend/resolution_after_suspend
60@@ -247,7 +253,7 @@
61
62 plugin: shell
63 name: suspend/suspend_advanced_auto
64-depends: power-management/rtc networking/detect suspend/cpu_before_suspend suspend/memory_before_suspend suspend/wireless_before_suspend_auto
65+depends: power-management/rtc suspend/network_before_suspend suspend/cpu_before_suspend suspend/memory_before_suspend suspend/wireless_before_suspend_auto
66 requires:
67 package.name == 'pm-utils'
68 sleep.mem == 'supported'
69
70=== added file 'scripts/network_wait'
71--- scripts/network_wait 1970-01-01 00:00:00 +0000
72+++ scripts/network_wait 2012-02-01 14:45:47 +0000
73@@ -0,0 +1,20 @@
74+#!/bin/bash
75+
76+set -e
77+
78+x=1
79+while true; do
80+ state=$(/usr/bin/nmcli -t -f STATE nm)
81+ if [ "$state" = "connected" ]; then
82+ echo $state
83+ exit 0
84+ fi
85+
86+ x=$(($x + 1))
87+ if [ $x -gt 12 ]; then
88+ echo $state
89+ exit 1
90+ fi
91+
92+ sleep 5
93+done
94
95=== modified file 'scripts/sleep_test'
96--- scripts/sleep_test 2012-01-03 10:37:54 +0000
97+++ scripts/sleep_test 2012-02-01 14:45:47 +0000
98@@ -75,11 +75,9 @@
99
100 import sys
101 import logging
102-from subprocess import call,check_output
103+from subprocess import call
104 from optparse import OptionParser
105
106-from datetime import datetime, timedelta
107-from time import sleep
108
109 class ListDictHandler(logging.StreamHandler):
110 '''
111@@ -277,22 +275,6 @@
112 return False
113
114
115-def check_network():
116- start = datetime.now()
117- logging.debug("Waiting 60 seconds for NetworkManager to reconnect.")
118- while True:
119- try:
120- if check_output(["/usr/bin/nmcli", "-t", "-f",
121- "STATE", "nm"]).strip() == "connected":
122- return True
123- # give 60 seconds to NetworkManager to get to a CONNECTED state, then give up
124- if datetime.now() - start > timedelta(seconds=60):
125- return False
126- sleep(5)
127- except OSError as err:
128- logging.error("Unable to verify network status: %s" % err)
129- return False
130-
131 def main():
132 usage = 'Usage: %prog [OPTIONS]'
133 parser = OptionParser(usage)
134@@ -351,9 +333,7 @@
135 logging.debug(options_dict)
136
137 suspender = SuspendTest(options.max_sleep)
138- run_result = {}
139- run_count = 0
140-
141+
142 # Chcek fo the S3 state availability
143 if not suspender.CanWeSleep(options.mode):
144 logging.error('%s sleep state not supported' % options.mode)
145@@ -366,22 +346,10 @@
146 # Set new alarm time and suspend.
147 suspender.SetWakeTime(options.wake_time)
148 suspender.DoSuspend(options.mode)
149- run_count += 1
150 if suspender.CheckAlarm(options.mode):
151 logging.debug('The alarm is still set')
152-
153- run_result[run_count] = 'Pass'
154-
155- # Be reasonably sure the network is up before returning, we want
156- # the network to be up for possible following tests.
157- network_is_live = check_network()
158-
159- if 'Fail' in run_result.values():
160- logging.error('One or more suspend tests failed')
161- logging.error(run_result)
162- return 1
163- else:
164- return 0
165+
166+ return 0
167
168 if __name__ == '__main__':
169 sys.exit(main())

Subscribers

People subscribed via source and target branches