From 771cebabce9b9aab33527b35cc65d43ce811e53d Mon Sep 17 00:00:00 2001 From: Drew Roen <102626803+drewroengoogle@users.noreply.github.com> Date: Tue, 18 Jul 2023 09:36:17 -0500 Subject: [PATCH] Update dart-internal-build-results-sub to different pubsub message format (#2936) * Now that buildbucket is publishing the results instead of within the recipes, this PR updates how we handle buildbucket results based on the updated publishing format * Remove the retry on the buildbucket client now that we can correctly handle builds that are in progress * Add new tests --- .../dart_internal_subscription.dart | 73 ++++--- app_dart/lib/src/service/config.dart | 3 - .../dart_internal_subscription_test.dart | 188 +++++++----------- 3 files changed, 102 insertions(+), 162 deletions(-) diff --git a/app_dart/lib/src/request_handlers/dart_internal_subscription.dart b/app_dart/lib/src/request_handlers/dart_internal_subscription.dart index 682b9914f..7c19977d4 100644 --- a/app_dart/lib/src/request_handlers/dart_internal_subscription.dart +++ b/app_dart/lib/src/request_handlers/dart_internal_subscription.dart @@ -6,13 +6,11 @@ import 'dart:convert'; import 'package:cocoon_service/src/model/luci/buildbucket.dart'; import 'package:meta/meta.dart'; -import 'package:retry/retry.dart'; import '../../cocoon_service.dart'; import '../model/appengine/task.dart'; import '../request_handling/subscription_handler.dart'; import '../service/datastore.dart'; -import '../service/exceptions.dart'; import '../service/logging.dart'; /// TODO(drewroengoogle): Make this subscription generic so we can accept more @@ -33,36 +31,57 @@ class DartInternalSubscription extends SubscriptionHandler { super.authProvider, required this.buildBucketClient, @visibleForTesting this.datastoreProvider = DatastoreService.defaultProvider, - this.retryOptions = Config.buildbucketRetry, }) : super(subscriptionName: 'dart-internal-build-results-sub'); final BuildBucketClient buildBucketClient; final DatastoreServiceProvider datastoreProvider; - final RetryOptions retryOptions; @override Future post() async { final DatastoreService datastore = datastoreProvider(config.db); - // This message comes from the engine_v2 recipes once a build run on - // dart-internal has completed. - // - // Example: https://flutter.googlesource.com/recipes/+/c6af020f0f22e392e30b769a5ed97fadace308fa/recipes/engine_v2/engine_v2.py#185 - log.info("Getting buildbucket id from pubsub message"); - final dynamic messageJson = json.decode(message.data.toString()); + if (message.data == null) { + log.info('no data in message'); + return Body.empty; + } + + final dynamic buildData = json.decode(message.data!); + log.info("Build data json: $buildData"); + + if (buildData["build"] == null) { + log.info('no build information in message'); + return Body.empty; + } + + final String project = buildData['build']['builder']['project']; + final String bucket = buildData['build']['builder']['bucket']; + final String builder = buildData['build']['builder']['builder']; - final int buildbucketId = messageJson['buildbucket_id']; - log.info("Buildbucket id: $buildbucketId"); + // This should already be covered by the pubsub filter, but adding an additional check + // to ensure we don't process builds that aren't from dart-internal/flutter. + if (project != 'dart-internal' || bucket != 'flutter') { + log.info("Ignoring build not from dart-internal/flutter bucket"); + return Body.empty; + } - log.info("Creating build request object"); + // Only publish the parent release_builder builds to the datastore. + // TODO(drewroengoogle): Determine which builds we want to save to the datastore and check for them. + final regex = RegExp(r'(Linux|Mac|Windows)\s+(engine_release_builder|packaging_release_builder)'); + if (!regex.hasMatch(builder)) { + log.info("Ignoring builder that is not a release builder"); + return Body.empty; + } + + final String buildbucketId = buildData['build']['id']; + log.info("Creating build request object with build id $buildbucketId"); final GetBuildRequest request = GetBuildRequest( - id: buildbucketId.toString(), + id: buildbucketId, ); log.info( "Calling buildbucket api to get build data for build $buildbucketId", ); - final Build build = await _getBuildFromBuildbucket(request); + final Build build = await buildBucketClient.getBuild(request); log.info("Checking for existing task in datastore"); final Task? existingTask = await datastore.getTaskFromBuildbucketBuild(build); @@ -82,28 +101,4 @@ class DartInternalSubscription extends SubscriptionHandler { return Body.forJson(taskToInsert.toString()); } - - Future _getBuildFromBuildbucket( - GetBuildRequest request, - ) async { - return retryOptions.retry( - () async { - final Build build = await buildBucketClient.getBuild(request); - - if (build.status != Status.success && - build.status != Status.failure && - build.status != Status.infraFailure && - build.status != Status.canceled) { - log.info( - "Build is not finished", - ); - throw UnfinishedBuildException( - "Build is not finished", - ); - } - return build; - }, - retryIf: (Exception e) => e is UnfinishedBuildException, - ); - } } diff --git a/app_dart/lib/src/service/config.dart b/app_dart/lib/src/service/config.dart index f1116ecc8..8bf134d5f 100644 --- a/app_dart/lib/src/service/config.dart +++ b/app_dart/lib/src/service/config.dart @@ -170,9 +170,6 @@ class Config { /// Max retries when scheduling builds. static const RetryOptions schedulerRetry = RetryOptions(maxAttempts: 3); - /// Max retries when getting builds from buildbucket. - static const RetryOptions buildbucketRetry = RetryOptions(maxAttempts: 3, delayFactor: Duration(seconds: 2)); - /// List of GitHub accounts related to releases. Future> get releaseAccounts => _getReleaseAccounts(); diff --git a/app_dart/test/request_handlers/dart_internal_subscription_test.dart b/app_dart/test/request_handlers/dart_internal_subscription_test.dart index b3a606956..519d58819 100644 --- a/app_dart/test/request_handlers/dart_internal_subscription_test.dart +++ b/app_dart/test/request_handlers/dart_internal_subscription_test.dart @@ -10,7 +10,6 @@ import 'package:cocoon_service/src/model/luci/push_message.dart' as push; import 'package:cocoon_service/src/service/datastore.dart'; import 'package:gcloud/db.dart'; import 'package:mockito/mockito.dart'; -import 'package:retry/retry.dart'; import 'package:test/test.dart'; import '../src/datastore/fake_config.dart'; @@ -31,11 +30,22 @@ void main() { final DateTime endTime = DateTime(2023, 1, 1, 0, 14, 23); const String project = "dart-internal"; const String bucket = "flutter"; - const String builder = "Mac amazing_builder_tests"; + const String builder = "Linux packaging_release_builder"; const int buildId = 123456; const String fakeHash = "HASH12345"; const String fakeBranch = "test-branch"; - final String fakePubsubMessage = "{ \"buildbucket_id\": ${buildId.toString()} }"; + const String fakePubsubMessage = """ + { + "build": { + "id": "$buildId", + "builder": { + "project": "$project", + "bucket": "$bucket", + "builder": "$builder" + } + } + } + """; setUp(() async { config = FakeConfig(); @@ -46,7 +56,6 @@ void main() { authProvider: FakeAuthenticationProvider(), buildBucketClient: buildBucketClient, datastoreProvider: (DatastoreDB db) => DatastoreService(config.db, 5), - retryOptions: const RetryOptions(maxAttempts: 3, delayFactor: Duration.zero), ); request = FakeHttpRequest(); @@ -90,7 +99,7 @@ void main() { }); test('creates a new task successfully', () async { - tester.message = push.PushMessage(data: fakePubsubMessage); + tester.message = const push.PushMessage(data: fakePubsubMessage); await tester.post(handler); @@ -173,7 +182,7 @@ void main() { final List datastoreCommit = [fakeTask]; await config.db.commit(inserts: datastoreCommit); - tester.message = push.PushMessage(data: fakePubsubMessage); + tester.message = const push.PushMessage(data: fakePubsubMessage); await tester.post(handler); @@ -233,128 +242,67 @@ void main() { ); }); - test('retries buildbucket when build is still in progress', () async { - final Build fakeCompletedBuild = Build( - builderId: const BuilderId(project: project, bucket: bucket, builder: builder), - number: buildId, - id: 'fake-build-id', - status: Status.success, - startTime: startTime, - endTime: endTime, - input: const Input( - gitilesCommit: GitilesCommit( - project: "flutter/flutter", - hash: fakeHash, - ref: "refs/heads/$fakeBranch", - ), - ), - ); - - final Build fakeInProgressBuild = Build( - builderId: const BuilderId(project: project, bucket: bucket, builder: builder), - number: buildId, - id: 'fake-build-id', - status: Status.started, - startTime: startTime, - endTime: null, - input: const Input( - gitilesCommit: GitilesCommit( - project: "flutter/flutter", - hash: fakeHash, - ref: "refs/heads/$fakeBranch", - ), - ), - ); - - // The first time the mocked getBuild is called, return fakeInProgressBuild, - // then return fakeCompletedBuild the second time. - int count = 0; - when( - buildBucketClient.getBuild( - any, - buildBucketUri: "https://cr-buildbucket.appspot.com/prpc/buildbucket.v2.Builds", - ), - ).thenAnswer( - (_) => [Future.value(fakeInProgressBuild), Future.value(fakeCompletedBuild)][count++], - ); - - tester.message = push.PushMessage(data: fakePubsubMessage); - - await tester.post(handler); + test('ignores null message', () async { + tester.message = const push.PushMessage(data: null); + expect(await tester.post(handler), equals(Body.empty)); + }); - verify( - buildBucketClient.getBuild(any), - ).called(2); + test('ignores message with empty build data', () async { + tester.message = const push.PushMessage(data: "{}"); + expect(await tester.post(handler), equals(Body.empty)); + }); - // This is used for testing to pull the data out of the "datastore" so that - // we can verify what was saved. - late Task taskInDb; - late Commit commitInDb; - config.db.values.forEach((k, v) { - if (v is Task && v.buildNumberList == buildId.toString()) { - taskInDb = v; - } - if (v is Commit) { - commitInDb = v; + test('ignores message not from flutter bucket', () async { + tester.message = const push.PushMessage( + data: """ + { + "build": { + "id": "$buildId", + "builder": { + "project": "$project", + "bucket": "dart", + "builder": "$builder" + } } - }); - - // Ensure the task has the correct parent and commit key - expect( - commitInDb.id, - equals(taskInDb.commitKey?.id), - ); - - expect( - commitInDb.id, - equals(taskInDb.parentKey?.id), - ); - - // Ensure the task in the db is exactly what we expect - final Task expectedTask = Task( - attempts: 1, - buildNumber: buildId, - buildNumberList: buildId.toString(), - builderName: builder, - commitKey: commitInDb.key, - createTimestamp: startTime.millisecondsSinceEpoch, - endTimestamp: endTime.millisecondsSinceEpoch, - luciBucket: bucket, - name: builder, - stageName: "dart-internal", - startTimestamp: startTime.millisecondsSinceEpoch, - status: "Succeeded", - key: commit.key.append(Task), - timeoutInMinutes: 0, - reason: '', - requiredCapabilities: [], - reservedForAgentId: '', - ); - - expect( - taskInDb.toString(), - equals(expectedTask.toString()), + } + """, ); + expect(await tester.post(handler), equals(Body.empty)); }); - test('does not retry buildbucket when buildbucket throws an exception', () async { - when( - await buildBucketClient.getBuild( - any, - buildBucketUri: "https://cr-buildbucket.appspot.com/prpc/buildbucket.v2.Builds", - ), - ).thenThrow( - Exception("Failed to get from buildbucket for some reason!"), + test('ignores message not from dart-internal project', () async { + tester.message = const push.PushMessage( + data: """ + { + "build": { + "id": "$buildId", + "builder": { + "project": "different-project", + "bucket": "$bucket", + "builder": "$builder" + } + } + } + """, ); + expect(await tester.post(handler), equals(Body.empty)); + }); - tester.message = push.PushMessage(data: fakePubsubMessage); - await expectLater( - tester.post(handler), - throwsA(isA()), + test('ignores message not from an accepted builder', () async { + tester.message = const push.PushMessage( + data: """ + { + "build": { + "id": "$buildId", + "builder": { + "project": "different-project", + "bucket": "$bucket", + "builder": "different-builder" + } + } + } + """, ); - - verify( - buildBucketClient.getBuild(any), - ).called(1); + expect(await tester.post(handler), equals(Body.empty)); }); }