I suspect the easiest future-proof solution would be to pass the canonical 'mastername' property from the recipe to the script via a command line argument. The python classnames are an implementation detail of the way that chrome infra's internal configuration for buildbot works (and none of that implementation will be preserved after the migration)

For clarification for Ashley and I, we are talking about the chromium_utils.GetActiveMaster() call in the current results_dashboard.py script: 

https://cs.chromium.org/chromium/build/scripts/slave/results_dashboard.py?q=GetActiveMaster+file:%5Ebuild/scripts/slave/+package:%5Echromium$&dr=C&l=185

How many different masters upload to the perf dashboard?

I think you are right, we should probably start passing it as a flag to the upload script.  

Before src side upload script ( crbug.com/758632 ) we should be able to do that in chromium tests in the args section: https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_tests/steps.py?l=1722.  We will have to figure out a way to hard code that somewhere. 

This is similar to a problem we have with the perf id, crbug.com/793346.   I think we are planning to hard code the perf id/master name in the args section of merge script json arg, but before that exists we are going to have to brainstorm since the args section of the isolated_scripts gets lost after the trigger. 

Adding this as a blocker for luci migration.  

I raised this bug's priority to P1 because it block LUCI migration. Can someone who is familiar with this script takes this bug?

Simon & Emily explained to me the context of the problem. I will take this bug

The plan is to add perf_dashboard_mastername https://docs.google.com/document/d/1tsNATiZl00MnR_Gm-4gPo5TXLi8imSwZ9vPLdn755pU/edit# to the build properties.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/eba45707a4e59ec9808647b137d8aab9e5f91d1d

commit eba45707a4e59ec9808647b137d8aab9e5f91d1d
Author: Nghia Nguyen <nednguyen@google.com>
Date: Wed Jan 31 19:51:00 2018

Add options to set perf-dashboard-mastername for perf dashboard upload script

This will later allow us to update the recipes which use this script to plumb
the perf_dashboard_mastername directly if the builder is LUCI.

For more details of why builder using LUCI need to set perf_dashboard_mastername
explictly, see: bit.ly/perf-mastername

Bug:801289
Change-Id: I6871a2b3e08435097f351004cbb92f964e86810d
Reviewed-on: https://chromium-review.googlesource.com/894363
Reviewed-by: Simon Hatch <simonhatch@chromium.org>
Reviewed-by: Emily Hanley <eyaich@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Edward Lemur <ehmaldonado@google.com>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/eba45707a4e59ec9808647b137d8aab9e5f91d1d/scripts/slave/unittests/results_dashboard_test.py
[modify] https://crrev.com/eba45707a4e59ec9808647b137d8aab9e5f91d1d/scripts/slave/unittests/runtest_test.py
[modify] https://crrev.com/eba45707a4e59ec9808647b137d8aab9e5f91d1d/scripts/slave/upload_perf_dashboard_results.py
[modify] https://crrev.com/eba45707a4e59ec9808647b137d8aab9e5f91d1d/scripts/slave/results_dashboard.py
[add] https://crrev.com/eba45707a4e59ec9808647b137d8aab9e5f91d1d/scripts/slave/unittests/upload_perf_dashboard_results_test.py
[modify] https://crrev.com/eba45707a4e59ec9808647b137d8aab9e5f91d1d/scripts/slave/runtest.py
[modify] https://crrev.com/eba45707a4e59ec9808647b137d8aab9e5f91d1d/scripts/slave/recipe_modules/webrtc/resources/upload_to_perf_dashboard.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/758515bfb12619d5bfdb1217cddc2cbbcf30d5d7

commit 758515bfb12619d5bfdb1217cddc2cbbcf30d5d7
Author: Nghia Nguyen <nednguyen@google.com>
Date: Thu Feb 01 22:47:47 2018

Rename perf_dashboard_master_name to perf_dashboard_machine_group

The concept is "masters" will be obsolete, so perf dashboard team has decided
to use the term "machine_group" to refers to a logical group of
machines/bots that upload to perf dashboard instead.

This CL update the variable naming and flag of scripts/slave/unittests/upload_perf_dashboard_results_test.py to reflect that change.

Bug:801289
Change-Id: Ia30c3bf8f35ae5d01671250e823058135432db3c
Reviewed-on: https://chromium-review.googlesource.com/897907
Reviewed-by: Simon Hatch <simonhatch@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Emily Hanley <eyaich@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/758515bfb12619d5bfdb1217cddc2cbbcf30d5d7/scripts/slave/recipe_modules/webrtc/resources/upload_to_perf_dashboard.py
[modify] https://crrev.com/758515bfb12619d5bfdb1217cddc2cbbcf30d5d7/scripts/slave/unittests/results_dashboard_test.py
[modify] https://crrev.com/758515bfb12619d5bfdb1217cddc2cbbcf30d5d7/scripts/slave/upload_perf_dashboard_results.py
[modify] https://crrev.com/758515bfb12619d5bfdb1217cddc2cbbcf30d5d7/scripts/slave/results_dashboard.py
[modify] https://crrev.com/758515bfb12619d5bfdb1217cddc2cbbcf30d5d7/scripts/slave/unittests/upload_perf_dashboard_results_test.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/1808c689d48cc4deff8888780a5fc7dd7c8b1d0f

