Merge ~ltrager/maas:lp1733923_2.3 into maas:2.3

[Lee Trager]

Backport a03af41 from master - LP: #1733923 - Fail badblocks if any error is detected

[Lee Trager (ltrager)]

review: Approve

diff --git a/src/metadataserver/builtin_scripts/badblocks.py b/src/metadataserver/builtin_scripts/badblocks.py
index 6dfeca9..292b7fd 100644
--- a/src/metadataserver/builtin_scripts/badblocks.py
+++ b/src/metadataserver/builtin_scripts/badblocks.py
@@ -31,6 +31,18 @@
 #   badblocks:
 #     title: Bad blocks
 #     description: The number of bad blocks found on the storage device.
+#   read_errors:
+#     title: Bad blocks read errors
+#     description: >
+#       The number of bad blocks read errors found on the storage device.
+#   write_errors:
+#     title: Bad blocks write errors
+#     description: >
+#       The number of bad blocks write errors found on the storage device.
+#   comparison_errors:
+#     title: Bad blocks comparison errors
+#     description: >
+#       The number of bad blocks comparison errors found on the storage device.
 # parameters:
 #   storage: {type: storage}
 # destructive: {{if 'destructive' in name}}True{{else}}False{{endif}}
@@ -113,37 +125,56 @@ def run_badblocks(storage, destructive=False):
     if destructive:
         cmd = [
             'sudo', '-n', 'badblocks', '-b', str(blocksize),
-            '-c', str(parallel_blocks), '-v', '-f', '-w', storage
+            '-c', str(parallel_blocks), '-v', '-f', '-s', '-w', storage
         ]
     else:
         cmd = [
             'sudo', '-n', 'badblocks', '-b', str(blocksize),
-            '-c', str(parallel_blocks), '-v', '-f', '-n', storage
+            '-c', str(parallel_blocks), '-v', '-f', '-s', '-n', storage
         ]
 
     print('INFO: Running command: %s\n' % ' '.join(cmd))
     proc = Popen(cmd, stdout=PIPE, stderr=STDOUT)
     stdout, _ = proc.communicate()
+    stdout = stdout.decode()
 
     # Print stdout to the console.
-    if stdout is not None:
-        print(stdout.decode())
-
-    result_path = os.environ.get("RESULT_PATH")
+    print(stdout)
+
+    m = re.search(
+        '^Pass completed, (?P<badblocks>\d+) bad blocks found. '
+        '\((?P<read>\d+)\/(?P<write>\d+)\/(?P<comparison>\d+) errors\)$',
+        stdout, re.M)
+    badblocks = int(m.group('badblocks'))
+    read_errors = int(m.group('read'))
+    write_errors = int(m.group('write'))
+    comparison_errors = int(m.group('comparison'))
+    result_path = os.environ.get('RESULT_PATH')
     if result_path is not None:
