Syncing script shouldn't schedule a build request when there is a build from another...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 May 2017 21:10:09 +0000 (21:10 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 May 2017 21:10:09 +0000 (21:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172577
<rdar://problem/32395049>

Reviewed by Chris Dumez.

When a buildbot master gets restarted while there is an in-progress build and a pending build, the master will
re-schedule the currently running build, and this can result in multiple build requests from different test
groups being scheduled simultaneously.

sync-buildbot.js was supposed to recover from this state by only processing build requests from one test group
at a time and eventually come back to a state where only a single test group is running per buildbot slave.

We had a test for this particular case but it wasn't testing what it claimed to test. Rewriten the test case
and fixed the bug by explicitly checking this condition and treating it as if there is a pending build already
scheduled in the builder in this case.

* public/api/test-groups.php:
(main): Fixed a regression from r217397. Return the platform ID of the first request when none of the requets
have been processed yet or all of them had failed.
* server-tests/tools-buildbot-triggerable-tests.js: Rewritten a test case intended to cover this bug.
(.assertRequestAndResolve): Added.
* tools/js/buildbot-syncer.js:
(BuildbotSyncer.prototype.scheduleRequestInGroupIfAvailable): Fixed the bug. Avoid scheduling a new request on
this syncer if there is a build in progress for a test group different from that of the new request. Reuse the
code we had to deal with a pending build for this purpose.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@217448 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/api/test-groups.php
Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js
Websites/perf.webkit.org/tools/js/buildbot-syncer.js

index 9087956..a12b5e7 100644 (file)
@@ -1,3 +1,32 @@
+2017-05-25  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Syncing script shouldn't schedule a build request when there is a build from another test group in progress
+        https://bugs.webkit.org/show_bug.cgi?id=172577
+        <rdar://problem/32395049>
+
+        Reviewed by Chris Dumez.
+
+        When a buildbot master gets restarted while there is an in-progress build and a pending build, the master will
+        re-schedule the currently running build, and this can result in multiple build requests from different test
+        groups being scheduled simultaneously.
+
+        sync-buildbot.js was supposed to recover from this state by only processing build requests from one test group
+        at a time and eventually come back to a state where only a single test group is running per buildbot slave.
+
+        We had a test for this particular case but it wasn't testing what it claimed to test. Rewriten the test case
+        and fixed the bug by explicitly checking this condition and treating it as if there is a pending build already
+        scheduled in the builder in this case.
+
+        * public/api/test-groups.php:
+        (main): Fixed a regression from r217397. Return the platform ID of the first request when none of the requets
+        have been processed yet or all of them had failed.
+        * server-tests/tools-buildbot-triggerable-tests.js: Rewritten a test case intended to cover this bug.
+        (.assertRequestAndResolve): Added.
+        * tools/js/buildbot-syncer.js:
+        (BuildbotSyncer.prototype.scheduleRequestInGroupIfAvailable): Fixed the bug. Avoid scheduling a new request on
+        this syncer if there is a build in progress for a test group different from that of the new request. Reuse the
+        code we had to deal with a pending build for this purpose.
+
 2017-05-24  Ryosuke Niwa  <rniwa@webkit.org>
 
         Opening an analysis task from the queue page is broken
index 8cdd696..9110c7e 100644 (file)
@@ -43,6 +43,11 @@ function main($path) {
             WHERE run_config = config_id AND run_build = request_build AND request_group = $1', array($group_id));
         if ($platforms)
             $group['platform'] = $platforms[0]['config_platform'];
+        else {
+            $first_request = $db->select_first_row('build_requests', 'request', array('group' => $group_id), 'order');
+            if ($first_request)
+                $group['platform'] = $first_request['request_platform'];
+        }
     }
 
     $build_requests = $build_requests_fetcher->results();
index d15b3cc..d32eec7 100644 (file)
@@ -9,6 +9,13 @@ const TestServer = require('./resources/test-server.js');
 const prepareServerTest = require('./resources/common-operations.js').prepareServerTest;
 const MockLogger = require('./resources/mock-logger.js').MockLogger;
 
