Merge ~kissiel/plainbox-provider-checkbox:apste-support-job into plainbox-provider-checkbox:master

[Maciej Kisielewski]

add job that checks APSTE availabilty on NVMe drives

I decided to go with device.driver == 'nvme' and
device.category == 'OTHER'" template-filter to make sure only one job is
generated (per controller), and that the template will be given nvmeN name (N being 0, 1..), so the check for pm_qos file would be possible. If I'd narrow it down to category == 'DISK' then the drives would be reported, like nvme0n1, which is fine for the nvme cli tool, but it would require some obscure tweaking to get 'the parent' for the sysfs check.

To test it, use a machine with an NVMe drive.
It should fail on generic kernel, to make it pass you have to use this one:
 http://people.canonical.com/~khfeng/xenial-nvme-apst/

[Jeff Lane  (bladernr)]

You linked the wrong bug :)

[Maciej Kisielewski (kissiel)]

> You linked the wrong bug :)
Thanks!
What's funny is I did not link any bug :D LP Must have deduced that from my long commit message.

[Maciej Kisielewski (kissiel)]

> > You linked the wrong bug :)
> Thanks!
> What's funny is I did not link any bug :D LP Must have deduced that from my
> long commit message.

Ugh. Actually it says my MR has unmerged commits that include yours 9067c12. That's why.

[Pierre Equoy (pieq)]

I added two comments below.

Also, we should discuss about integrating this job into the certification test plan if needed.

review: Needs Fixing

[Maciej Kisielewski (kissiel)]

> I added two comments below.
>
> Also, we should discuss about integrating this job into the certification test
> plan if needed.

Thanks for the feedback!
I fixed both things.

review: Needs Resubmitting

[Jeff Lane  (bladernr)]

I'm not sure why, but you have somehow built this on my LXD branch (which is already merged). Look at the unmerged commits in your branch:

b2147f0... by Maciej Kisielewski on 2017-03-29
add job that checks APSTE availabilty on NVMe drives

I decided to go with "device.bus == 'nvme' and device.driver == 'nvme' and
device.category == 'OTHER'" template-filter to make sure only one job is
generated (per controller), and that the template will be given nvmeN name (N
being 0, 1..), so the check for pm_qos file would be possible. If I'd narrow it
down to category == 'DISK' then the drives would be reported, like nvme0n1,
which is fine for the nvme cli tool, but it would require some obscure tweaking
to get 'the parent' for the sysfs check.

64f1e79... by Jeff Lane on 2017-03-28
PEP8 cleanup

9067c12... by Jeff Lane on 2017-03-28
More refinements to LXD tests LP: #1677003

5a1c4ff... by Jeff Lane on 2017-02-13
Forgot to remove the import of pdb from debugging

1b1a07e... by Jeff Lane on 2017-02-11
fixed regression in kvm test description caused by my changes for LXD tests

023ff20... by Jeff Lane on 2017-02-10
jobs/virtualization.txt.in: added lxd job

ab9dba5... by Jeff Lane on 2017-02-10
pep8 fixes

37e9340... by Jeff Lane on 2017-02-10
refactored lxd test code

4d890cd... by Jeff Lane on 2017-02-09
Working LXD test now present.

2411e0c... by Jeff Lane on 2017-02-01
First working version of LXD test

review: Needs Fixing

[Jeff Lane  (bladernr)]

Sylvain thinks it wont hurt anything, so just an FYI in case you meant
to link this to your own tracking bug

On Mon, Apr 3, 2017 at 9:32 AM, Jeff Lane <email address hidden> wrote:
> Review: Needs Fixing
>
> I'm not sure why, but you have somehow built this on my LXD branch (which is already merged). Look at the unmerged commits in your branch:
>
>
>
> b2147f0... by Maciej Kisielewski on 2017-03-29
> add job that checks APSTE availabilty on NVMe drives
>
> I decided to go with "device.bus == 'nvme' and device.driver == 'nvme' and
> device.category == 'OTHER'" template-filter to make sure only one job is
> generated (per controller), and that the template will be given nvmeN name (N
> being 0, 1..), so the check for pm_qos file would be possible. If I'd narrow it
> down to category == 'DISK' then the drives would be reported, like nvme0n1,
> which is fine for the nvme cli tool, but it would require some obscure tweaking
> to get 'the parent' for the sysfs check.
>
> 64f1e79... by Jeff Lane on 2017-03-28
> PEP8 cleanup
>
> 9067c12... by Jeff Lane on 2017-03-28
> More refinements to LXD tests LP: #1677003
>
> 5a1c4ff... by Jeff Lane on 2017-02-13
> Forgot to remove the import of pdb from debugging
>
> 1b1a07e... by Jeff Lane on 2017-02-11
> fixed regression in kvm test description caused by my changes for LXD tests
>
> 023ff20... by Jeff Lane on 2017-02-10
> jobs/virtualization.txt.in: added lxd job
>
> ab9dba5... by Jeff Lane on 2017-02-10
> pep8 fixes
>
> 37e9340... by Jeff Lane on 2017-02-10
> refactored lxd test code
>
> 4d890cd... by Jeff Lane on 2017-02-09
> Working LXD test now present.
>
> 2411e0c... by Jeff Lane on 2017-02-01
> First working version of LXD test
> --
> https://code.launchpad.net/~kissiel/plainbox-provider-checkbox/+git/plainbox-provider-checkbox/+merge/321439
> You are reviewing the proposed merge of ~kissiel/plainbox-provider-checkbox:apste-support-job into plainbox-provider-checkbox:master.
>
> Launchpad-Message-Rationale: Reviewer
> Launchpad-Message-For: bladernr
> Launchpad-Notification-Type: code-review
> Launchpad-Branch: ~kissiel/plainbox-provider-checkbox/+git/plainbox-provider-checkbox:apste-support-job
> Launchpad-Project: plainbox-provider-checkbox