-        # Parse the results for the desired information and
-        # then wrtie this to the results file.
-        match = re.search(b', ([0-9]+) bad blocks found', stdout)
-        if match is not None:
-            results = {
-                'results': {
-                    'badblocks': int(match.group(1).decode()),
-                }
+        results = {
+            'results': {
+                'badblocks': badblocks,
+                'read_errors': read_errors,
+                'write_errors': write_errors,
+                'comparison_errors': comparison_errors,
             }
-            with open(result_path, 'w') as results_file:
-                yaml.safe_dump(results, results_file)
-
-    return proc.returncode
+        }
+        with open(result_path, 'w') as results_file:
+            yaml.safe_dump(results, results_file)
+
+    # LP: #1733923 - Badblocks normally returns 0 no matter the result. If any
+    # errors are found fail the test.
+    if (proc.returncode + badblocks + read_errors + write_errors +
+            comparison_errors) != 0:
+        print('FAILURE: Test FAILED!')
+        print('INFO: %s badblocks found' % badblocks)
+        print('INFO: %s read errors found' % read_errors)
+        print('INFO: %s write errors found' % write_errors)
+        print('INFO: %s comparison errors found' % comparison_errors)
+        return 1
+    else:
+        print('SUCCESS: Test PASSED!')
+        return 0
 
 
 if __name__ == '__main__':
diff --git a/src/metadataserver/builtin_scripts/tests/test_badblocks.py b/src/metadataserver/builtin_scripts/tests/test_badblocks.py
index a04c33c..66f6b25 100644
--- a/src/metadataserver/builtin_scripts/tests/test_badblocks.py
+++ b/src/metadataserver/builtin_scripts/tests/test_badblocks.py
@@ -9,7 +9,6 @@ __all__ = []
 import io
 import os
 import random
-import re
 from subprocess import (
     PIPE,
     STDOUT,
@@ -18,22 +17,22 @@ from textwrap import dedent
 from unittest.mock import ANY
 
 from maastesting.factory import factory
-from maastesting.matchers import (
-    MockCalledOnce,
-    MockCalledOnceWith,
-)
+from maastesting.matchers import MockCalledOnceWith
 from maastesting.testcase import MAASTestCase
 from metadataserver.builtin_scripts import badblocks
 import yaml
 
 
 BADBLOCKS = random.randint(0, 1000)
+READ_ERRORS = random.randint(0, 1000)
+WRITE_ERRORS = random.randint(0, 1000)
+COMPARISON_ERRORS = random.randint(0, 1000)
 BADBLOCKS_OUTPUT = dedent("""
     Checking for bad blocks in non-destructive read-write mode
     From block 0 to 5242879
     Testing with random pattern:
-    Pass completed, %s bad blocks found. (0/0/%s errors)
-    """ % (BADBLOCKS, BADBLOCKS))
+    Pass completed, %s bad blocks found. (%s/%s/%s errors)
+    """ % (BADBLOCKS, READ_ERRORS, WRITE_ERRORS, COMPARISON_ERRORS))
 
 
 class TestRunBadBlocks(MAASTestCase):
@@ -75,7 +74,7 @@ class TestRunBadBlocks(MAASTestCase):
         })
         cmd = [
             'sudo', '-n', 'badblocks', '-b', str(blocksize),
-            '-c', str(parallel_blocks), '-v', '-f', '-n', storage
+            '-c', str(parallel_blocks), '-v', '-f', '-s', '-n', storage
         ]
         mock_popen = self.patch(badblocks, "Popen")
         proc = mock_popen.return_value
@@ -88,10 +87,13 @@ class TestRunBadBlocks(MAASTestCase):
         results = {
             'results': {
                 'badblocks': BADBLOCKS,
+                'read_errors': READ_ERRORS,
+                'write_errors': WRITE_ERRORS,
+                'comparison_errors': COMPARISON_ERRORS,
             }
         }
 
-        self.assertEquals(0, badblocks.run_badblocks(storage))
+        self.assertEquals(1, badblocks.run_badblocks(storage))
         self.assertThat(mock_popen, MockCalledOnceWith(
             cmd, stdout=PIPE, stderr=STDOUT))
         self.assertThat(mock_open, MockCalledOnceWith(ANY, "w"))
@@ -107,7 +109,7 @@ class TestRunBadBlocks(MAASTestCase):
             parallel_blocks)
         cmd = [
             'sudo', '-n', 'badblocks', '-b', str(blocksize),
-            '-c', str(parallel_blocks), '-v', '-f', '-w', storage,
+            '-c', str(parallel_blocks), '-v', '-f', '-s', '-w', storage,
         ]
         mock_popen = self.patch(badblocks, "Popen")
         proc = mock_popen.return_value
@@ -116,27 +118,6 @@ class TestRunBadBlocks(MAASTestCase):
         proc.returncode = 0
 
         self.assertEquals(
-            0, badblocks.run_badblocks(storage, destructive=True))
+            1, badblocks.run_badblocks(storage, destructive=True))
         self.assertThat(mock_popen, MockCalledOnceWith(
             cmd, stdout=PIPE, stderr=STDOUT))
-
-    def test_run_badblocks_exits_if_no_regex_match_found(self):
-        storage = factory.make_name('storage')
-        blocksize = random.randint(512, 4096)
-        self.patch(badblocks, 'get_block_size').return_value = blocksize
-        parallel_blocks = random.randint(1, 50000)
-        self.patch(badblocks, 'get_parallel_blocks').return_value = (
-            parallel_blocks)
-        self.patch(os, "environ", {
-            "RESULT_PATH": factory.make_name()
-        })
-        mock_popen = self.patch(badblocks, "Popen")
-        proc = mock_popen.return_value
-        proc.communicate.return_value = (
-            BADBLOCKS_OUTPUT.encode('utf-8'), None)
-        proc.returncode = 0
-        mock_re_search = self.patch(re, "search")
-        mock_re_search.return_value = None
-
-        self.assertEquals(0, badblocks.run_badblocks(storage))
-        self.assertThat(mock_re_search, MockCalledOnce())