From d4d91c40af6cd9e4e531938fe3e712979ea735cc Mon Sep 17 00:00:00 2001 From: Karl Klose Date: Fri, 31 Jul 2020 07:59:20 +0000 Subject: [PATCH] [infra] Add a script to narrow blamelists in the result feed This script takes a list of test results and identifies blamelists in the result feed data that include the commit of the test results and tries to narrow the blamelist, if possible. This CL also adds a small library to use the firestore REST API, which contains mostly the functionality used in the script, but should be easy enough to extend for other scripts. Change-Id: If3c8272438e2a9bbf24891d9f5b62c342ea77cc6 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/153966 Commit-Queue: Karl Klose Reviewed-by: William Hesse --- pkg/test_runner/lib/bot_results.dart | 46 ++++++ tools/bots/compare_results.dart | 23 --- tools/bots/lib/src/firestore.dart | 217 +++++++++++++++++++++++++++ tools/bots/update_blamelists.dart | 188 +++++++++++++++++++++++ 4 files changed, 451 insertions(+), 23 deletions(-) create mode 100644 tools/bots/lib/src/firestore.dart create mode 100644 tools/bots/update_blamelists.dart diff --git a/pkg/test_runner/lib/bot_results.dart b/pkg/test_runner/lib/bot_results.dart index 8ae90595911..88b2eb12b5b 100644 --- a/pkg/test_runner/lib/bot_results.dart +++ b/pkg/test_runner/lib/bot_results.dart @@ -14,6 +14,52 @@ import 'package:pool/pool.dart'; /// The path to the gsutil script. String gsutilPy; +// TODO(karlklose): Update this class with all fields that +// are used in pkg/test_runner and the tools/bots scripts and include +// validation (in particular for fields from extend_results.dart, that are +// optional but expected in some contexts and should always be all or nothing). +class Result { + final String configuration; + final String expectation; + final bool matches; + final String name; + final String outcome; + final bool changed; + final String commitHash; + // TODO(karlklose): this field is unnecessary with extended results and + // should be removed. + final bool flaked; + final bool isFlaky; + final String previousOutcome; + + Result( + this.configuration, + this.name, + this.outcome, + this.expectation, + this.matches, + this.changed, + this.commitHash, + this.isFlaky, + this.previousOutcome, + [this.flaked = false]); + + Result.fromMap(Map map, [Map flakinessData]) + : configuration = map["configuration"] as String, + name = map["name"] as String, + outcome = map["result"] as String, + expectation = map["expected"] as String, + matches = map["matches"] as bool, + changed = map["changed"] as bool, + commitHash = map["commit_hash"] as String, + isFlaky = map["flaky"] as bool, + previousOutcome = map["previous_result"] as String, + flaked = flakinessData != null && + (flakinessData["outcomes"] as List).contains(map["result"]); + + String get key => "$configuration:$name"; +} + /// Cloud storage location containing results. const testResultsStoragePath = "gs://dart-test-results/builders"; diff --git a/tools/bots/compare_results.dart b/tools/bots/compare_results.dart index 4b6405728f3..ef74e021bd1 100755 --- a/tools/bots/compare_results.dart +++ b/tools/bots/compare_results.dart @@ -13,29 +13,6 @@ import 'dart:io'; import 'package:args/args.dart'; import 'package:test_runner/bot_results.dart'; -class Result { - final String configuration; - final String name; - final String outcome; - final String expectation; - final bool matches; - final bool flaked; - - Result(this.configuration, this.name, this.outcome, this.expectation, - this.matches, this.flaked); - - Result.fromMap(Map map, Map flakinessData) - : configuration = map["configuration"], - name = map["name"], - outcome = map["result"], - expectation = map["expected"], - matches = map["matches"], - flaked = flakinessData != null && - flakinessData["outcomes"].contains(map["result"]); - - String get key => "$configuration:$name"; -} - class Event { final Result before; final Result after; diff --git a/tools/bots/lib/src/firestore.dart b/tools/bots/lib/src/firestore.dart new file mode 100644 index 00000000000..6e8cc47b162 --- /dev/null +++ b/tools/bots/lib/src/firestore.dart @@ -0,0 +1,217 @@ +// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'dart:convert' show jsonDecode, jsonEncode; +import 'dart:io' show File, HttpStatus; + +import 'package:http/http.dart' as http; + +Future readGcloudAuthToken(String path) async { + String token = await File(path).readAsString(); + return token.split("\n").first; +} + +/// Helper class to access the Firestore REST API. +/// +/// This class is not a complete implementation of the Firestore REST protocol +/// and is only meant to support the operations required by scripts in +/// tools/bots. +class FirestoreDatabase { + final http.Client _client = http.Client(); + final String _authToken; + final String _project; + + /// The current transaction ID in base64 (or `null`) + String _currentTransaction; + + /// Returns the current transaction escaped to be useable as part of a URI. + String get _escapedCurrentTransaction { + return Uri.encodeFull(_currentTransaction) + // The Firestore API does not accept '+' in URIs + .replaceAll("+", "%2B"); + } + + FirestoreDatabase(this._project, this._authToken); + + static const apiUrl = 'https://firestore.googleapis.com/v1beta1'; + + String get projectUrl => '$apiUrl/projects/$_project'; + + String get documentsUrl => '$projectUrl/databases/(default)/documents'; + + String get queryUrl => '$documentsUrl:runQuery'; + + Map get _headers { + return { + 'Authorization': 'Bearer $_authToken', + 'Accept': 'application/json', + 'Content-Type': 'application/json' + }; + } + + Future runQuery(Query query) async { + var body = jsonEncode(query.data); + var response = await _client.post(queryUrl, headers: _headers, body: body); + if (response.statusCode == HttpStatus.ok) { + return jsonDecode(response.body); + } else { + throw _error(response); + } + } + + Future getDocument(String collectionName, String documentName) async { + var url = '$documentsUrl/$collectionName/$documentName'; + if (_currentTransaction != null) { + url = '$url?transaction=${_escapedCurrentTransaction}'; + } + var response = await _client.get(url, headers: _headers); + if (response.statusCode == HttpStatus.ok) { + return jsonDecode(response.body); + } else { + throw _error(response); + } + } + + Future updateField(Map document, String field) async { + var url = '$apiUrl/${document["name"]}?updateMask.fieldPaths=$field'; + var response = + await _client.patch(url, headers: _headers, body: jsonEncode(document)); + if (response.statusCode == HttpStatus.ok) { + return jsonDecode(response.body); + } else { + throw _error(response); + } + } + + void beginTransaction() async { + if (_currentTransaction != null) { + throw Exception('Error: nested transactions'); + } + var url = '$documentsUrl:beginTransaction'; + var body = '{"options": {}}'; + var response = await _client.post(url, headers: _headers, body: body); + if (response.statusCode == HttpStatus.ok) { + var result = jsonDecode(response.body); + _currentTransaction = result['transaction'] as String; + if (_currentTransaction == null) { + throw Exception("Call returned no transaction identifier"); + } + } else { + throw _error(response, message: 'Could not start transaction:'); + } + } + + Future commit([List writes]) async { + if (_currentTransaction == null) { + throw Exception('"commit" called without transaction'); + } + var body = jsonEncode({ + "writes": writes.map((write) => write.data).toList(), + "transaction": "$_currentTransaction" + }); + var url = '$documentsUrl:commit'; + var response = await _client.post(url, headers: _headers, body: body); + _currentTransaction = null; + if (response.statusCode == HttpStatus.conflict) { + // This HTTP status code corresponds to the ABORTED error code, see + // https://cloud.google.com/datastore/docs/concepts/errors and + // https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto#L137 + return false; + } + if (response.statusCode != HttpStatus.ok) { + throw _error(response); + } + return true; + } + + Exception _error(http.Response response, {String message: 'Error'}) { + throw Exception('$message: ${response.statusCode}: ' + '${response.reasonPhrase}:\n${response.body}'); + } + + /// Closes the underlying HTTP client. + void closeClient() => _client.close(); +} + +abstract class Write { + Map get data; +} + +class Update implements Write { + final Map data; + Update(List updateMask, Map document, {String updateTime}) + : data = { + if (updateTime != null) "currentDocument": {"updateTime": updateTime}, + "updateMask": {"fieldPaths": updateMask}, + "update": document + }; +} + +class Query { + final Map data; + Query(String collection, Filter filter, {int limit}) + : data = { + 'structuredQuery': { + 'from': [ + {'collectionId': collection} + ], + if (limit != null) 'limit': limit, + 'where': filter.data, + } + }; +} + +class Filter { + final Map data; + Filter(this.data); +} + +class FieldFilter extends Filter { + FieldFilter(String field, String op, String type, Object value) + : super({ + 'fieldFilter': { + 'field': {'fieldPath': field}, + 'op': op, + 'value': {'$type': value}, + } + }); +} + +class Field { + final String name; + Field(this.name); + FieldFilter equals(Value value) { + return FieldFilter(name, 'EQUAL', value.type, value.value); + } + + FieldFilter greaterOrEqual(Value value) { + return FieldFilter(name, 'GREATER_THAN_OR_EQUAL', value.type, value.value); + } + + FieldFilter lessOrEqual(Value value) { + return FieldFilter(name, 'LESS_THAN_OR_EQUAL', value.type, value.value); + } + + FieldFilter contains(Value value) { + return FieldFilter(name, 'ARRAY_CONTAINS', value.type, value.value); + } +} + +class Value { + final String type; + final Object value; + Value.boolean(bool this.value) : type = 'booleanValue'; + Value.string(String this.value) : type = 'stringValue'; + Value.integer(int this.value) : type = 'integerValue'; +} + +class CompositeFilter extends Filter { + CompositeFilter(String op, List parts) + : super({ + 'compositeFilter': { + 'op': op, + 'filters': parts.map((part) => part.data).toList(), + } + }); +} diff --git a/tools/bots/update_blamelists.dart b/tools/bots/update_blamelists.dart new file mode 100644 index 00000000000..3fb3c9d6b5a --- /dev/null +++ b/tools/bots/update_blamelists.dart @@ -0,0 +1,188 @@ +// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// This script is used by the bisection mechanism to update the blamelists +// of active, non-approved failures which include the commit of the current +// bisection build. + +import 'dart:io'; + +import 'package:args/args.dart'; + +import 'lib/src/firestore.dart'; +import 'package:test_runner/bot_results.dart'; + +const newTest = 'new test'; +const skippedTest = 'skipped'; + +const maxAttempts = 20; + +FirestoreDatabase database; + +class ResultRecord { + final data; + + ResultRecord(this.data); + + Map field(String name) => data['fields'][name]; + + int get blamelistStartIndex { + return int.parse(field('blamelist_start_index')['integerValue']); + } + + void set blamelistStartIndex(int index) { + field('blamelist_start_index')['integerValue'] = '$index'; + } + + int get blamelistEndIndex { + return int.parse(field('blamelist_end_index')['integerValue']); + } + + String get result => field('result')['stringValue']; + + String get previousResult => field('previous_result')['stringValue']; + + String get name => field('name')['stringValue']; + + String get updateTime => data['updateTime']; +} + +Query unapprovedActiveFailuresQuery(String configuration) { + return Query( + 'results', + CompositeFilter('AND', [ + Field('approved').equals(Value.boolean(false)), + // TODO(karlklose): also search for inactive failures? + Field('active_configurations').contains(Value.string(configuration)), + // TODO(karlklose): add index to check for blamelist_start_index < ? + ])); +} + +Future getCommitIndex(String commit) async { + try { + Map document = await database.getDocument('commits', commit); + var index = document['fields']['index']; + if (index['integerValue'] == null) { + throw Exception('Expected an integer, but got "$index"'); + } + return int.parse(index['integerValue']); + } catch (exception) { + print('Could not retrieve index for commit "$commit".\n'); + rethrow; + } +} + +/// Compute if the record should be updated based on the outcomes in the +/// result record and the new test result. +bool shouldUpdateRecord(ResultRecord resultRecord, Result testResult) { + if (testResult == null || !testResult.matches) { + return false; + } + var baseline = testResult.expectation.toLowerCase(); + if (resultRecord.previousResult.toLowerCase() != baseline) { + // Currently we only support the case where a bisection run improves the + // accuracy of a "Green" -> "Red" result record. + return false; + } + if (resultRecord.result.toLowerCase() == newTest || + resultRecord.result.toLowerCase() == skippedTest) { + // Skipped tests are often configuration dependent, so it could be wrong + // to generalize their effect for the result record to different + // configurations. + return false; + } + return true; +} + +void updateBlameLists( + String configuration, String commit, Map testResults) async { + int commitIndex = await getCommitIndex(commit); + var query = unapprovedActiveFailuresQuery(configuration); + bool needsRetry; + int attempts = 0; + do { + needsRetry = false; + var documents = (await database.runQuery(query)) + .where((result) => result['document'] != null) + .map((result) => result['document']['name']); + for (var documentPath in documents) { + await database.beginTransaction(); + var documentName = documentPath.split('/').last; + var result = + ResultRecord(await database.getDocument('results', documentName)); + if (commitIndex < result.blamelistStartIndex || + commitIndex >= result.blamelistEndIndex) { + continue; + } + String name = result.name; + var testResultData = testResults['$configuration:$name']; + var testResult = + testResultData != null ? Result.fromMap(testResultData) : null; + if (!shouldUpdateRecord(result, testResult)) { + continue; + } + print('Found result record: $configuration:${result.name}: ' + '${result.previousResult} -> ${result.result} ' + 'in ${result.blamelistStartIndex}..${result.blamelistEndIndex} ' + 'to update with ${testResult.outcome} at $commitIndex.'); + // We found a result representation for this test and configuration whose + // blamelist includes this results' commit but whose outcome is different + // then the outcome in the provided test results. + // This means that this commit should not be part of the result + // representation and we can update the lower bound of the commit range + // and the previous result. + var newStartIndex = commitIndex + 1; + if (newStartIndex > result.blamelistEndIndex) { + print('internal error: inconsistent results; skipping results entry'); + continue; + } + result.blamelistStartIndex = newStartIndex; + var updateIndex = Update(['blamelist_start_index'], result.data); + if (!await database.commit([updateIndex])) { + // Commiting the change to the database had a conflict, retry. + needsRetry = true; + if (++attempts == maxAttempts) { + throw Exception('Exceeded maximum retry attempts ($maxAttempts).'); + } + print('Transaction failed, trying again!'); + } + } + } while (needsRetry); +} + +main(List arguments) async { + var parser = ArgParser() + ..addOption('auth-token', + abbr: 'a', + help: 'path to a file containing the gcloud auth token (required)') + ..addOption('results', + abbr: 'r', + help: 'path to a file containing the test results (required)') + ..addFlag('staging', + abbr: 's', + help: 'use staging database', + defaultsTo: true, + negatable: true); + var options = parser.parse(arguments); + if (options.rest.isNotEmpty || + options['results'] == null || + options['auth-token'] == null) { + print(parser.usage); + exit(1); + } + var results = await loadResultsMap(options['results']); + if (results.isEmpty) { + print("No test results provided, nothing to update."); + return; + } + // Pick an arbitrary result entry to find configuration and commit hash. + var firstResult = Result.fromMap(results.values.first); + var commit = firstResult.commitHash; + var configuration = firstResult.configuration; + var project = options['staging'] ? 'dart-ci-staging' : 'dart-ci'; + database = FirestoreDatabase( + project, await readGcloudAuthToken(options['auth-token'])); + await updateBlameLists(configuration, commit, results); + database.closeClient(); +}