New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 793526 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocked on:
issue 812064
issue 817530
issue 818764



Sign in to add a comment

Add network_WlanDriver or similar to board/family-specific pre-cqs

Project Member Reported by kirtika@google.com, Dec 9 2017

Issue description

OS: R64

What steps will reproduce the problem?
* Submit a CL that breaks wifi .. i.e. no interface comes up

What is the expected result?
* pre-cq should reject it 

What happens instead?
* pre-cq succeeds 

Example: 
https://chromium-review.googlesource.com/#/c/817903/ is a merge that breaks wifi. pre-cq succeeded on glados, oak, cyan, 



 
Owner: kirtika@chromium.org
Pre-cq normally doesn't run HW tests, so that can't work. It could work to include this on the wificell-pre-cq though. You might also get by with putting it in a bvt suite. (bvt-cq?)

And I suppose this *is* one of the few Wifi tests that could run on non-wificells, because it doesn't actually test the functionality... but there's also very few things this test would catch. Are you sure it would catch a "no interface" problem? You'd probably just hit the NA result here:

        if device_obj is None:
            raise error.TestNAError('Found no recognized wireless device')
We can include this in wificell-pre-cq and also add it to bvt-prebuild (to start w/ before adding to bvt-cq). bvt-perbuild does not block builds or CLs but it will give us a better idea of how this test performs across all chromeos boards before we add it to bvt-cq.
LGTM.

We should also understand the scope of this. Will bvt-<foo> run on any devices that don't have Wifi? Or, don't have Wifi that goes through the same review process as most Chromebooks (e.g., any jetstream?)?

I think the "no wifi" case is fine; we'll give TEST_NA. But the latter could be annoying.

Or, I guess we can observe this all on bvt-perbuild. So might as well do it.

Comment 6 by kirtia@google.com, Jan 29 2018

Cc: -kirtia@chromium.org kirtika@google.com
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/780db3faf8783843e9e2b12b31b0bd3e30899a16

commit 780db3faf8783843e9e2b12b31b0bd3e30899a16
Author: Brian Norris <briannorris@chromium.org>
Date: Tue Jan 30 22:54:16 2018

network_WlanDriver: add to bvt-perbuild

We're considering whether we want this test (or a similar one) on the
CQ, to prevent massive breakage to Wifi drivers. While this test
certainly doesn't test for a lot, it should give us some basic sanity
checks.

BUG= chromium:793526 
TEST=watch bvt runs

Change-Id: I50f3c2335b4d02ca1ceb8ee2e963817626164a0b
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/890050
Reviewed-by: Kirtika Ruchandani <kirtika@chromium.org>

[modify] https://crrev.com/780db3faf8783843e9e2b12b31b0bd3e30899a16/client/site_tests/network_WlanDriver/control

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/e93b4bac4030f3e73f94088057e093e1e0b8b6bc

commit e93b4bac4030f3e73f94088057e093e1e0b8b6bc
Author: Brian Norris <briannorris@chromium.org>
Date: Tue Jan 30 22:54:16 2018

network_WlanDriver: make it fatal if no devices are found

We want to use this as a litmus test for basic driver support in the BVT
suite. But currently, it gives a Pass (TestNA) for a device with no
Wifi driver loaded. This is bad, since it doesn't catch one of various
failure modes:
(a) driver is not enabled, when it should be
(b) driver is poorly written and fails to initialize reliably
(c) other drivers (e.g., cfg80211.ko duplication) prevent our driver
    from loading

These are things we should catch.

Let's go with the following rule: if you disable shill's Wifi support
(USE="-wifi"), then we'll forgive you. But if you enable Wifi support,
we expect to find a supported Wifi device.

BUG= chromium:793526 
TEST=$subject test, on devices with
   (a) unsupported driver -> FAIL
   (b) supported driver, but removed (rmmod) -> FAIL
   (c) no shill/wifi support (USE="-wifi") -> TEST_NA
   (d) supported driver, properly initialized -> PASS

