Remove Linux orchestrator builds that only kick off one other build and wait #55186

jmagman · 2024-09-13T19:15:41Z

"Orchestrator" builders are top-level builds that can perform some caching, kick off "drone" sub-builds, and then do things with those build artifacts like run tests, or "generators" that do work on those artifacts. See more details in https://flutter.dev/go/engine-build-definition-language.

Some orchestrators in ci/builders only kick off one sub-build. This is suboptimal because:

The orchestrator uses a bot that does nothing except essentially idle.
The orchestrator has some overhead, so the orchestrator + single drone sub-build takes longer than just the sub-build run alone.
The "drone" sub-build often has a generic name like "Linux Engine Drone" which makes it difficult to keep track of which build is which.

This PR:

Hoists a few of these Linux builds into ci/builders/standalone which are run as individual builds, and removes the orchestrator.
Swaps the recipe from engine_v2/engine_v2 (orchestrator) to engine_v2/builder (standalone).
Moves the drone_dimensions from the orchestrator build json into .ci.yaml.

Builder	Before	After
`Linux clangd`	9m & 2 Linux bots (drone build)	6m & 1 Linux bot
`Linux linux_android_emulator_skia_tests_34`	29m & 2 Linux bots (drone build)	25m & 1 Linux bot

Part of flutter/flutter#155041

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…in presubmit

jmagman · 2024-09-13T21:23:19Z

.ci.yaml

+    recipe: engine_v2/builder
    properties:
      config_name: linux_android_emulator_skia
+      kvm: "1"


Copied from

engine/ci/builders/linux_android_emulator_skia.json

Line 7 in 38e37ce

"kvm=1",

jmagman · 2024-09-13T21:23:39Z

.ci.yaml

+    recipe: engine_v2/builder
    properties:
      config_name: linux_android_emulator_skia_34
+      kvm: "1"


Copied from

engine/ci/builders/linux_android_emulator_skia_34.json

Line 7 in 38e37ce

"kvm=1",

jmagman · 2024-09-13T21:24:07Z

.ci.yaml

-    recipe: engine_v2/engine_v2
+    recipe: engine_v2/builder
    properties:
+      cores: "32"


Copied from

engine/ci/builders/linux_unopt_debug_no_rbe.json

Line 12 in 38e37ce

"drone_dimensions": ["device_type=none", "os=Linux", "cores=32"],

This is the magic "you get an emulator" right?

No, drone_dimensions in this case are properties that get passed to luci to decide which bots are eligible to run builds. If you open the "Infra" tab of a build you can see the input properties. For example

device_type=none means no physically tethered device (devicelab), os=Linux is self-explanatory, and cores=32 means the bot it runs on should have 32 cores.

Come to think of it though, I actually don't think the core=32 constraint is necessary for the clangd builder. It was introduced in #50901 and my guess is that it was copy/pasted from clang_tidy #43448.

Since this test runs really fast even on the default 8 core machine, I think we probably shouldn't use the more "valuable" 32-core bots (40 bots in the pool) vs the 8-core machines (240 in the pool). @matanlurey since this was a builder you created, wdyt?

I agree, I definitely blindly copied this without thinking.

jmagman · 2024-09-13T22:37:40Z

