Skip to content

Commit

Permalink
Update dart-internal-build-results-sub to different pubsub message fo…
Browse files Browse the repository at this point in the history 
…rmat (#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
  • Loading branch information
@drewroengoogle
drewroengoogle authored Jul 18, 2023
1 parent cef706a commit 771ceba
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 162 deletions.
73 changes: 34 additions & 39 deletions app_dart/lib/src/request_handlers/dart_internal_subscription.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<Body> 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);
Expand All @@ -82,28 +101,4 @@ class DartInternalSubscription extends SubscriptionHandler {

return Body.forJson(taskToInsert.toString());
}

Future<Build> _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,
);
}
}
3 changes: 0 additions & 3 deletions app_dart/lib/src/service/config.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<List<String>> get releaseAccounts => _getReleaseAccounts();

Expand Down
188 changes: 68 additions & 120 deletions app_dart/test/request_handlers/dart_internal_subscription_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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();
Expand All @@ -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();

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -173,7 +182,7 @@ void main() {
final List<Task> datastoreCommit = <Task>[fakeTask];
await config.db.commit(inserts: datastoreCommit);

tester.message = push.PushMessage(data: fakePubsubMessage);
tester.message = const push.PushMessage(data: fakePubsubMessage);

await tester.post(handler);

Expand Down Expand Up @@ -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<Build>.value(fakeInProgressBuild), Future<Build>.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<Exception>()),
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));
});
}

0 comments on commit 771ceba

Please sign in to comment.