Merge lp:~sylvain-pineau/checkbox/add_removable_attribute_to_udev_resource into lp:checkbox

[Sylvain Pineau]

After reviewing several systems which reported disk_read_performance_test failures after running the SRU tests, it turns out that the usb stick we use is unable to reach the usb threshold defined in scripts/disk_read_performance_test.

An easy fix is to lower this value to 7 MB/s.

Another proposal is to run disks.txt.in jobs only on non-removable devices, this is the purpose of the changes to udev_resource and its corresponding parser, add a new attribute we could use in job requirements.

I'm still not sure about what to do for removable devices, duplicating the read_performance test in their related sections (usb, firewire, etc...), add benchmarks-like tests to removable_storage_test or just rely on the current version of removable_storage_test.

So several solutions for one problem, that's why i'm requesting your reviews.

Thanks.

[Daniel Manrique (roadmr)]

I don't think we should do benchmarking on removable devices (yet; we may have to when we start testing USB 3.0). The reason is that this is very dependent on the device itself, I've seen USB sticks with transfer rates as low as 1 MB/s, especially cheap ones. On the other hand, performance for internal hard drives is pretty much standard so we *can* benchmark those more reliably.

[Marc Tardif (cr3)]

I not sure I agree with extending the udev_resource script just for the sake of the removable_storage_test script. I can appreciate the need for this information but I question the way it is exposed. The "removable" property provided in the output looks like the concept of "capabilities" in HAL. Instead of being implemented as individual attributes, this is implemented as a list of values where you'd have something like:

  capabilities: removable

This could be reused for optical devices to look like:

  capabilities: cdrom-readable cdrom-writable dvd-readable dvd-writable

Instead, with your approach, you would need an attribute for each capability:

  cdrom-readable: supported
  cdrom-writable: supported
  dvd-readable: supported
  dvd-writable: supported

Both approaches would work equally well to reach the ultimate objective of expressing requirements, but it seems that capabilities would provide a more generic way of solving the same problem. However, note that the reason capabilities were never implemented is that the names of the actual capabilities might not be universal. For example, the name of the capability to define whether an optical device is readable could be any of: cdromread, cdrom-read, cdread, cd-readable, or any upper case variation. In other words, nobody could guess the name of a capability without actually reading the source code.

So, what to do from here? To move this merge request forward, I'd suggest putting a bit more thought into the changes made to udev_resource so that it might apply to other devices as well. First, I'd suggest thinking about optical devices as described above. Second, I'd also suggest looking outside Checkbox, like facter that is also used for gathering system information by Puppet. What do you think?

review: Needs Information

[Daniel Manrique (roadmr)]

Just a comment, we already have a optical_drive resource that tells if the CD and/or DVD readers are "writable" or "readonly". Job definition is as follows:

name: optical_drive
plugin: resource
command: for media in CD DVD; do wodim -prcap 2>/dev/null | grep -q -i "Does write $media" && echo "$media: writable" || echo "$media: readonly"; done
description: Create resource info for supported optical actions

[Sylvain Pineau (sylvain-pineau)]

I've created lp:~sylvain-pineau/checkbox/disk_tests_on_internal_drives_only to not hack into udev_resources and propose something similar to optical_drives resources.

So I'd prefer to reject this one.

Sylvain

Unmerged revisions

1396. By Sylvain Pineau on 2012-05-25

Add the removable attribute to udev_resource and a new requirement to all jobs in disk.txt.in to only run on non-removable devices.

=== modified file 'checkbox/parsers/udevadm.py'
--- checkbox/parsers/udevadm.py	2012-02-28 21:26:55 +0000
+++ checkbox/parsers/udevadm.py	2012-05-25 14:29:20 +0000
@@ -377,6 +377,18 @@
 
         return None
 
+    @property
+    def removable(self):
+        if self._environment.get("DEVTYPE") == "disk":
+            devpath = self._environment.get("DEVPATH")
+            with open("/sys%s/removable" %devpath) as f:
+                if f.read(1) == '1':
+                    return u'supported'
+                else:
+                    return u'unsupported'
+
+        return None
+
 
 class UdevadmParser:
     """Parser for the udevadm command."""
 