.ci.yaml

      - testing/skia_gold_client/**

  - name: Linux linux_android_emulator_skia_tests
    bringup: true


I tested that this one works by removing bringup: true in an intermediate commit.

jtmcdole

Thank you!

jtmcdole · 2024-09-13T22:57:01Z

.ci.yaml

-    recipe: engine_v2/engine_v2
+    recipe: engine_v2/builder
    properties:
+      cores: "32"


This is the magic "you get an emulator" right?

jtmcdole · 2024-09-13T23:03:50Z

ci/builders/standalone/linux_android_emulator_skia.json

+            "test_dependencies": [
+                {
+                    "dependency": "android_virtual_device",
+                    "version": "android_35_google_apis_x64.textpb"


Should this file encode the "35" nature in some way? "ci/android_emulator_skia_debug_x64" is duplicate, but "ci/android_emulator_skia_debug_x64/API35" is more descriptive

It took me having to copy these two files and diff them locally to spot the difference

I completely blindly copied one file to the other, the only difference is that I removed two levels of nesting:

engine/ci/builders/linux_android_emulator_skia_34.json

Lines 1 to 2 in 38e37ce

{

"builds": [

Like this:
595cdc2?diff=split&w=1

Can you file an issue if you'd like it changed? Someone from the Android team should think that through for future bumps https://github.com/flutter/engine/pull/54186/files#diff-ee89d2cc6e90e32301d49469c8a095fc1f92a33526471b2f71f13fea468fa225L77

zanderso

lgtm w/ Linux clangd on the default 8 core machine.

…er build and wait (flutter/engine#55186)

…155549) flutter/engine@61f0a3f...9bb0ece 2024-09-23 [email protected] Remove Linux orchestrator builds that only kick off one other build and wait (flutter/engine#55186) 2024-09-23 [email protected] Roll Skia from a402d3c60c16 to c20cce6273fb (1 revision) (flutter/engine#55359) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#155549) flutter/engine@61f0a3f...9bb0ece 2024-09-23 [email protected] Remove Linux orchestrator builds that only kick off one other build and wait (flutter/engine#55186) 2024-09-23 [email protected] Roll Skia from a402d3c60c16 to c20cce6273fb (1 revision) (flutter/engine#55359) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Remove some orchestrator builds that only kick off other builds

c14bd5b

jmagman self-assigned this Sep 13, 2024

jmagman added 4 commits September 13, 2024 12:17

Remove 'builds' key

595cdc2

Move drone requirements to .ci.yaml

9670b35

linux_android_emulator_skia_tests_34

87022f0

linux_android_emulator_skia

7584f33

jmagman changed the title ~~Remove some orchestrator builds that only kick off other builds~~ Remove orchestrator builds that only kick off one other build and wait Sep 13, 2024

jmagman changed the title ~~Remove orchestrator builds that only kick off one other build and wait~~ Remove Linux orchestrator builds that only kick off one other build and wait Sep 13, 2024

jmagman added 2 commits September 13, 2024 14:15

Temporarily remove linux_android_emulator_skia_tests bringup to test …

4a914fc

…in presubmit

Newlines

d3e3b1f

jmagman commented Sep 13, 2024

View reviewed changes

Revert bringup removal

e9a20ff

jmagman marked this pull request as ready for review September 13, 2024 22:36

jmagman commented Sep 13, 2024

View reviewed changes

jmagman requested review from jtmcdole and zanderso September 13, 2024 22:40

jtmcdole approved these changes Sep 13, 2024

View reviewed changes

matanlurey approved these changes Sep 14, 2024

View reviewed changes

zanderso approved these changes Sep 14, 2024

View reviewed changes

32->8 cores

ff2badd

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 23, 2024

auto-submit bot merged commit 9bb0ece into flutter:main Sep 23, 2024

jmagman deleted the orchestrator-removal branch September 23, 2024 16:28

engine-flutter-autoroll mentioned this pull request Sep 23, 2024

Roll Flutter Engine from 61f0a3fbabbe to 9bb0ece79ae2 (2 revisions) flutter/flutter#155549

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 23, 2024

9bb0ece79 Remove Linux orchestrator builds that only kick off one oth…

23b3735

…er build and wait (flutter/engine#55186)

Remove Linux orchestrator builds that only kick off one other build and wait #55186

Remove Linux orchestrator builds that only kick off one other build and wait #55186

Uh oh!

Conversation

jmagman commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtmcdole left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmagman commented Sep 13, 2024 •

edited

Loading

jmagman Sep 13, 2024 •

edited

Loading