From 91e930930ddc20395125617a4805af523efae4b7 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Sun, 25 May 2025 11:38:26 -0400 Subject: [PATCH] Fix backfill execution based on ToT target comparison. --- .../scheduler/backfill_grid.dart | 36 ++---- .../scheduler/batch_backfiller.dart | 26 ++-- .../scheduler/backfill_grid_test.dart | 12 +- .../scheduler/backfill_strategy_test.dart | 10 +- .../scheduler/batch_backfiller_test.dart | 113 +++++++++++++++++- .../src/service/fake_ci_yaml_fetcher.dart | 42 +++++-- 6 files changed, 177 insertions(+), 62 deletions(-) diff --git a/app_dart/lib/src/request_handlers/scheduler/backfill_grid.dart b/app_dart/lib/src/request_handlers/scheduler/backfill_grid.dart index d4f18f1826..6576a49697 100644 --- a/app_dart/lib/src/request_handlers/scheduler/backfill_grid.dart +++ b/app_dart/lib/src/request_handlers/scheduler/backfill_grid.dart @@ -30,19 +30,20 @@ final class BackfillGrid { /// Creates an indexed data structure from the provided `(commit, [tasks])`. /// /// Some automatic filtering is applied, removing targets/tasks where: - /// - [tipOfTreeTargets] that does not use [BatchPolicy]; - /// - [tipOfTreeTargets] without a task; - /// - task that does have a matching [tipOfTreeTargets]. + /// - [postsubmitTargets] that does not use [BatchPolicy]; + /// - [postsubmitTargets] without a task; + /// - task that does have a matching [postsubmitTargets]. factory BackfillGrid.from( Iterable<(CommitRef, List)> grid, { - required Iterable tipOfTreeTargets, + required Iterable postsubmitTargets, }) { - final totTargetsByName = {for (final t in tipOfTreeTargets) t.name: t}; + final totTargetsByName = {for (final t in postsubmitTargets) t.name: t}; final commitsByName = {}; final tasksByName = >{}; for (final (commit, tasks) in grid) { commitsByName[commit.sha] = commit; for (final task in tasks) { + (tasksByName[task.name] ??= []).add(task); // Must exist at ToT (in this Map) and must be BatchPolicy. if (totTargetsByName[task.name]?.schedulerPolicy is! BatchPolicy) { // Even if it existed, let's remove it at this point because it is no @@ -51,7 +52,6 @@ final class BackfillGrid { totTargetsByName.remove(task.name); continue; } - (tasksByName[task.name] ??= []).add(task); } } @@ -122,15 +122,11 @@ final class BackfillGrid { } @useResult - Target _validateColumnAndTarget(String name, List column) { + Target? _validateColumnAndTarget(String name, List column) { if (column.isEmpty) { throw StateError('A target ("$name") should never have 0 tasks'); } - final target = _targetsByName[name]; - if (target == null) { - throw StateError('A target ("$name") should have existed in the grid'); - } - return target; + return _targetsByName[name]; } /// Each task, ordered by column (task by task). @@ -139,7 +135,7 @@ final class BackfillGrid { Iterable<(Target, List)> get eligibleTasks sync* { for (final MapEntry(key: name, value: column) in _tasksByName.entries) { final target = _validateColumnAndTarget(name, column); - if (target.backfill) { + if (target != null && target.backfill) { yield (target, column); } } @@ -151,7 +147,7 @@ final class BackfillGrid { Iterable get skippableTasks sync* { for (final MapEntry(key: name, value: column) in _tasksByName.entries) { final target = _validateColumnAndTarget(name, column); - if (target.backfill) { + if (target?.backfill == true) { continue; } for (final task in column) { @@ -164,7 +160,7 @@ final class BackfillGrid { 'A commit ("${task.commitSha}") should have existed in the grid', ); } - yield SkippableTask._from(task, target: target, commit: commit); + yield SkippableTask._from(task, commit: commit); } } } @@ -235,18 +231,11 @@ final class BackfillTask { /// A proposed task to be skipped as part of the backfill process. @immutable final class SkippableTask { - const SkippableTask._from( - this.task, { - required this.target, - required this.commit, - }); + const SkippableTask._from(this.task, {required this.commit}); /// The task itself. final TaskRef task; - /// Which [Target] (originating from `.ci.yaml`) defined this task. - final Target target; - /// The commit this task is associated with. final CommitRef commit; @@ -254,7 +243,6 @@ final class SkippableTask { String toString() { return 'SkippableTask ${const JsonEncoder.withIndent(' ').convert({ 'task': '$task', // - 'target': '$target', 'commit': '$commit', })}'; } diff --git a/app_dart/lib/src/request_handlers/scheduler/batch_backfiller.dart b/app_dart/lib/src/request_handlers/scheduler/batch_backfiller.dart index 65dbcceefc..d4e3dd04da 100644 --- a/app_dart/lib/src/request_handlers/scheduler/batch_backfiller.dart +++ b/app_dart/lib/src/request_handlers/scheduler/batch_backfiller.dart @@ -106,26 +106,26 @@ final class BatchBackfiller extends RequestHandler { '${fsGrid.map((i) => i.tasks).expand((i) => i).length} tasks', ); - // Download the ToT .ci.yaml targets. - final totCiYaml = await _ciYamlFetcher.getTipOfTreeCiYaml(slug: slug); - - final totTargets = [ - ...totCiYaml.postsubmitTargets(), - if (totCiYaml.isFusion) - ...totCiYaml.postsubmitTargets(type: CiType.fusionEngine), + // Download the current commits targets. + final currentCiYaml = await _ciYamlFetcher.getCiYamlByCommit( + fsGrid.first.commit, + postsubmit: true, + ); + final currentTargets = [ + ...currentCiYaml.postsubmitTargets(), + if (currentCiYaml.isFusion) + ...currentCiYaml.postsubmitTargets(type: CiType.fusionEngine), ]; - if (totTargets.isEmpty) { - log.warn( - 'Did not fetch any tip-of-tree targets. Backfill will do nothing!', - ); + if (currentTargets.isEmpty) { + log.warn('Did not fetch any targets. Backfill will do nothing!'); } else { - log.debug('Fetched ${totTargets.length} tip-of-tree targets'); + log.debug('Fetched ${currentTargets.length} targets'); } grid = BackfillGrid.from([ for (final CommitAndTasks(:commit, :tasks) in fsGrid) (commit, [...tasks.map((t) => t.toRef())]), - ], tipOfTreeTargets: totTargets); + ], postsubmitTargets: currentTargets); } log.debug('Built a grid of ${grid.eligibleTasks.length} target columns'); diff --git a/app_dart/test/request_handlers/scheduler/backfill_grid_test.dart b/app_dart/test/request_handlers/scheduler/backfill_grid_test.dart index b76e793618..b06be452fa 100644 --- a/app_dart/test/request_handlers/scheduler/backfill_grid_test.dart +++ b/app_dart/test/request_handlers/scheduler/backfill_grid_test.dart @@ -24,7 +24,7 @@ void main() { [ (commit, [task]), ], - tipOfTreeTargets: [target], + postsubmitTargets: [target], ); expect( @@ -51,7 +51,7 @@ void main() { (c1, [t1c1, t2c1]), (c2, [t1c2, t2c2]), ], - tipOfTreeTargets: [tg1, tg2], + postsubmitTargets: [tg1, tg2], ); expect( @@ -88,7 +88,7 @@ void main() { (c1, [t1c1, t2c1]), (c2, [t1c2, t2c2]), ], - tipOfTreeTargets: [tg1, tg2], + postsubmitTargets: [tg1, tg2], ); expect( @@ -119,7 +119,7 @@ void main() { [ (commit, [taskExists, taskMissing]), ], - tipOfTreeTargets: [targetExists], + postsubmitTargets: [targetExists], ); expect( @@ -149,7 +149,7 @@ void main() { [ (commit, [taskBatch, taskNonBatch]), ], - tipOfTreeTargets: [targetBatch, targetNonBatch], + postsubmitTargets: [targetBatch, targetNonBatch], ); expect( @@ -172,7 +172,7 @@ void main() { [ (commit, [task]), ], - tipOfTreeTargets: [target], + postsubmitTargets: [target], ); expect( diff --git a/app_dart/test/request_handlers/scheduler/backfill_strategy_test.dart b/app_dart/test/request_handlers/scheduler/backfill_strategy_test.dart index 142fbe3d99..564ef9ff49 100644 --- a/app_dart/test/request_handlers/scheduler/backfill_strategy_test.dart +++ b/app_dart/test/request_handlers/scheduler/backfill_strategy_test.dart @@ -88,7 +88,7 @@ void main() { // Linux TASK_0 Linux TASK_1 (commits[0], [taskNew (0, 0), taskNew (0, 1)]), (commits[1], [taskSucceeded (1, 0), taskInProgress (1, 1)]), - ], tipOfTreeTargets: [ + ], postsubmitTargets: [ targets[0], targets[1], ]); @@ -112,7 +112,7 @@ void main() { // Linux TASK_0 Linux TASK_1 (commits[0], [taskNew (0, 0), taskNew (0, 1)]), (commits[1], [taskNew (1, 0), taskNew (1, 1)]), - ], tipOfTreeTargets: [ + ], postsubmitTargets: [ targets[0], targets[1], ]); @@ -146,7 +146,7 @@ void main() { // Linux TASK_0 Linux TASK_1 (commits[0], [taskNew (0, 0), taskSucceeded (0, 1)]), (commits[1], [taskNew (1, 0), taskNew (1, 1)]), - ], tipOfTreeTargets: [ + ], postsubmitTargets: [ targets[0], targets[1], ]); @@ -195,7 +195,7 @@ void main() { (commits[1], [taskNew (5, 0), taskNew (5, 1), taskFailed (5, 2)]), // < 5 (commits[1], [taskNew (6, 0), taskFailed (6, 1), taskNew (6, 2)]), // < 6 (commits[1], [taskFailed (7, 0), taskNew (7, 1), taskNew (7, 2)]), // < 7 - ], tipOfTreeTargets: [ + ], postsubmitTargets: [ targets[0], targets[1], targets[2], @@ -228,7 +228,7 @@ void main() { (commit, [ generateFirestoreTask(1, commitSha: '123', name: targets[0].name).toRef() ]) - ], tipOfTreeTargets: [ + ], postsubmitTargets: [ targets[0], ]); // dart format on diff --git a/app_dart/test/request_handlers/scheduler/batch_backfiller_test.dart b/app_dart/test/request_handlers/scheduler/batch_backfiller_test.dart index 2db02c1f0c..59c0e30daf 100644 --- a/app_dart/test/request_handlers/scheduler/batch_backfiller_test.dart +++ b/app_dart/test/request_handlers/scheduler/batch_backfiller_test.dart @@ -212,8 +212,8 @@ void main() { // AFTER: expect(await visualizeFirestoreGrid(), [ - '🧑‍💼 ⬜ 🟨 🟥 🟩', - '🧑‍💼 ⬜ 🟨 🟨 🟨', + '🧑‍💼 ⬛️ 🟨 🟥 🟩', + '🧑‍💼 ⬛️ 🟨 🟨 🟨', ]); }); @@ -379,6 +379,115 @@ void main() { reason: 'Should use a low priority for all executions', ); }); + + test('skips targets that do not exist in ToT', () async { + ciYamlFetcher.setTotCiYamlFrom(''' + enabled_branches: + - master + + targets: + - name: Linux Will_Run + ''', engine: ''); + ciYamlFetcher.setCiYamlFrom(''' + enabled_branches: + - master + + targets: + - name: Linux Will_Run + - name: Linux Will_Not_Run + ''', engine: ''); + // Add a commit and two tasks. + // The second task will be skipped because it doesn't exist in ToT. + firestore.putDocument( + generateFirestoreCommit( + 0, + branch: 'master', + createTimestamp: fakeNow.millisecondsSinceEpoch, + ), + ); + firestore.putDocument( + generateFirestoreTask(0, name: 'Linux Will_Run', commitSha: '0'), + ); + firestore.putDocument( + generateFirestoreTask(1, name: 'Linux Will_Not_Run', commitSha: '0'), + ); + + // BEFORE: + expect( + await visualizeFirestoreGrid(), + // dart format off + [ + '🧑‍💼 ⬜ ⬜', + ], + // dart format on + ); + + await tester.get(handler); + + // AFTER: + expect( + await visualizeFirestoreGrid(), + // dart format off + [ + '🧑‍💼 🟨 ⬛️', + ], + // dart format on + ); + }); + + test('uses target definition from the current commit', () async { + ciYamlFetcher.setTotCiYamlFrom(''' + enabled_branches: + - master + + targets: + - name: Linux Will_Run + properties: + is-tot: "true" + ''', engine: ''); + ciYamlFetcher.setCiYamlFrom(''' + enabled_branches: + - master + + targets: + - name: Linux Will_Run + properties: + is-tot: "false" + - name: Linux Will_Not_Run + ''', engine: ''); + + // Add a commit and two tasks. + // The first task will have updated properties. + // The second task will be skipped because it doesn't exist in ToT. + firestore.putDocument( + generateFirestoreCommit( + 0, + branch: 'master', + createTimestamp: fakeNow.millisecondsSinceEpoch, + ), + ); + firestore.putDocument( + generateFirestoreTask(0, name: 'Linux Will_Run', commitSha: '0'), + ); + firestore.putDocument( + generateFirestoreTask(1, name: 'Linux Will_Not_Run', commitSha: '0'), + ); + + // Run the backfiller. + await tester.get(handler); + + // Lookup the scheduled tasks. + final tasks = fakeLuciBuildService.scheduledPostsubmitBuilds; + expect(tasks, [ + isA() + .having((t) => t.taskName, 'name', 'Linux Will_Run') + .having( + (t) => t.target.getProperties(), + 'target.getProperties()', + containsPair('is-tot', isFalse), + ), + ]); + }); } /// A very hermetic but dumb backfilling algorithm. diff --git a/app_dart/test/src/service/fake_ci_yaml_fetcher.dart b/app_dart/test/src/service/fake_ci_yaml_fetcher.dart index 8f1c69657c..2fa52583fb 100644 --- a/app_dart/test/src/service/fake_ci_yaml_fetcher.dart +++ b/app_dart/test/src/service/fake_ci_yaml_fetcher.dart @@ -17,30 +17,47 @@ final class FakeCiYamlFetcher implements CiYamlFetcher { this.failCiYamlValidation = false, }); + static CiYamlSet _from( + String root, { + String? branch, + String? engine, + CiYamlSet? totCiYaml, + }) { + return CiYamlSet( + slug: Config.flutterSlug, + branch: branch ?? Config.defaultBranch(Config.flutterSlug), + yamls: { + CiType.any: pb.SchedulerConfig()..mergeFromProto3Json(loadYaml(root)), + if (engine != null) + CiType.fusionEngine: + pb.SchedulerConfig()..mergeFromProto3Json(loadYaml(engine)), + }, + totConfig: totCiYaml, + ); + } + /// The value that should be returned as a canned response for [getCiYamlByCommit]. /// /// If omitted (`null`) defaults to a configuration with a single target. CiYamlSet? ciYaml; + /// Sets [ciYaml] by loading a YAML document. + /// + /// Optionally may also specify [engine] for a fusion [CiYamlSet]. + void setCiYamlFrom(String root, {String? engine}) { + ciYaml = _from(root, engine: engine, totCiYaml: totCiYaml); + } + /// The value that should be returned as a canned response for [getTipOfTreeCiYaml]. /// /// If omitted (`null`), defaults to the same response as [ciYaml]. CiYamlSet? totCiYaml; - /// Sets [ciYaml] by loading a YAML document. + /// Sets [totCiYaml] by loading a YAML document. /// /// Optionally may also specify [engine] for a fusion [CiYamlSet]. - void setCiYamlFrom(String root, {String? engine}) { - ciYaml = CiYamlSet( - slug: Config.flutterSlug, - branch: 'will-be-replaced', - yamls: { - CiType.any: pb.SchedulerConfig()..mergeFromProto3Json(loadYaml(root)), - if (engine != null) - CiType.fusionEngine: - pb.SchedulerConfig()..mergeFromProto3Json(loadYaml(engine)), - }, - ); + void setTotCiYamlFrom(String root, {String? engine}) { + totCiYaml = _from(root, engine: engine); } /// If `true`, [getCiYamlByCommit] will throw a [FormatException]. @@ -65,6 +82,7 @@ final class FakeCiYamlFetcher implements CiYamlFetcher { slug: commit.slug, branch: commit.branch, yamls: ci.configs.map((k, v) => MapEntry(k, v.config)), + totConfig: totCiYaml, ); }