That is a bit strange, because the AndroidMetadata stage was originally from the PFQ to give us metadata about the Android version I thought (davidriley@ might remember this better).  Could this have happened due to the PFQ refactoring?

Probably related to the refactor.

But... what does this stage do?

It publishes the Android version information to a json, it does not do anything that should functionally change the build itself, though it does result in some Portage queries you might not otherwise see.

Adding nya who most recently made changes here and likely remembers a lot better than me.

According to https://chromium-review.googlesource.com/331809 AndroidMetadata was added for non-PFQ builds to ensure that data was available.  I think for PFQ builds, data was already available.  ARC++ data was originally added to metadata.json in the PFQ https://chromium-review.googlesource.com/328847 when I realized that the masters and slaves needed some way to share a version between them.

master-android-pfq runs AndroidMetadata stage when it actually uprevs android-container, but the stage is skipped when new version is not available.

For example, this build runs AndroidMetadata:
https://uberchromegw.corp.google.com/i/chromeos/builders/master-android-pfq/builds/3146

But not in this build:
https://uberchromegw.corp.google.com/i/chromeos/builders/master-android-pfq/builds/3147

This is due to ExitEarlyException raised in UprevAndroidStage:
https://chromium.googlesource.com/chromiumos/chromite/+/master/cbuildbot/stages/android_stages.py#91

If we want metadata in every PFQ run, we should avoid skipping AndroidMetadata stage somehow.

In this case perhaps we don't care about results from the PFQ builders so this is ok?

A PFQ run that determines the newest possible version is what we already have is not really that meaningful to track. 

Even in the build that ran AndroidMetadata (in c#5), the android-branch isn't populated, so it must run differently than the non-PFQ version.  This is the main piece of metadata that we need to disambiguate the PFQ runs by branch.  Currently, they are jumbled together.

Hmm, it seems like it is there but maybe not in the same place somehow?

https://uberchromegw.corp.google.com/i/chromeos/builders/reef-nyc-android-pfq/builds/117 

is an example Android PFQ builder that did run AndroidMetadata:
https://uberchromegw.corp.google.com/i/chromeos/builders/reef-nyc-android-pfq/builds/117/steps/AndroidMetadata/logs/stdio

it puts a metadata.json file up at:
https://pantheon.corp.google.com/storage/browser/chromeos-image-archive/reef-nyc-android-pfq/R60-9592.0.0-rc2


which contains:
...
"version": {"full": "R60-9592.0.0-rc2", "android-branch": "git_nyc-mr1-arc", "chrome": null, "platform": "9592.0.0-rc2", "milestone": "60", "android": "4046673"},
...

Is this the wrong place?

OK, sorry, I guess this is being populated only in the slaves, and the master never gives us the branch information (that's OK by us).  But, we would like the stage to be consistently run, not just on uprevs (since then when a version isn't uprevved, we wouldn't know what branch this particular version pertained to).

I think it should publish on any PFQ run which could have potentially uprevved, failure or not, it looks like it only skips if the PFQ run was a going to be a NOP (e.g. no new version available to try when we polled for a new build).

Looking at https://uberchromegw.corp.google.com/i/chromeos/builders/reef-nyc-android-pfq/builds/100 we run AndroidMetadata even though we failed. 

If we really need it we are back to Shuhei's analysis in comment 5, but it is not clear to me why GE would care in the cases where the PFQ is not doing anything by design. I guess this means GE would be inconsistent from the waterfall, which is not optimal. 

Shuhei, any ideas on how we would avoid skipping AndroidMetadata? 

I guess UprevAndroid could set a flag and AndroidMetadata could exit?

Well, I think leaving metadata consistently is a good idea because it allows easier analysis of build results, even if it is not actually consumed by GE.

I will take a look.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/6845ad096253f4264b714f06fe3fe7ebefe31d30

commit 6845ad096253f4264b714f06fe3fe7ebefe31d30
Author: Shuhei Takahashi <nya@chromium.org>
Date: Fri Jun 02 06:49:26 2017

cbuildbot: Clean up UprevAndroidStage.

Behavior should not change.

BUG=chromium:726499
TEST=cbuildbot --remote --nouprev master-android-pfq cyan-android-pfq

Change-Id: Ic24d92d6ac62782dcbb19560c818ae212664a725
Reviewed-on: https://chromium-review.googlesource.com/517606
Commit-Ready: Shuhei Takahashi <nya@chromium.org>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: David Riley <davidriley@chromium.org>

[modify] https://crrev.com/6845ad096253f4264b714f06fe3fe7ebefe31d30/cbuildbot/commands.py
[modify] https://crrev.com/6845ad096253f4264b714f06fe3fe7ebefe31d30/cbuildbot/stages/android_stages.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/35fc347045b10926a608cdd91c741625655626b5

commit 35fc347045b10926a608cdd91c741625655626b5
Author: Shuhei Takahashi <nya@chromium.org>
Date: Fri Jun 02 06:49:26 2017

cbuildbot: Clean up AndroidMetadataStage.

Behavior is mostly preserved, but now the branch name is read from
the builder config in Android PFQ master.

BUG=chromium:726499
TEST=cbuildbot --remote --nouprev master-android-pfq cyan-android-pfq

Change-Id: I2d0e25b068e1edf3ea92ec54650521173be4d1ca
Reviewed-on: https://chromium-review.googlesource.com/517607
Commit-Ready: Shuhei Takahashi <nya@chromium.org>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: David Riley <davidriley@chromium.org>

[modify] https://crrev.com/35fc347045b10926a608cdd91c741625655626b5/cbuildbot/stages/android_stages.py