--
"Entropy isn't what it used to be."

Jeff Lane -
Server Certification Lead, Warrior Poet, Biker, Lover of Pie
Phone: [phone]
Ubuntu Ham: W4KDH Freenode IRC: bladernr or bladernr_
gpg: 1024D/3A14B2DD 8C88 B076 0DD7 B404 1417 C466 4ABD 3635 3A14 B2DD

[Maciej Kisielewski (kissiel)]

Managed to reset && rebase on a freshly fetched master.
Seems fine now \o/

review: Needs Resubmitting

[Sylvain Pineau (sylvain-pineau)]

In the checkbox-support parser tests database, there is a system (DELL_POWEREDGE_R820_NVME.txt) where the controller does not get a category. I've checked why and it's not on the nvme subsystem but on misc:

P: /devices/pci0000:40/0000:40:03.0/0000:44:00.0/0000:45:04.0/0000:46:00.0/misc/nvme0
N: nvme0
E: DEVNAME=/dev/nvme0
E: DEVPATH=/devices/pci0000:40/0000:40:03.0/0000:44:00.0/0000:45:04.0/0000:46:00.0/misc/nvme0
E: MAJOR=10
E: MINOR=57
E: SUBSYSTEM=misc

I'm working on a fix for the parser to report them as well bit your selection requirement would need an update.
Will device.driver == 'nvme' and device.category == 'OTHER' enough to get only the controllers?

With my work in progress fix, you could get:

path: /devices/pci0000:40/0000:40:03.0/0000:44:00.0/0000:45:04.0/0000:46:00.0/misc/nvme0
name: nvme0
bus: misc
category: OTHER
driver: nvme
product_id: 43040
vendor_id: 5197
subproduct_id: 8087
subvendor_id: 4136
product: nvme0
vendor: Samsung Electronics Co Ltd
product_slug: nvme0
vendor_slug: Samsung_Electronics_Co_Ltd

As you can see the bus is set to misc which is a shame...

And a small nitpick, egrep is deprecated (see grep man page):

       In addition, the variant programs egrep, fgrep and rgrep are the same
       as grep -E, grep -F, and grep -r, respectively. These variants are
       deprecated, but are provided for backward compatibility.

I think you can safely use:

... | grep '(APSTE).*Enabled'

review: Needs Fixing

[Maciej Kisielewski (kissiel)]

Changed the `requires` field to device.driver == 'nvme' and device.category == 'OTHER'.
And s/egrep/\ grep/.

Amended. Pushed.

review: Needs Resubmitting

[Sylvain Pineau (sylvain-pineau)]

Looks ok, thanks for the fixes. On my side I've update the nvme controller detection in the udev parser (See https://code.launchpad.net/~sylvain-pineau/checkbox-support/+git/checkbox-support/+merge/321875).

+1

review: Approve

[Pierre Equoy (pieq)]

Tested on a device with one SATA HDD and one NVMe drive using the fix made by spineau in the udev parser, it works as expected.

+1

review: Approve

diff --git a/jobs/disk.txt.in b/jobs/disk.txt.in
index 8d22fb3..c1df7e3 100644
--- a/jobs/disk.txt.in
+++ b/jobs/disk.txt.in
@@ -1,3 +1,8 @@
+# This is for disk/apste_support_on_*
+unit: packaging meta-data
+os-id: debian
+Depends: nvme-cli
+
 plugin: shell
 category_id: 2013.com.canonical.plainbox::disk
 id: disk/detect
@@ -128,3 +133,13 @@ _description:
   The verification of this test is automated. Do not change the
   automatically selected result.
 
+unit: template
+template-resource: device
+template-filter: device.driver == 'nvme' and device.category == 'OTHER'
+plugin: shell
+category_id: 2013.com.canonical.plainbox::disk
+id: disk/apste_support_on_{name}
+estimated_duration: 1.0
+user: root
+command: nvme get-feature -f 0x0c -H /dev/{name} | grep '(APSTE).*Enabled' && test -e /sys/class/nvme/{name}/power/pm_qos_latency_tolerance_us
+_summary: Check support for Autonomous Power State Transition on {name}