From e85150d89fbf2f89076dc67f10a058ad4ad51601 Mon Sep 17 00:00:00 2001 From: keyonghan <54558023+keyonghan@users.noreply.github.com> Date: Wed, 21 Feb 2024 15:52:35 -0800 Subject: [PATCH] Update Firestore when reseting a prod task (#3505) Part of https://github.com/flutter/flutter/issues/142951 This PR: 1) inserts new task document to Firestore when retried manually 2) fixes `trigger_type` tag: instead of overriding manual case as `trigger_type: auto`, we now distinguish them with 2.1) `trigger_type: manual_retry` for manual retry 2.2) `trigger_type: auto_retry` for auto retry --- .../src/request_handlers/reset_prod_task.dart | 19 ++++++++++++++- .../lib/src/service/luci_build_service.dart | 2 +- .../reset_prod_task_test.dart | 24 +++++++++++++++++++ .../test/service/luci_build_service_test.dart | 1 + 4 files changed, 44 insertions(+), 2 deletions(-) diff --git a/app_dart/lib/src/request_handlers/reset_prod_task.dart b/app_dart/lib/src/request_handlers/reset_prod_task.dart index 19e3162bd..a708c70cb 100644 --- a/app_dart/lib/src/request_handlers/reset_prod_task.dart +++ b/app_dart/lib/src/request_handlers/reset_prod_task.dart @@ -11,6 +11,7 @@ import 'package:meta/meta.dart'; import '../model/appengine/commit.dart'; import '../model/appengine/key_helper.dart'; import '../model/appengine/task.dart'; +import '../model/firestore/task.dart' as firestore; import '../model/ci_yaml/ci_yaml.dart'; import '../model/ci_yaml/target.dart'; import '../model/google/token_info.dart'; @@ -19,6 +20,8 @@ import '../request_handling/body.dart'; import '../request_handling/exceptions.dart'; import '../service/config.dart'; import '../service/datastore.dart'; +import '../service/firestore.dart'; +import '../service/logging.dart'; import '../service/luci_build_service.dart'; import '../service/scheduler.dart'; @@ -54,6 +57,7 @@ class ResetProdTask extends ApiRequestHandler { @override Future post() async { final DatastoreService datastore = datastoreProvider(config.db); + final FirestoreService firestoreService = await config.createFirestoreService(); final String? encodedKey = requestData![taskKeyParam] as String?; String? branch = requestData![branchParam] as String?; final String owner = requestData![ownerParam] as String? ?? 'flutter'; @@ -87,6 +91,7 @@ class ResetProdTask extends ApiRequestHandler { futures.add( rerun( datastore: datastore, + firestoreService: firestoreService, branch: branch, sha: sha, taskName: task.name, @@ -99,6 +104,7 @@ class ResetProdTask extends ApiRequestHandler { } else { await rerun( datastore: datastore, + firestoreService: firestoreService, encodedKey: encodedKey, branch: branch, sha: sha, @@ -114,6 +120,7 @@ class ResetProdTask extends ApiRequestHandler { Future rerun({ required DatastoreService datastore, + required FirestoreService firestoreService, String? encodedKey, String? branch, String? sha, @@ -135,9 +142,17 @@ class ResetProdTask extends ApiRequestHandler { final CiYaml ciYaml = await scheduler.getCiYaml(commit); final Target target = ciYaml.postsubmitTargets.singleWhere((Target target) => target.value.name == task.name); + firestore.Task? taskDocument; + final int currentAttempt = task.attempts!; + final String documentName = '$kDatabase/documents/$kTaskCollectionId/${sha}_${taskName}_$currentAttempt'; + try { + taskDocument = await firestore.Task.fromFirestore(firestoreService: firestoreService, documentName: documentName); + } catch (error) { + log.warning('Failed to read task $documentName from Firestore: $error'); + } final Map> tags = >{ 'triggered_by': [email], - 'trigger_type': ['manual'], + 'trigger_type': ['manual_retry'], }; final bool isRerunning = await luciBuildService.checkRerunBuilder( commit: commit, @@ -146,6 +161,8 @@ class ResetProdTask extends ApiRequestHandler { datastore: datastore, tags: tags, ignoreChecks: ignoreChecks, + firestoreService: firestoreService, + taskDocument: taskDocument, ); // For human retries from the dashboard, notify if a task failed to rerun. diff --git a/app_dart/lib/src/service/luci_build_service.dart b/app_dart/lib/src/service/luci_build_service.dart index 4327ac519..fa0069b2d 100644 --- a/app_dart/lib/src/service/luci_build_service.dart +++ b/app_dart/lib/src/service/luci_build_service.dart @@ -691,7 +691,7 @@ class LuciBuildService { } log.info('Rerun builder: ${target.value.name} for commit ${commit.sha}'); tags ??= >{}; - tags['trigger_type'] = ['retry']; + tags['trigger_type'] ??= ['auto_retry']; // TODO(keyonghan): remove check when [ResetProdTask] supports firestore update. if (taskDocument != null) { diff --git a/app_dart/test/request_handlers/reset_prod_task_test.dart b/app_dart/test/request_handlers/reset_prod_task_test.dart index 6be487aa7..4d92203bb 100644 --- a/app_dart/test/request_handlers/reset_prod_task_test.dart +++ b/app_dart/test/request_handlers/reset_prod_task_test.dart @@ -5,7 +5,9 @@ import 'package:cocoon_service/cocoon_service.dart'; import 'package:cocoon_service/src/model/appengine/commit.dart'; import 'package:cocoon_service/src/model/appengine/task.dart'; +import 'package:cocoon_service/src/model/firestore/task.dart' as firestore; import 'package:cocoon_service/src/request_handling/exceptions.dart'; +import 'package:googleapis/firestore/v1.dart'; import 'package:mockito/mockito.dart'; import 'package:test/test.dart'; @@ -24,13 +26,16 @@ void main() { late FakeConfig config; FakeKeyHelper keyHelper; late MockLuciBuildService mockLuciBuildService; + late MockFirestoreService mockFirestoreService; late ApiRequestHandlerTester tester; late Commit commit; late Task task; + final firestore.Task firestoreTask = generateFirestoreTask(1, attempts: 1); setUp(() { final FakeDatastoreDB datastoreDB = FakeDatastoreDB(); clientContext = FakeClientContext(); + mockFirestoreService = MockFirestoreService(); clientContext.isDevelopmentEnvironment = false; keyHelper = FakeKeyHelper(applicationContext: clientContext.applicationContext); config = FakeConfig( @@ -39,6 +44,7 @@ void main() { supportedBranchesValue: [ Config.defaultBranch(Config.flutterSlug), ], + firestoreService: mockFirestoreService, ); final FakeAuthenticatedContext authContext = FakeAuthenticatedContext(clientContext: clientContext); tester = ApiRequestHandlerTester(context: authContext); @@ -63,6 +69,16 @@ void main() { 'Key': config.keyHelper.encode(task.key), }; + when( + mockFirestoreService.getDocument( + captureAny, + ), + ).thenAnswer((Invocation invocation) { + return Future.value( + firestoreTask, + ); + }); + when( mockLuciBuildService.checkRerunBuilder( commit: anyNamed('commit'), @@ -71,6 +87,8 @@ void main() { target: anyNamed('target'), tags: anyNamed('tags'), ignoreChecks: anyNamed('ignoreChecks'), + firestoreService: mockFirestoreService, + taskDocument: anyNamed('taskDocument'), ), ).thenAnswer((_) async => true); }); @@ -103,6 +121,8 @@ void main() { target: anyNamed('target'), tags: anyNamed('tags'), ignoreChecks: true, + firestoreService: mockFirestoreService, + taskDocument: anyNamed('taskDocument'), ), ).called(1); }); @@ -127,6 +147,8 @@ void main() { target: anyNamed('target'), tags: anyNamed('tags'), ignoreChecks: false, + firestoreService: mockFirestoreService, + taskDocument: anyNamed('taskDocument'), ), ).called(2); }); @@ -173,6 +195,8 @@ void main() { target: anyNamed('target'), tags: anyNamed('tags'), ignoreChecks: true, + firestoreService: mockFirestoreService, + taskDocument: anyNamed('taskDocument'), ), ).thenAnswer((_) async => false); config.db.values[task.key] = task; diff --git a/app_dart/test/service/luci_build_service_test.dart b/app_dart/test/service/luci_build_service_test.dart index 283ef94ee..6bad124f6 100644 --- a/app_dart/test/service/luci_build_service_test.dart +++ b/app_dart/test/service/luci_build_service_test.dart @@ -961,6 +961,7 @@ void main() { } expect(scheduleBuildRequest.priority, LuciBuildService.kRerunPriority); expect(scheduleBuildRequest.gitilesCommit?.project, 'mirrors/engine'); + expect(scheduleBuildRequest.tags?['trigger_type'], ['auto_retry']); expect(rerunFlag, isTrue); expect(task.attempts, 2); expect(task.status, Task.statusInProgress);