commit 1808c689d48cc4deff8888780a5fc7dd7c8b1d0f
Author: Nghia Nguyen <nednguyen@google.com>
Date: Fri Feb 02 02:00:00 2018

Update chromium_tests steps to set --perf-dashboard-mastername for perf dashboard upload script under LUCI

Bug:801289
Change-Id: Ib88e44a83f5aa06e79086ae9473cf0d47b6f96b1
Reviewed-on: https://chromium-review.googlesource.com/895393
Reviewed-by: Aaron Gable <agable@chromium.org>
Reviewed-by: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Simon Hatch <simonhatch@chromium.org>
Reviewed-by: Emily Hanley <eyaich@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/1808c689d48cc4deff8888780a5fc7dd7c8b1d0f/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_isolated_script_test.expected/histograms.json
[modify] https://crrev.com/1808c689d48cc4deff8888780a5fc7dd7c8b1d0f/scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_passed_isolated_script_perf_test_histograms.json
[modify] https://crrev.com/1808c689d48cc4deff8888780a5fc7dd7c8b1d0f/scripts/slave/README.recipes.md
[modify] https://crrev.com/1808c689d48cc4deff8888780a5fc7dd7c8b1d0f/scripts/slave/recipes/chromium.py
[rename] https://crrev.com/1808c689d48cc4deff8888780a5fc7dd7c8b1d0f/scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_passed_isolated_script_perf_test_failed_upload.json
[modify] https://crrev.com/1808c689d48cc4deff8888780a5fc7dd7c8b1d0f/scripts/slave/recipe_modules/chromium_tests/steps.py
[add] https://crrev.com/1808c689d48cc4deff8888780a5fc7dd7c8b1d0f/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_isolated_script_test.expected/histograms_LUCI_missing_perf_dashboard_machine_group_property.json
[modify] https://crrev.com/1808c689d48cc4deff8888780a5fc7dd7c8b1d0f/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_isolated_script_test.py

I think most of the recipe code is ready for LUCI build. We won't know for sure how well this work until we validate this path on a actual perf LUCI builder, so I leave this bug open for now.

Awesome thank you for taking a stab at this :)

Is there a way to eliminate the rest of the calls to GetActiveMaster (https://cs.chromium.org/search/?q=GetActiveMaster&sq=package:chromium&type=cs)? We should be able to access the mastername property in all current recipes (even on buildbot) so I think it should be possible before actually switching to LUCI.

Though maybe I misunderstood the code :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/d7e2523f7fb6428cca68fa403ec68b6c4fbb2ef3

commit d7e2523f7fb6428cca68fa403ec68b6c4fbb2ef3
Author: Ashley Enstad <ashleymarie@chromium.org>
Date: Fri Feb 02 18:35:02 2018

Removing unused dependency

We no longer use chromium_utils here so removing it from the file

Bug:801289
Change-Id: Ib9fa6dfad69ffb57d7d55d30dbb501f16f0a629d
Reviewed-on: https://chromium-review.googlesource.com/899503
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Ashley Enstad <ashleymarie@chromium.org>

[modify] https://crrev.com/d7e2523f7fb6428cca68fa403ec68b6c4fbb2ef3/scripts/slave/results_dashboard.py

#13: I can add a manual JSON that map from mastername --> GetActiveMaster() in recipe code to get rid of GetActiveMaster() once & for all. 

If you & nodir@ are happy with this approach, I am happy to make patch work.

I think that would be an awesome step! That means that the buildbot+luci runs of the recipe would behave identically (better feedback earlier), and we could get rid of this code for real :)

Waiting for nodir@ to confirm the approach.

ashley/eyaich: if I go forward with this, it means that I will remove "--luci" flag & always pass the perf_dashboard_master_name to the upload script directly in the recipe side. 

I thought we renamed perf_dashboard_master_name to perf_dashboard_machine_group. If so, a constant {master_name -> perf_dashboard_machine_group} dict in perf-specific recipe code SGTM.

 nodir@: sorry, it was a typo. I meant perf_dashboard_machine_group. Though there multiple recipes that use the upload script: https://cs.chromium.org/search/?q=upload_perf_dashboard_results.py+f:py&type=cs

What would be the recommended way to host the  {master_name -> perf_dashboard_machine_group} dict? Should I create put it in some common recipe module?

I am not sure I agree with this approach.  We were trying to move perf specific code out of the recipe.

