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

Issue 699188 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 0
Type: Bug



Sign in to add a comment

hostinfo changes broke test_for_push

Project Member Reported by nxia@chromium.org, Mar 7 2017

Issue description

[chromeos-autotest.hot.corp.google.com] out: testbed_DummyTest   [ FAILED ]
[chromeos-autotest.hot.corp.google.com] out: testbed_DummyTest     FAIL: Unhandled AutoservError: The host android1758-row1-rack7-test-station-4.cros has no attribute job_repo_url_84B7N16625000441. `install_apk_from_build` only works for test with image specified.




Error logs:

http://shortn/_HUsgtIknoc
 
Status: Started (was: Untriaged)
This is the failing test:
http://shortn/_249TIk2CpO

What's wierd is that we _had_ the correct HostInfo _and_ the host attributes earlier in the logs:

03/07 09:43:49.180 DEBUG|         host_info:0202| New host_info: HostInfo [Labels: ['board:angler-1', 'loopback-dongle', 'camera-hal', 'board:angler-2', 'os:android', 'testbed', 'pool:bvt', 'testbed-version:git_oc-release/angler-userdebug/3771772#2'], Attributes: {'serials': '84B7N16625000441,84B7N16625000002', 'job_repo_url_84B7N16625000441': 'http://100.107.126.162:8082/static/git_oc-release/angler-userdebug/3771772', 'job_repo_url_84B7N16625000002': 'http://100.107.126.162:8082/static/git_oc-release/angler-userdebug/3771772'}


... but somehow we lost them.
03/07 09:44:05.167 DEBUG|         host_info:0193| Refreshing HostInfo using store <autotest_lib.server.hosts.host_info.InMemoryHostInfoStore object at 0x7f223f146190>
03/07 09:44:05.167 DEBUG|         host_info:0194| Old host_info: None
03/07 09:44:05.168 DEBUG|         host_info:0202| New host_info: HostInfo [Labels: [], Attributes: {}


This suggests that the server_job setup the correct HostInfoStore, but the one used from AdbHost is not the same one.
Found it: http://shortn/_lFL5gMUmxc

testbed creates adbhost without going through the factory methods, so it doesn't get the HostInfoStore correctly.
Labels: -Pri-3 Pri-0
This could potentially break testbed tests that need even labels (not just attributes).

But the only relevant CL since the last push can be reverted to unblock us: https://chromium-review.googlesource.com/c/450845/

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 7 2017

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

commit b9d7adf07aec05db6a3f1a8b74f45abd7a87c74a
Author: Prathmesh Prabhu <pprabhu@chromium.org>
Date: Tue Mar 07 19:14:48 2017

Revert "Use HostInfo to obtain host attributes"

This reverts commit 838f89f628e404bd3d657e5eb4b45c02bfb363b9.


Original change's description:
> Use HostInfo to obtain host attributes
> 
> BUG= chromium:678430 
> TEST=unittests
> 
> Change-Id: I4ae2d4ee0c19eae5545eeb9ff01a7fe8853bc34d
> Reviewed-on: https://chromium-review.googlesource.com/440366
> Commit-Ready: Prathmesh Prabhu <pprabhu@chromium.org>
> Tested-by: Prathmesh Prabhu <pprabhu@chromium.org>
> Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org>
> 

TBR=pprabhu@chromium.org,ayatane@chromium.org
BUG= chromium:678430 
BUG= chromium:699188 

Change-Id: I36b47500518ef4a710145f35f730e53d40cdb0a0
Reviewed-on: https://chromium-review.googlesource.com/450845
Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org>
Tested-by: Prathmesh Prabhu <pprabhu@chromium.org>

[modify] https://crrev.com/b9d7adf07aec05db6a3f1a8b74f45abd7a87c74a/server/site_tests/android_ACTS/android_ACTS.py
[modify] https://crrev.com/b9d7adf07aec05db6a3f1a8b74f45abd7a87c74a/server/afe_utils.py
[modify] https://crrev.com/b9d7adf07aec05db6a3f1a8b74f45abd7a87c74a/server/hosts/cros_host.py
[modify] https://crrev.com/b9d7adf07aec05db6a3f1a8b74f45abd7a87c74a/server/site_tests/autoupdate_EndToEndTest/autoupdate_EndToEndTest.py
[modify] https://crrev.com/b9d7adf07aec05db6a3f1a8b74f45abd7a87c74a/server/adb_utils.py
[modify] https://crrev.com/b9d7adf07aec05db6a3f1a8b74f45abd7a87c74a/server/cros/autoupdate_utils.py
[modify] https://crrev.com/b9d7adf07aec05db6a3f1a8b74f45abd7a87c74a/server/hosts/adb_host.py

http://chromeos-autotest.hot.corp.google.com/afe/#tab_id=view_job&object_id=6195


I manually updated the repos on push servers and cloned the failed job. It passed. So we're in the clear.

I've also manually kicked a test_push again.
Status: Fixed (was: Started)
Follow ups (why did this happen):

- We need testbed coverage in the CQ:  issue 699211 
- I must admit that there was some lapse of testing on my part: I test my changes against a CrOS DUT locally using a local autotest setup. I don't currently intend to add a testbed to that setup. Should I? Are we suggesting that all infra team members working on autotest test their changes against a local testbed?
- There's clearly lacking unittest coverage of the host classes.

Comment 8 by dshi@chromium.org, Mar 7 2017

Re #7

For such changes, usually I will borrow the test push environment, cherrypick the change to drone and run a test_push run.
I could start doing #8, but I think we don't want to encourage that. We often end up leaving the push environment in weird states that in the best case cause the following test_push to fail (happens almost every other week) and in the worst case silently pass in an environment that no longer reflects prod.
Status: Started (was: Fixed)
Not fixed.
We still see the same issue.

And guess what, this CL is in git log cros/prod..cros/master at this point: 

commit c239c0a22120c7a50177bb3243ec3e8267290b4f
Author: Dan Shi <dshi@google.com>
Date:   Wed Mar 1 21:50:54 2017 +0000

    Revert "[autotest] Disable testbed in test push before testbed is restored"
    
    This reverts commit ff92802456f39d7b939b841318021d65e38f3ff6.
    
    Change-Id: I49541bc19b2504e8bb3b3fd4619c1324722968f3
    Reviewed-on: https://chromium-review.googlesource.com/447855
    Tested-by: Dan Shi <dshi@google.com>
    Trybot-Ready: Dan Shi <dshi@google.com>
    Reviewed-by: Shuqian Zhao <shuqianz@chromium.org>
    Commit-Queue: Dan Shi <dshi@google.com>


So, we just restarted testbed testing in test_push after a couple weeks. So, my assumption that the bad CL had to be something since last push is incorrect.
It could be an older CL.
Also, this means that it is possible testbed testing is currently broken in prod....?

At this point, it's better to just chump the fix that I already have in the CQ: https://chromium-review.googlesource.com/c/450866/
Status: Fixed (was: Started)
False alarm. This one is really issue 699254
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 8 2017

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

commit 8456cb61eea2e6e9173f457ba351755876da8fce
Author: Prathmesh Prabhu <pprabhu@chromium.org>
Date: Wed Mar 08 00:18:22 2017

[autotest] Pass in HostInfoStore from TestBed to it's children

BUG= chromium:699188 
BUG= chromium:678430 
TEST=unittests, test_push passes.

Change-Id: I08c75151e8b8e2aeaac76ae1151dcb1759d8c935
Reviewed-on: https://chromium-review.googlesource.com/450866
Commit-Ready: Prathmesh Prabhu <pprabhu@chromium.org>
Tested-by: Prathmesh Prabhu <pprabhu@chromium.org>
Reviewed-by: Dan Shi <dshi@google.com>

[modify] https://crrev.com/8456cb61eea2e6e9173f457ba351755876da8fce/server/hosts/testbed.py

Sign in to add a comment