Change-Id: I54277d8ab12f2c65f7a2c7c9aa8cc79956d61e83
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/890121
Tested-by: Kirtika Ruchandani <kirtika@chromium.org>
Reviewed-by: Kirtika Ruchandani <kirtika@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/e93b4bac4030f3e73f94088057e093e1e0b8b6bc/client/site_tests/network_WlanDriver/network_WlanDriver.py
[modify] https://crrev.com/e93b4bac4030f3e73f94088057e093e1e0b8b6bc/client/cros/networking/shill_proxy.py

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 31 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/fd42714facf83d32e66efd97a27c07b0a67df9ee

commit fd42714facf83d32e66efd97a27c07b0a67df9ee
Author: Brian Norris <briannorris@chromium.org>
Date: Wed Jan 31 19:14:02 2018

network_WlanDriver: add to wificell-pre-cq suite

This is a useful sanity test, and we might be moving it to the CQ too.

pre-cq-configs: mixed-wificell-pre-cq

BUG= chromium:793526 
TEST=wificell-pre-cq

Change-Id: Ie91291436e9a662a9a45b484d5074232cb61b1a7
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/894726
Reviewed-by: Harpreet Grewal <harpreet@chromium.org>
Reviewed-by: Kirtika Ruchandani <kirtika@chromium.org>

[modify] https://crrev.com/fd42714facf83d32e66efd97a27c07b0a67df9ee/client/site_tests/network_WlanDriver/control

Blockedon: 809138
Blockedon: 812064
Blockedon: 812063
Blockedon: 817530
Blockedon: -812063
Blockedon: -809138
Cc: jrbarnette@chromium.org bhthompson@chromium.org
Alright, so we've had this on bvt-perbuild for almost a month. We've flushed out a few tiny bugs, and locked down/removed a few defective devices. You can view recent results on things like this:

https://cros-goldeneye.corp.google.com/chromeos/healthmonitoring/testDetails?testName=network_WlanDriver

https://stainless.corp.google.com/search?view=matrix&row=model&col=build&first_date=2018-02-22&last_date=2018-02-28&test=%5Enetwork%5C_WlanDriver%24&status=GOOD&status=WARN&status=FAIL&status=ERROR&status=TEST_NA&status=NOSTATUS&exclude_cts=true&exclude_not_run=false&exclude_non_release=false&exclude_au=true&exclude_acts=true&exclude_retried=true&exclude_non_production=true

The only recent failures I'm aware of are on nyan_kitty and reef. The reef failures are due to bug 817530 (likely defective unit). The nyan_kitty failures are legit failures, but that's because Marvell drivers/firmware are low-quality.

Bernie or Richard: do you have any suggestions on next steps, if we'd like to move this into the commit queue? From my reading of the suite descriptions, suite:bvt-cq would be the correct one. This test is short, doesn't require special lab resources, reliably indicates a bug in the product (unless we want to start making devices without Wifi), and in my opinion indicates the product is not fit for release (no Wifi = no good).

For the nyan_kitty-type problem: perhaps I should introduce a control.bvt that provides forgiveness for known-bad boards (or, e.g., to all Marvell-Wifi systems; or maybe just older ones)? Then the CQ can ignore it, but we can still track it in out-of-band tests.
It is best if a test can run on all possible devices, so optimally we fix the Marvell systems (as you say, no Wifi = no good), but given that may not be realistic, having a black list seems like a fine workaround.

Otherwise I think all you need to do is to mark the new test as a member of the bvt-cq or bvt-inline suite and then the CQ should pick it up.
> optimally we fix the Marvell systems (as you say, no Wifi = no good)

Of course. But for various reasons, that's becoming nearly impossible. Also, some of these problems may vary from DUT to DUT. The nastiest of bugs I've seen like this even vary from location to location -- e.g., because of the particular pattern of RF noise nearby.