+function assertRequestAndResolve(request, method, url, content)
+{
+    assert.equal(request.method, method);
+    assert.equal(request.url, url);
+    request.resolve(content);
+}
+
 describe('BuildbotTriggerable', function () {
     prepareServerTest(this);
 
@@ -683,7 +690,10 @@ describe('BuildbotTriggerable', function () {
 
         it('should recover from multiple test groups running simultenously', () => {
             const db = TestServer.database();
+            const requests = MockRemoteAPI.requests;
+
             let syncPromise;
+            let triggerable;
             return Promise.all([
                 MockData.addMockData(db, ['completed', 'pending', 'pending', 'pending']),
                 MockData.addAnotherMockTestGroup(db, ['completed', 'pending', 'pending', 'pending'])
@@ -693,70 +703,54 @@ describe('BuildbotTriggerable', function () {
                 const config = MockData.mockTestSyncConfigWithSingleBuilder();
                 const logger = new MockLogger;
                 const slaveInfo = {name: 'sync-slave', password: 'password'};
-                const triggerable = new BuildbotTriggerable(config, TestServer.remoteAPI(), MockRemoteAPI, slaveInfo, logger);
+                triggerable = new BuildbotTriggerable(config, TestServer.remoteAPI(), MockRemoteAPI, slaveInfo, logger);
                 syncPromise = triggerable.syncOnce();
                 return MockRemoteAPI.waitForRequest();
             }).then(() => {
-                assert.equal(MockRemoteAPI.requests.length, 1);
-                assert.equal(MockRemoteAPI.requests[0].method, 'GET');
-                assert.equal(MockRemoteAPI.requests[0].url, '/json/builders/some-builder-1/pendingBuilds');
-                MockRemoteAPI.requests[0].resolve([MockData.pendingBuild({buildRequestId: 711})]);
+                assert.equal(requests.length, 1);
+                assertRequestAndResolve(requests[0], 'GET', '/json/builders/some-builder-1/pendingBuilds', []);
                 return MockRemoteAPI.waitForRequest();
             }).then(() => {
-                assert.equal(MockRemoteAPI.requests.length, 2);
-                assert.equal(MockRemoteAPI.requests[1].method, 'GET');
-                assert.equal(MockRemoteAPI.requests[1].url, '/json/builders/some-builder-1/builds/?select=-1&select=-2');
-                MockRemoteAPI.requests[1].resolve({[-1]: MockData.runningBuild({buildRequestId: 700}), [-2]: MockData.finishedBuild({buildRequestId: 710})});
+                assert.equal(requests.length, 2);
+                assertRequestAndResolve(requests[1], 'GET', '/json/builders/some-builder-1/builds/?select=-1&select=-2',
+                    {[-1]: MockData.runningBuild({buildRequestId: 700}), [-2]: MockData.finishedBuild({buildRequestId: 710})});
                 return MockRemoteAPI.waitForRequest();
             }).then(() => {
-                assert.equal(MockRemoteAPI.requests.length, 3);
-                assert.equal(MockRemoteAPI.requests[2].method, 'GET');
-                assert.equal(MockRemoteAPI.requests[2].url, '/json/builders/some-builder-1/pendingBuilds');
-                MockRemoteAPI.requests[2].resolve([MockData.pendingBuild({buildRequestId: 701})]);
+                assert.equal(requests.length, 3);
+                assertRequestAndResolve(requests[2], 'POST', '/builders/some-builder-1/force');
+                assert.deepEqual(requests[2].data, {'wk': '192736', 'os': '10.11 15A284', 'build-request-id': '701'});
                 return MockRemoteAPI.waitForRequest();
             }).then(() => {
-                assert.equal(MockRemoteAPI.requests.length, 4);
-                assert.equal(MockRemoteAPI.requests[3].method, 'GET');
-                assert.equal(MockRemoteAPI.requests[3].url, '/json/builders/some-builder-1/builds/?select=-1&select=-2');
-                MockRemoteAPI.requests[3].resolve({[-1]: MockData.runningBuild({buildRequestId: 700}), [-2]: MockData.finishedBuild({buildRequestId: 710})});
+                assert.equal(requests.length, 4);
+                assertRequestAndResolve(requests[3], 'GET', '/json/builders/some-builder-1/pendingBuilds',
+                    [MockData.pendingBuild({buildRequestId: 701})]);
+                return MockRemoteAPI.waitForRequest();
+            }).then(() => {
+                assert.equal(requests.length, 5);
+                assertRequestAndResolve(requests[4], 'GET', '/json/builders/some-builder-1/builds/?select=-1&select=-2',
+                    {[-1]: MockData.runningBuild({buildRequestId: 700}), [-2]: MockData.finishedBuild({buildRequestId: 710})});
                 return syncPromise;
             }).then(() => {
-                assert.equal(BuildRequest.all().length, 8);
-                assert.equal(BuildRequest.findById(700).status(), 'completed');
-                assert.equal(BuildRequest.findById(700).statusUrl(), null);
-                assert.equal(BuildRequest.findById(701).status(), 'pending');
-                assert.equal(BuildRequest.findById(701).statusUrl(), null);
-                assert.equal(BuildRequest.findById(702).status(), 'pending');
-                assert.equal(BuildRequest.findById(702).statusUrl(), null);
-                assert.equal(BuildRequest.findById(703).status(), 'pending');
-                assert.equal(BuildRequest.findById(703).statusUrl(), null);
-                assert.equal(BuildRequest.findById(710).status(), 'completed');
-                assert.equal(BuildRequest.findById(710).statusUrl(), null);
-                assert.equal(BuildRequest.findById(711).status(), 'pending');
-                assert.equal(BuildRequest.findById(711).statusUrl(), null);
-                assert.equal(BuildRequest.findById(712).status(), 'pending');
-                assert.equal(BuildRequest.findById(712).statusUrl(), null);
-                assert.equal(BuildRequest.findById(713).status(), 'pending');
-                assert.equal(BuildRequest.findById(713).statusUrl(), null);
-                return BuildRequest.fetchForTriggerable(MockData.mockTestSyncConfigWithTwoBuilders().triggerableName);
+                syncPromise = triggerable.syncOnce();
+                return MockRemoteAPI.waitForRequest();
             }).then(() => {
-                assert.equal(BuildRequest.all().length, 8);
-                assert.equal(BuildRequest.findById(700).status(), 'completed');
-                assert.equal(BuildRequest.findById(700).statusUrl(), 'http://build.webkit.org/builders/some-builder-1/builds/124');
-                assert.equal(BuildRequest.findById(701).status(), 'scheduled');
-                assert.equal(BuildRequest.findById(701).statusUrl(), 'http://build.webkit.org/builders/some-builder-1/');
-                assert.equal(BuildRequest.findById(702).status(), 'pending');
-                assert.equal(BuildRequest.findById(702).statusUrl(), null);
-                assert.equal(BuildRequest.findById(703).status(), 'pending');
-                assert.equal(BuildRequest.findById(703).statusUrl(), null);
-                assert.equal(BuildRequest.findById(710).status(), 'completed');
-                assert.equal(BuildRequest.findById(710).statusUrl(), 'http://build.webkit.org/builders/some-builder-1/builds/123');
-                assert.equal(BuildRequest.findById(711).status(), 'pending');
-                assert.equal(BuildRequest.findById(711).statusUrl(), null);
-                assert.equal(BuildRequest.findById(712).status(), 'pending');
-                assert.equal(BuildRequest.findById(712).statusUrl(), null);
-                assert.equal(BuildRequest.findById(713).status(), 'pending');
-                assert.equal(BuildRequest.findById(713).statusUrl(), null);
+                assert.equal(requests.length, 6);
+                assertRequestAndResolve(requests[5], 'GET', '/json/builders/some-builder-1/pendingBuilds', []);
+                return MockRemoteAPI.waitForRequest();
+            }).then(() => {
+                assert.equal(requests.length, 7);
+                assertRequestAndResolve(requests[6], 'GET', '/json/builders/some-builder-1/builds/?select=-1&select=-2',
+                    {[-1]: MockData.runningBuild({buildRequestId: 701}), [-2]: MockData.runningBuild({buildRequestId: 700})});
+                return MockRemoteAPI.waitForRequest();
+            }).then(() => {
+                assert.equal(requests.length, 8);
+                assertRequestAndResolve(requests[7], 'GET', '/json/builders/some-builder-1/pendingBuilds', []);
+                return MockRemoteAPI.waitForRequest();
+            }).then(() => {
+                assert.equal(requests.length, 9);
+                assertRequestAndResolve(requests[8], 'GET', '/json/builders/some-builder-1/builds/?select=-1&select=-2',
+                    {[-1]: MockData.runningBuild({buildRequestId: 701}), [-2]: MockData.runningBuild({buildRequestId: 700})});
+                return syncPromise;
             });
         });
 
index 7abb0c9..2d83a69 100644 (file)
@@ -124,7 +124,13 @@ class BuildbotSyncer {
         let hasPendingBuildsWithoutSlaveNameSpecified = false;
         let usedSlaves = new Set;
         for (let entry of this._entryList) {
-            if (entry.isPending()) {
+            let entryPreventsNewRequest = entry.isPending();
+            if (entry.isInProgress()) {
+                const requestInProgress = BuildRequest.findById(entry.buildRequestId());
+                if (!requestInProgress || requestInProgress.testGroupId() != newRequest.testGroupId())
+                    entryPreventsNewRequest = true;
+            }
+            if (entryPreventsNewRequest) {
                 if (!entry.slaveName())
                     hasPendingBuildsWithoutSlaveNameSpecified = true;
                 usedSlaves.add(entry.slaveName());