Merge lp:~rodsmith/checkbox/smart-crash-fix into lp:checkbox

[Rod Smith]

Fixes for a disk_smart script crash when unexpected output encountered. Also, increased the timeout value because a WD 4TB disk was taking 10% too long for the old timeout value.

Daniel: I think I fixed that glitch, which looks like a weird bzr thing.

[Daniel Manrique (roadmr)]

Looks like some merge conflict markers ran away and made their way into the text; I see them (see comment below) but the merge is not marked as conflicting.

review: Needs Fixing

[Daniel Manrique (roadmr)]

One further comment...

review: Needs Information

[Daniel Manrique (roadmr)]

Hey Rod,

I've been circling around this MR and I really don't think catching CalledProcessError here would solve it. Maybe the same exception was raised elsewhere in the code? This would be very strange: CalledProcessError is only raised by check_call or check_output, each of which is used only once in the code, and both have proper exception handling. There are two uses of old-stype Popen/communicate but that doesn't raise the same exception.

With that in mind, though I really hate to reject merge requests, I'll do so with this one, particularly since you also said the disk was a bit flaky. Could you please submit the timeout increase separately?

Thanks and sorry for the wait.

review: Disapprove

[Rod Smith (rodsmith)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/10/2015 02:48 PM, Daniel Manrique wrote:
> Review: Disapprove
>
> Hey Rod,
>
> I've been circling around this MR and I really don't think
> catching CalledProcessError here would solve it. Maybe the same
> exception was raised elsewhere in the code? This would be very
> strange: CalledProcessError is only raised by check_call or
> check_output, each of which is used only once in the code, and both
> have proper exception handling. There are two uses of old-stype
> Popen/communicate but that doesn't raise the same exception.
>
> With that in mind, though I really hate to reject merge requests,
> I'll do so with this one, particularly since you also said the
> disk was a bit flaky.

OK, no problem. The bug was difficult to reproduce, and the test fails
either way, so it's a matter of having a more helpful error message
when that happens.

> Could you please submit the timeout increase separately?

Done:

https://code.launchpad.net/~rodsmith/checkbox/smart-increase-timeout/+merge/252634

- --
Rod Smith
Server and Cloud Certification Engineer
<email address hidden>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJVAIumAAoJEFgyRI+V0FjmE3gH/0/y5Lirm1XFzzVkBJElXxnY
fMGSX5IUK6m5hzrmEt2YUMBH2oDPPom92+mGLNZymFh8R+PvtEFAVu7+44QYjzU5
4UN71Ph0O7JoqGMfvbRKC802XKpLySgXZ+uhqd35vM7JQWUuw/WoJz2EXkYuOkZM
S+eAbp+9PYtW5t+BIkJxuRm6myp2scX7vygBPcEX/1Q5FW8Fy/3956C/1sALi1rv
sEz4qOX2NwYwiCOcUzTvRTaDApNFvft1gfxFwBjkCSmbeZ9zlnjKIgn0bIhVIT3C
1WjRY8azA2p3q5Wuhus484xrEr85PPuX1n4pBGLqmWNnpoKcV3SP/mLlf6UBp3s=
=UJes
-----END PGP SIGNATURE-----

Unmerged revisions

3607. By Rod Smith on 2015-03-06

Fix weird bzr merge problem

3606. By Rod Smith on 2015-03-06

Fixes for a disk_smart script crash when unexpected output encountered. Also, increased the timeout value because a WD 4TB disk was taking 10% too long for the old timeout value.

=== modified file 'providers/plainbox-provider-checkbox/bin/disk_smart'
--- providers/plainbox-provider-checkbox/bin/disk_smart	2015-02-13 16:22:22 +0000
+++ providers/plainbox-provider-checkbox/bin/disk_smart	2015-03-06 22:57:00 +0000
@@ -218,7 +218,7 @@
 # (and then move it to the top once the tests are done).
 def poll_for_status(args, disk, previous_entries):
     # Priming read... this is here in case our test is finished or fails
-    # immediate after it beginsAccording to.
+    # immediate after it begins.
     logging.debug('Polling selftest.log for status')
     keep_going = True
 
@@ -302,7 +302,13 @@
         run_smart_test(disk)
         previous_entries, returncode = get_smart_entries(disk)
 
-    status, returncode = poll_for_status(args, disk, previous_entries)
+    try:
+        status, returncode = poll_for_status(args, disk, previous_entries)
+    except CalledProcessError as err:
+        logging.error("FAIL: error code ", err)
+        logging.error("Run 'sudo smartctl -l selftest %s' to see the SMART log",
+                      disk)
+        return 1
 
     if returncode != 0:
         log, returncode = get_smart_entries(disk)

=== modified file 'providers/plainbox-provider-checkbox/jobs/disk.txt.in'
--- providers/plainbox-provider-checkbox/jobs/disk.txt.in	2015-03-05 19:56:28 +0000
+++ providers/plainbox-provider-checkbox/jobs/disk.txt.in	2015-03-06 22:57:00 +0000
@@ -65,7 +65,7 @@
  description:
   This tests the SMART capabilities for $product (Note that this test will not work against hardware RAID)
  user: root
- command: disk_smart -b /dev/`ls /sys$path/block | sed 's|!|/|'` -s 130 -t 270
+ command: disk_smart -b /dev/`ls /sys$path/block | sed 's|!|/|'` -s 130 -t 400
  EOF
 
 plugin: local