=== modified file 'jobs/disk.txt.in'
--- jobs/disk.txt.in	2012-02-29 08:33:20 +0000
+++ jobs/disk.txt.in	2012-05-25 14:29:20 +0000
@@ -11,7 +11,7 @@
  cat <<'EOF' | run_templates -t -s 'udev_resource | filter_templates -w "category=DISK"'
  plugin: shell
  name: disk/benchmark_`ls /sys$path/block`
- requires: device.path == "$path" and package.name == 'linux'
+ requires: device.path == "$path" and device.removable == 'unsupported' and package.name == 'linux'
  user: root
  command: hdparm -tT /dev/`ls /sys$path/block | sed 's|!|/|'` | sed 's/:.*= */ = /' | grep -v "^$"
  description: This test runs hdparm timing tests as a benchmark for $path
@@ -25,7 +25,7 @@
  cat <<'EOF' | run_templates -t -s 'udev_resource | filter_templates -w "category=DISK"'
  plugin: shell
  name: disk/stats_`ls /sys$path/block`
- requires: device.path == "$path" and package.name == 'linux'
+ requires: device.path == "$path" and device.removable == 'unsupported' and package.name == 'linux'
  user: root
  command: disk_stats_test `ls /sys$path/block | sed 's|!|/|'`
  description: This test checks disk stats, generates some activity and rechecks stats to verify they've changed. It also verifies that disks appear in the various files they're supposed to.
@@ -42,9 +42,9 @@
  cat <<'EOF' | run_templates -t -s 'udev_resource | filter_templates -w "category=DISK"'
  plugin: shell
  name: disk/smart_`ls /sys$path/block`
- requires: device.path == "$path"
+ requires: device.path == "$path" and device.removable == 'unsupported' 
  description:
-  This tests the SMART capabilities for $product (Note that this test will not work against removable media (USB, Firewire) and hardware RAID)
+  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
  EOF
@@ -58,7 +58,7 @@
  cat <<'EOF' | run_templates -t -s 'udev_resource | filter_templates -w "category=DISK"'
  plugin: shell
  name: disk/max_diskspace_used_`ls /sys$path/block`
- requires: device.path == "$path"
+ requires: device.path == "$path" and device.removable == 'unsupported' 
  description: Maximum disk space test for $product
  user: root
  command: max_diskspace_used `ls /sys$path/block | sed 's|!|/|'`
@@ -75,6 +75,7 @@
  name: disk/read_performance_`ls /sys$path/block`
  requires:
   device.path == "$path"
+  device.removable == 'unsupported' 
   package.name == 'linux'
  description: Disk performance test for $product
  user: root
@@ -93,6 +94,7 @@
  user: root
  requires:
   device.path == "$path"
+  device.removable == 'unsupported' 
   package.name == 'linux'
  description: Disk I/O stress test for $product
  command: storage_test `ls /sys$path/block | sed 's|!|/|'`
 
=== modified file 'scripts/disk_read_performance_test'
--- scripts/disk_read_performance_test	2011-06-24 12:43:06 +0000
+++ scripts/disk_read_performance_test	2012-05-25 14:29:20 +0000
@@ -10,7 +10,7 @@
   disk_type=`udevadm info --name /dev/$disk --query property | grep "ID_BUS" | awk '{gsub(/ID_BUS=/," ")}{printf $1}'`
 
   case $disk_type in
-    "usb" ) MIN_BUF_READ=15;; #Custom metrics are guesstimates for now...
+    "usb" ) MIN_BUF_READ=7;; #Custom metrics are guesstimates for now...
     "ide" ) MIN_BUF_READ=40;;
     *     ) MIN_BUF_READ=$DEFAULT_BUF_READ;;
   esac
 
=== modified file 'scripts/udev_resource'
--- scripts/udev_resource	2011-10-11 20:15:01 +0000
+++ scripts/udev_resource	2012-05-25 14:29:20 +0000
@@ -30,7 +30,8 @@
 class UdevResult(object):
 
     attributes = ("path", "bus", "category", "driver", "product_id",
-        "vendor_id", "subproduct_id", "subvendor_id", "product", "vendor",)
+        "vendor_id", "subproduct_id", "subvendor_id", "product", "vendor",
+        "removable",)
 
     def addDevice(self, device):
         for attribute in self.attributes: