Merge ~ltrager/maas:lp1695229 into maas:master

Proposed by Lee Trager
Status: Merged
Approved by: Lee Trager
Approved revision: d98df423956d58e9060d94e18302765d009ec7f9
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~ltrager/maas:lp1695229
Merge into: maas:master
Diff against target: 84 lines (+21/-6)
2 files modified
src/metadataserver/builtin_scripts/badblocks.py (+9/-2)
src/metadataserver/builtin_scripts/tests/test_badblocks.py (+12/-4)
Reviewer Review Type Date Requested Status
Andres Rodriguez (community) Approve
Review via email: mp+326204@code.launchpad.net

Commit message

Run badblocks using the physical blocksize of the disk.

By default badblocks always runs using a blocksize of 1024 and uses a 32bit int
for the number of blocks it can test. For large disks the number of blocks
exceeds the 32bit int limit and causes badblocks to fail. Many large uses use
a larger physical blocksize which avoids hitting the 32bit int limit which
helps avoid hitting LP:1695229.

To post a comment you must log in.
Revision history for this message
Andres Rodriguez (andreserl) :
review: Needs Information
Revision history for this message
Lee Trager (ltrager) :
Revision history for this message
Andres Rodriguez (andreserl) :
review: Needs Information
Revision history for this message
Lee Trager (ltrager) :
Revision history for this message
Andres Rodriguez (andreserl) wrote :

What is the TIMEOUT current? What's the value for it ?

review: Needs Information
Revision history for this message
Andres Rodriguez (andreserl) wrote :

And is this set for this particular test or is it a global variable for all tests?

Revision history for this message
Lee Trager (ltrager) wrote :

Just that one command.

Revision history for this message
Andres Rodriguez (andreserl) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/metadataserver/builtin_scripts/badblocks.py b/src/metadataserver/builtin_scripts/badblocks.py
2index eba78ed..46e9111 100644
3--- a/src/metadataserver/builtin_scripts/badblocks.py
4+++ b/src/metadataserver/builtin_scripts/badblocks.py
5@@ -48,10 +48,17 @@ class RunBadBlocks(Thread):
6 self.returncode = None
7
8 def run(self):
9+ blocksize = check_output(
10+ ['sudo', '-n', 'blockdev', '--getbsz', self.drive['PATH']],
11+ timeout=TIMEOUT, stderr=DEVNULL).strip()
12 if self.destructive:
13- cmd = ['sudo', '-n', 'badblocks', '-v', '-w', self.drive['PATH']]
14+ cmd = [
15+ 'sudo', '-n', 'badblocks', '-b', blocksize, '-v', '-w',
16+ self.drive['PATH']]
17 else:
18- cmd = ['sudo', '-n', 'badblocks', '-v', '-n', self.drive['PATH']]
19+ cmd = [
20+ 'sudo', '-n', 'badblocks', '-b', blocksize, '-v', '-n',
21+ self.drive['PATH']]
22 # Run badblocks and capture its output. Once all threads have completed
23 # output the results serially so output is proerly grouped.
24 with Popen(cmd, stdout=PIPE, stderr=STDOUT) as proc:
25diff --git a/src/metadataserver/builtin_scripts/tests/test_badblocks.py b/src/metadataserver/builtin_scripts/tests/test_badblocks.py
26index 6e08dde..6b3c922 100644
27--- a/src/metadataserver/builtin_scripts/tests/test_badblocks.py
28+++ b/src/metadataserver/builtin_scripts/tests/test_badblocks.py
29@@ -127,6 +127,7 @@ class TestSmartCTL(MAASTestCase):
30 drive = self.make_drive(with_path=True)
31 run_bad_blocks = badblocks.RunBadBlocks(drive, True)
32 output = factory.make_string()
33+ self.patch(badblocks, 'check_output').return_value = '4096'
34 mock_popen = self.patch(badblocks, 'Popen')
35 mock_popen.return_value = Popen(['echo', '-n', output], stdout=PIPE)
36
37@@ -137,13 +138,16 @@ class TestSmartCTL(MAASTestCase):
38 self.assertThat(
39 mock_popen,
40 MockCalledOnceWith(
41- ['sudo', '-n', 'badblocks', '-v', '-w', drive['PATH']],
42- stdout=PIPE, stderr=STDOUT))
43+ [
44+ 'sudo', '-n', 'badblocks', '-b', '4096', '-v', '-w',
45+ drive['PATH'],
46+ ], stdout=PIPE, stderr=STDOUT))
47
48 def test_run_nondestructive(self):
49 drive = self.make_drive(with_path=True)
50 run_bad_blocks = badblocks.RunBadBlocks(drive, False)
51 output = factory.make_string()
52+ self.patch(badblocks, 'check_output').return_value = '4096'
53 mock_popen = self.patch(badblocks, 'Popen')
54 mock_popen.return_value = Popen(['echo', '-n', output], stdout=PIPE)
55
56@@ -154,8 +158,10 @@ class TestSmartCTL(MAASTestCase):
57 self.assertThat(
58 mock_popen,
59 MockCalledOnceWith(
60- ['sudo', '-n', 'badblocks', '-v', '-n', drive['PATH']],
61- stdout=PIPE, stderr=STDOUT))
62+ [
63+ 'sudo', '-n', 'badblocks', '-b', '4096', '-v', '-n',
64+ drive['PATH'],
65+ ], stdout=PIPE, stderr=STDOUT))
66
67 def test_run_badblocks(self):
68 drive = {
69@@ -166,6 +172,7 @@ class TestSmartCTL(MAASTestCase):
70 }
71 self.patch(badblocks, 'list_drives').return_value = [drive]
72 output = factory.make_string()
73+ self.patch(badblocks, 'check_output').return_value = '4096'
74 mock_popen = self.patch(badblocks, 'Popen')
75 mock_popen.return_value = Popen(['echo', '-n', output], stdout=PIPE)
76 mock_print = self.patch(badblocks, 'print')
77@@ -192,6 +199,7 @@ class TestSmartCTL(MAASTestCase):
78 }
79 self.patch(badblocks, 'list_drives').return_value = [drive]
80 output = factory.make_string()
81+ self.patch(badblocks, 'check_output').return_value = '4096'
82 mock_popen = self.patch(badblocks, 'Popen')
83 mock_popen.return_value = Popen(
84 'echo -n %s; exit 1' % output, stdout=PIPE, shell=True)

Subscribers

People subscribed via source and target branches