opencompute-developers team mailing list archive

[Jeff Lane <jeffrey.lane@xxxxxxxxxxxxx>]

Review: Needs Fixing

Hi Nelson,

Thank you for breaking this into smaller pieces... it makes review a LOT easier.  Now, there are some things that need fixing.

I'll take them one file at a time:
data/whitelists/opencompute-certify-local.whitelist looks fine.

jobs/TC-001-0001-CPU_Memory.txt.in:
1: any job that calls a script that needs root permissions to run must include the 'user: root' definition.  See the file 'jobs/cpu.txt.in' for some examples where this is necessary.  When I run the script cpu_info without root, the cache data is not returned and the test will fail.  So this job needs 'user:root'
2: TC-001-0001-003-Memory_Information also needs 'user: root' added to properly run.

jobs/local.txt.in
1: The word "Verified" should be "Verify" in the description field.

scripts/cpu_info:
1: When I manually run the cpu_info script for the test TC-001-0001-001-CPU_information with and without root permissions, I get the following different outputs:
bladernr@klaatu:~/development/ocp-nelson-memory-cpu-test$ ./scripts/cpu_infoIntel(R) Core(TM) i7 CPU       Q 720  @ 1.60GHz
Can not found any CPU cache.
bladernr@klaatu:~/development/ocp-nelson-memory-cpu-test$ echo $?
30
bladernr@klaatu:~/development/ocp-nelson-memory-cpu-test$ sudo ./scripts/cpu_info
Intel(R) Core(TM) i7 CPU       Q 720  @ 1.60GHz
L1 Cache 32 KB
L2 Cache 256 KB
L3 Cache 6 MB
L3 cache size less than 20MB.
bladernr@klaatu:~/development/ocp-nelson-memory-cpu-test$ echo $?
50

I don't have a system that meets the test criteria, so it will always fail for me.  First, the explanation for failure should be more explicit.  Example: 'FAIL: Can not find any CPU cache.' for the first example.  This is more important in the second example where 'L3 cache size less than 20MB' looks like part of the lshw output.  It would be better more explicitly stated as 'FAIL: L3 cache size less than 20MB.'

2: You don't need all those explicit error codes.  Checkbox only knows and stores 0 and Not 0 exit codes.  A 0 exit code indicates a test passed.  A non-zero exit code indicates a failure.  Checkbox does not store the actual exit codes.  This is not necessarily something you need to change, but you DO need to be aware of the behaviour in case a script behaves differently than expected when you run it via checkbox.

3: The description and spec for this test says: "CPU model should belong to Intel Xeon processor E5-2600 family..." but your test does not fail on non-Xeon processors.  For example, when I comment out the error code return for my cache limit on my laptop, I get this output:
bladernr@klaatu:~/development/ocp-nelson-memory-cpu-test$ sudo ./scripts/cpu_info; echo $?
Intel(R) Core(TM) i7 CPU       Q 720  @ 1.60GHz
L1 Cache 32 KB
L2 Cache 256 KB
L3 Cache 6 MB
L3 cache size less than 20MB.
0

but my laptop should clearly fail the test case since it's not up to OCP spec.

4:  The output needs to be cleaned up a bit.  Sorry, I know English is a second language for you, so I'll try to help as much as I can. 
    "Can not parser" should be "Can not parse"
    "Can not found" should be "Can not find"
    "# Parser lshw XML for gather" should be "# Parse lshw XML for gathering"

scripts/memory_info:
1: Needs to be run as root, see comments above about the job description field.
2: Same comment as above about the English words.
3: Script does not seem to fail on my laptop.  The nice thing about checkbox is that it can give you a good idea at a glance of what failed or not without relying on a human to parse the results.
bladernr@klaatu:~/development/ocp-nelson-memory-cpu-test$ sudo ./scripts/memory_info 
SODIMM DDR3 Synchronous 1333 MHz (0.8 ns),
vendor=AMI,
slot=M1,
size=2GB.

SODIMM DDR3 Synchronous 1333 MHz (0.8 ns),
vendor=AMD,
slot=M2,
size=4GB.

bladernr@klaatu:~/development/ocp-nelson-memory-cpu-test$ echo $?
0

Note I do NOT meet the requirements for the test case which are: 2 DDR3 slots per channel per processor (total of 16 DIMMs on the motherboard) yet according to this, I still pass the test.

4: No need for the commas and period.  They just make the output confusing

scripts/processor_topology:
1: Same comment as above regarding commas and periods in output.  They're not necessary.
2: You basically took my cpu_topology script and modifed it slightly to provide different output.  But left the guts of my original but there is no fail criteria.  The test case says "should be 8 cores per CPU and 2 threads per core (that is, up to 16 threads with Hyper-Threading Technology per CPU".  But this script will only fail if it is unable to get data via lscpu.
So the only way this will fail is in the case where CPU topology doesnt map out correctly per my original.  This should also include failure criteria per the test case in the spec.
-- 
https://code.launchpad.net/~nelson-chu/opencompute/add-ocp-cpu-memory-job/+merge/196651
Your team Open Compute Developers is subscribed to branch lp:opencompute/checkbox.