> having a black list seems like a fine workaround

I'll probably do that, and then add this to bvt-cq, pending continued decent results on bvt-perbuild.
Project Member

Comment 19 by bugdroid1@chromium.org, Mar 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/e6df2ce60f4c2a744cf88465388b9fa033d43bb7

commit e6df2ce60f4c2a744cf88465388b9fa033d43bb7
Author: Brian Norris <briannorris@chromium.org>
Date: Sat Mar 03 03:52:00 2018

network_WlanDriver: add new .bvt version, with board exceptions

Some boards have Wifi which is unresolvably flaky, such that the driver
may just crash entirely and leave us with no Wifi device. We don't want
this flakiness to affect CQ results.

For now, we run this test only on bvt-perbuild (as we were doing
already), but we plan to move to bvt-cq soon.

The current exception list only contains nyan_kitty, because it's the
only lab device currently failing with unresolvable symptoms in the lab.
Occasionally, the device fails to load the mwifiex firmware correctly,
saying "FW failed to be active in time" after some long timeout. We've
debugged this issue before on some later platforms, but this requries
extensive vendor assistance, as the bug lies entirely within the
firmware.

I wouldn't be surprised if we had to add other Marvell-Wifi devices in
the future.

Also adjust the DOC wording to more accurately describe what it is
testing.

BUG= chromium:793526 
TEST=`test_that ... network_WlanDriver.bvt` with $BOARD in and out of
     exception list; with and without driver loaded; see WARN result for
     forgiven failures, GOOD for success

Change-Id: I8107f0d3b9a9d520009407a60dbdb43cf869b0e1
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/944502
Reviewed-by: Harpreet Grewal <harpreet@chromium.org>
Reviewed-by: Kirtika Ruchandani <kirtika@chromium.org>

[modify] https://crrev.com/e6df2ce60f4c2a744cf88465388b9fa033d43bb7/client/site_tests/network_WlanDriver/network_WlanDriver.py
[add] https://crrev.com/e6df2ce60f4c2a744cf88465388b9fa033d43bb7/client/site_tests/network_WlanDriver/control.bvt
[modify] https://crrev.com/e6df2ce60f4c2a744cf88465388b9fa033d43bb7/client/site_tests/network_WlanDriver/control

Blockedon: 818764
> [ ... ] From [ ... ] the suite descriptions, suite:bvt-cq would be the correct one.

Yup, if the test has a track record of no false alarms, then given the
described coverage, it qualifies for bvt-cq.  There's a case to be made
for bvt-inline, since lack of WiFi would impact any developer who wants
to manually test at a desktop using WiFi, and that feels like an "ordinary
development task".  I'm indifferent as to how you make that call.

Project Member

Comment 23 by bugdroid1@chromium.org, Mar 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/dcf298cf35a172f73c53b3db747317e039fc7941

commit dcf298cf35a172f73c53b3db747317e039fc7941
Author: Brian Norris <briannorris@chromium.org>
Date: Sat Mar 10 01:39:19 2018

network_WlanDriver: move to bvt-cq

Only failure in the last week was a snappy, which was a bad DUT (it's
now locked, with a lab-repair bug for investigation).

Setting JOB_RETRIES to 2 because the presubmits tell me to.

BUG= chromium:793526 
TEST=watch test results dashboards

Change-Id: I6fda1842e103ba2a5fb328e6141df32e5372be65
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/956497
Reviewed-by: Harpreet Grewal <harpreet@chromium.org>
Reviewed-by: Kirtika Ruchandani <kirtika@chromium.org>

[modify] https://crrev.com/dcf298cf35a172f73c53b3db747317e039fc7941/client/site_tests/network_WlanDriver/control.bvt

Status: Fixed (was: Started)
OK, we'll see how that goes.
Status: Verified (was: Fixed)

Sign in to add a comment