Regardless of that line of thought, I don't think a hard coded map in the recipe is the right decision.  It isn't a property of the framework running the test, it is a property of the group of machines that the tests run on, ie the name of the group.  That in my mind is a property of LUCI where the groups exist.

I like the idea of having it as a property in the luci configs.  The migration of perf to LUCI is going pretty quickly, it wouldn't be that long before we can officially deprecate getActiveMaster

If that's the preferred approach, then I would recommend adding the property to the buildbot configs and doing as you suggest (again, no need to wait for LUCI). It'll require a master restart (b/c buildbot), but otherwise will get plumbed through the recipe the same way the LUCI properties work.

(and in general I agree with the premise; the less hardcoded maps inside of recipes the better. The immediate issue is removing the dependency on the hardcoded map inside of buildbot (i.e. GetActiveMaster) for one that is portable to LUCI. To that end I would view moving the map to recipes to be an improvement over the status quo, but I wouldn't expect it to be a good end-state)

If you just want to kill GetActiveMaster before LUCI migration, eyaich@ & ashlymarie@ are actively working on migrating the upload script to src/ side. This would require hard code the map in the upload script, allowing us to kill GetActiveMaster by then

So I will leave it to them to finish the rest of this bug :-)

That sgtm too! (though really the ultimate fix would be to switch to using the canonical master names... might it make sense to have the hard-coded mapping live in the appengine app, switch to using canonical names from the bots, then when no more APIs are being hit with the old names do a schema migration on the appengine app and then delete the mapping?)

Ok this is currently blocking the migration of bots running the new perf recipe to the main waterfall since we currently hard code this value in for the chromium.perf.fyi waterfall.  

Therefore, I am proposing the following solution since we don't have a way to call chromium_utils.GetActiveMaster() on src side: 

Before LUCI migration: 
I will hard code this json mapping (https://docs.google.com/document/d/1PaWaIgpFoJ_dduixyncauI0-qBiMz2PWxrfdB2WD3fI/edit) on src side and key into it with build_properties['mastername']. 

After LUCI migration: 
I am assuming that build_properties['perf_dashboard_machine_group'] will exist since we only have the build properties on merge script?  Ned can you confirm where this value will surface after the LUCI migration?

#26:
> I am assuming that build_properties['perf_dashboard_machine_group'] will exist since we only have the build properties on merge script?  Ned can you confirm where this value will surface after the LUCI migration?

Correct. This value will surface in api.properties.get('perf_dashboard_machine_group'). We have code to sure that all our LUCI builder config have this field properly set.

* I also think  hard code the json mappingon src side and key into it with build_properties['mastername'] is the right call before LUCI migration.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d494fd1b37de9f1cb7a0e2fdd4ed94a901f2c859

commit d494fd1b37de9f1cb7a0e2fdd4ed94a901f2c859
Author: Emily Hanley <eyaich@google.com>
Date: Mon Apr 09 23:50:04 2018

Plumbing through hard coded perf_dashboard_machine_group before luci conversion

Moving perf bots to the main waterfall is currently blocked on this master name
mapping.  Hard coding mapping until new property is available.

NOTRY=true  # CQ flake

Bug: 801289
Change-Id: I5a2af121f0c1f59a13fb280eeb6e1cc7681be7ea
Reviewed-on: https://chromium-review.googlesource.com/1002912
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Ashley Enstad <ashleymarie@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Cr-Commit-Position: refs/heads/master@{#549326}
[add] https://crrev.com/d494fd1b37de9f1cb7a0e2fdd4ed94a901f2c859/tools/perf/core/perf_dashboard_machine_group_mapping.json
[modify] https://crrev.com/d494fd1b37de9f1cb7a0e2fdd4ed94a901f2c859/tools/perf/core/upload_results_to_perf_dashboard.py
[modify] https://crrev.com/d494fd1b37de9f1cb7a0e2fdd4ed94a901f2c859/tools/perf/process_perf_results.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/ccdd71cae2e6bce819a6d8f8b1dcaa27a9e31141

commit ccdd71cae2e6bce819a6d8f8b1dcaa27a9e31141
Author: Nghia Nguyen <nednguyen@google.com>
Date: Wed Aug 15 15:10:43 2018

Make sure we use perf_dashboard_machine_group from build properties when it's available

This is part of the effort to get rid of relying on python class master name ( chromium_utils.GetActiveMaster())
and for migrating to LUCI

BUG:801289,  874358 

Change-Id: I6f02554a3360bef3fc5d89eb089efc07898b21af
Reviewed-on: https://chromium-review.googlesource.com/1175434
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Reviewed-by: Simon Hatch <simonhatch@chromium.org>
Commit-Queue: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/ccdd71cae2e6bce819a6d8f8b1dcaa27a9e31141/scripts/slave/unittests/runtest_test.py
[modify] https://crrev.com/ccdd71cae2e6bce819a6d8f8b1dcaa27a9e31141/scripts/slave/runtest.py