New syncing script sometimes schedules a build request on a wrong builder
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Apr 2016 03:18:16 +0000 (03:18 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Apr 2016 03:18:16 +0000 (03:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=156489

Reviewed by Stephanie Lewis.

The bug was caused by _scheduleNextRequestInGroupIfSlaveIsAvailable scheduling the next build request on
any available syncer regardless of whether the request is the first one in the test group or not because
BuildRequest.order was returning a string instead of a number.

Also fixed a bug that BuildbotTriggerable.syncOnce was re-ordering test groups by their id's instead of
respecting the order in which the perf dashboard returned.

* public/v3/models/build-request.js:
(BuildRequest.prototype.order): Force the order to be a number.
* server-tests/api-build-requests-tests.js: Assert the order as numbers.
* server-tests/resources/mock-data.js:
(MockData.addAnotherMockTestGroup): Changed the test group id to 601, which is after the first mock data.
The old number was masking a bug in BuildbotTriggerable that it was re-ordering test groups by their id's
instead of using the order set forth by the perf dashboard.
(MockData.mockTestSyncConfigWithSingleBuilder):
* server-tests/tools-buildbot-triggerable-tests.js: Added a test case for scheduling two build requests in
a single call to syncOnce. Each build request should be scheduled on the same builder as the previous build
requests in the same test group.
* tools/js/buildbot-triggerable.js:
(BuildbotTriggerable.prototype.syncOnce): Order test groups by groupOrder, which is the index at which first
build request in the group appeared.
(BuildbotTriggerable.prototype._scheduleNextRequestInGroupIfSlaveIsAvailable): Don't re-order build requests
as they're already sorted on the server side.
(BuildbotTriggerable._testGroupMapForBuildRequests): Added groupOrder to test group info

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/v3/models/build-request.js
Websites/perf.webkit.org/server-tests/api-build-requests-tests.js
Websites/perf.webkit.org/server-tests/resources/mock-data.js
Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js
Websites/perf.webkit.org/tools/js/buildbot-triggerable.js

index d74c932..802e7b4 100644 (file)
@@ -1,3 +1,35 @@
+2016-04-11  Ryosuke Niwa  <rniwa@webkit.org>
+
+        New syncing script sometimes schedules a build request on a wrong builder
+        https://bugs.webkit.org/show_bug.cgi?id=156489
+
+        Reviewed by Stephanie Lewis.
+
+        The bug was caused by _scheduleNextRequestInGroupIfSlaveIsAvailable scheduling the next build request on
+        any available syncer regardless of whether the request is the first one in the test group or not because
+        BuildRequest.order was returning a string instead of a number.
+
+        Also fixed a bug that BuildbotTriggerable.syncOnce was re-ordering test groups by their id's instead of
+        respecting the order in which the perf dashboard returned.
+
+        * public/v3/models/build-request.js:
+        (BuildRequest.prototype.order): Force the order to be a number.
+        * server-tests/api-build-requests-tests.js: Assert the order as numbers.
+        * server-tests/resources/mock-data.js:
+        (MockData.addAnotherMockTestGroup): Changed the test group id to 601, which is after the first mock data.
+        The old number was masking a bug in BuildbotTriggerable that it was re-ordering test groups by their id's
+        instead of using the order set forth by the perf dashboard.
+        (MockData.mockTestSyncConfigWithSingleBuilder):
+        * server-tests/tools-buildbot-triggerable-tests.js: Added a test case for scheduling two build requests in
+        a single call to syncOnce. Each build request should be scheduled on the same builder as the previous build
+        requests in the same test group.
+        * tools/js/buildbot-triggerable.js:
+        (BuildbotTriggerable.prototype.syncOnce): Order test groups by groupOrder, which is the index at which first
+        build request in the group appeared.
+        (BuildbotTriggerable.prototype._scheduleNextRequestInGroupIfSlaveIsAvailable): Don't re-order build requests
+        as they're already sorted on the server side.
+        (BuildbotTriggerable._testGroupMapForBuildRequests): Added groupOrder to test group info 
+
 2016-04-09  Ryosuke Niwa  <rniwa@webkit.org>
 
         Build fix. Don't treat a build number 0 as a pending build.
index 92f8232..7d74a63 100644 (file)
@@ -37,7 +37,7 @@ class BuildRequest extends DataModelObject {
     testGroup() { return this._testGroup; }
     platform() { return this._platform; }
     test() { return this._test; }
-    order() { return this._order; }
+    order() { return +this._order; }
     rootSet() { return this._rootSet; }
 
     status() { return this._status; }
index 1aa79a3..6d9d80a 100644 (file)
@@ -338,21 +338,29 @@ describe('/api/build-requests', function () {
             assert.equal(buildRequests.length, 8);
             assert.equal(buildRequests[0].id(), 700);
             assert.equal(buildRequests[0].testGroupId(), 600);
+            assert.strictEqual(buildRequests[0].order(), 0);
             assert.equal(buildRequests[1].id(), 701);
             assert.equal(buildRequests[1].testGroupId(), 600);
+            assert.strictEqual(buildRequests[1].order(), 1);
             assert.equal(buildRequests[2].id(), 702);
             assert.equal(buildRequests[2].testGroupId(), 600);
+            assert.strictEqual(buildRequests[2].order(), 2);
             assert.equal(buildRequests[3].id(), 703);
             assert.equal(buildRequests[3].testGroupId(), 600);
+            assert.strictEqual(buildRequests[3].order(), 3);
 
             assert.equal(buildRequests[4].id(), 710);
-            assert.equal(buildRequests[4].testGroupId(), 599);
+            assert.equal(buildRequests[4].testGroupId(), 601);
+            assert.strictEqual(buildRequests[4].order(), 0);
             assert.equal(buildRequests[5].id(), 711);
-            assert.equal(buildRequests[5].testGroupId(), 599);
+            assert.equal(buildRequests[5].testGroupId(), 601);
+            assert.strictEqual(buildRequests[5].order(), 1);
             assert.equal(buildRequests[6].id(), 712);
-            assert.equal(buildRequests[6].testGroupId(), 599);
+            assert.equal(buildRequests[6].testGroupId(), 601);
+            assert.strictEqual(buildRequests[6].order(), 2);
             assert.equal(buildRequests[7].id(), 713);
-            assert.equal(buildRequests[7].testGroupId(), 599);
+            assert.equal(buildRequests[7].testGroupId(), 601);
+            assert.strictEqual(buildRequests[7].order(), 3);
             done();
         }).catch(done);
     });
@@ -368,22 +376,30 @@ describe('/api/build-requests', function () {
         }).then(function (buildRequests) {
             assert.equal(buildRequests.length, 8);
             assert.equal(buildRequests[0].id(), 710);
-            assert.equal(buildRequests[0].testGroupId(), 599);
+            assert.equal(buildRequests[0].testGroupId(), 601);
+            assert.strictEqual(buildRequests[0].order(), 0);
             assert.equal(buildRequests[1].id(), 711);
-            assert.equal(buildRequests[1].testGroupId(), 599);
+            assert.equal(buildRequests[1].testGroupId(), 601);
+            assert.strictEqual(buildRequests[1].order(), 1);
             assert.equal(buildRequests[2].id(), 712);
-            assert.equal(buildRequests[2].testGroupId(), 599);
+            assert.equal(buildRequests[2].testGroupId(), 601);
+            assert.strictEqual(buildRequests[2].order(), 2);
             assert.equal(buildRequests[3].id(), 713);
-            assert.equal(buildRequests[3].testGroupId(), 599);
+            assert.equal(buildRequests[3].testGroupId(), 601);
+            assert.strictEqual(buildRequests[3].order(), 3);
 
             assert.equal(buildRequests[4].id(), 700);
             assert.equal(buildRequests[4].testGroupId(), 600);
+            assert.strictEqual(buildRequests[4].order(), 0);
             assert.equal(buildRequests[5].id(), 701);
             assert.equal(buildRequests[5].testGroupId(), 600);
+            assert.strictEqual(buildRequests[5].order(), 1);
             assert.equal(buildRequests[6].id(), 702);
             assert.equal(buildRequests[6].testGroupId(), 600);
+            assert.strictEqual(buildRequests[6].order(), 2);
             assert.equal(buildRequests[7].id(), 703);
             assert.equal(buildRequests[7].testGroupId(), 600);
+            assert.strictEqual(buildRequests[7].order(), 3);
             done();
         }).catch(done);
     });
index 3a1e64a..2da3bd2 100644 (file)
@@ -51,11 +51,11 @@ MockData = {
         if (!statusList)
             statusList = ['pending', 'pending', 'pending', 'pending'];
         return Promise.all([
-            db.insert('analysis_test_groups', {id: 599, task: 500, name: 'another test group', author: author}),
-            db.insert('build_requests', {id: 713, status: statusList[3], triggerable: 1, platform: 65, test: 200, group: 599, order: 3, root_set: 402}),
-            db.insert('build_requests', {id: 710, status: statusList[0], triggerable: 1, platform: 65, test: 200, group: 599, order: 0, root_set: 401}),
-            db.insert('build_requests', {id: 712, status: statusList[2], triggerable: 1, platform: 65, test: 200, group: 599, order: 2, root_set: 401}),
-            db.insert('build_requests', {id: 711, status: statusList[1], triggerable: 1, platform: 65, test: 200, group: 599, order: 1, root_set: 402}),
+            db.insert('analysis_test_groups', {id: 601, task: 500, name: 'another test group', author: author}),
+            db.insert('build_requests', {id: 713, status: statusList[3], triggerable: 1, platform: 65, test: 200, group: 601, order: 3, root_set: 402}),
+            db.insert('build_requests', {id: 710, status: statusList[0], triggerable: 1, platform: 65, test: 200, group: 601, order: 0, root_set: 401}),
+            db.insert('build_requests', {id: 712, status: statusList[2], triggerable: 1, platform: 65, test: 200, group: 601, order: 2, root_set: 401}),
+            db.insert('build_requests', {id: 711, status: statusList[1], triggerable: 1, platform: 65, test: 200, group: 601, order: 1, root_set: 402}),
         ]);
     },
     mockTestSyncConfigWithSingleBuilder: function ()
index 45e3647..f9272d9 100644 (file)
@@ -545,6 +545,112 @@ describe('BuildbotTriggerable', function () {
             }).catch(done);
         });
 
+        it('should schedule a build request on the same scheduler the first request had ran', function (done) {
+            let db = TestServer.database();
+            let syncPromise;
+            db.connect().then(function () {
+                return Promise.all([
+                    MockData.addMockData(db, ['running', 'pending', 'pending', 'pending']),
+                    MockData.addAnotherMockTestGroup(db, ['running', 'pending', 'pending', 'pending'])
+                ]);
+            }).then(function () {
+                return Manifest.fetch();
+            }).then(function () {
+                let config = MockData.mockTestSyncConfigWithTwoBuilders();
+                let logger = new MockLogger;
+                let slaveInfo = {name: 'sync-slave', password: 'password'};
+                let triggerable = new BuildbotTriggerable(config, TestServer.remoteAPI(), MockRemoteAPI, slaveInfo, logger);
+                syncPromise = triggerable.syncOnce();
+                syncPromise.catch(done);
+                return MockRemoteAPI.waitForRequest();
+            }).then(function () {
+                assert.equal(MockRemoteAPI.requests.length, 2);
+                assert.equal(MockRemoteAPI.requests[0].method, 'GET');
+                assert.equal(MockRemoteAPI.requests[0].url, '/json/builders/some-builder-1/pendingBuilds');
+                MockRemoteAPI.requests[0].resolve([]);
+                assert.equal(MockRemoteAPI.requests[1].method, 'GET');
+                assert.equal(MockRemoteAPI.requests[1].url, '/json/builders/some%20builder%202/pendingBuilds');
+                MockRemoteAPI.requests[1].resolve([]);
+                return MockRemoteAPI.waitForRequest();
+            }).then(function () {
+                assert.equal(MockRemoteAPI.requests.length, 4);
+                assert.equal(MockRemoteAPI.requests[2].method, 'GET');
+                assert.equal(MockRemoteAPI.requests[2].url, '/json/builders/some-builder-1/builds/?select=-1&select=-2');
+                MockRemoteAPI.requests[2].resolve({[-1]: MockData.runningBuild({buildRequestId: 710})});
+                assert.equal(MockRemoteAPI.requests[3].method, 'GET');
+                assert.equal(MockRemoteAPI.requests[3].url, '/json/builders/some%20builder%202/builds/?select=-1&select=-2');
+                MockRemoteAPI.requests[3].resolve({[-1]: MockData.runningBuild({builder: 'some builder 2', buildRequestId: 700})});
+                return MockRemoteAPI.waitForRequest();
+            }).then(function () {
+                assert.equal(MockRemoteAPI.requests.length, 6);
+                assert.equal(MockRemoteAPI.requests[4].method, 'POST');
+                assert.equal(MockRemoteAPI.requests[4].url, '/builders/some%20builder%202/force');
+                assert.deepEqual(MockRemoteAPI.requests[4].data, {'wk': '192736', 'os': '10.11 15A284', 'build-request-id': '701'});
+                MockRemoteAPI.requests[4].resolve('OK');
+                assert.equal(MockRemoteAPI.requests[5].method, 'POST');
+                assert.equal(MockRemoteAPI.requests[5].url, '/builders/some-builder-1/force');
+                assert.deepEqual(MockRemoteAPI.requests[5].data, {'wk': '192736', 'os': '10.11 15A284', 'build-request-id': '711'});
+                MockRemoteAPI.requests[5].resolve('OK');
+                return MockRemoteAPI.waitForRequest();
+            }).then(function () {
+                assert.equal(MockRemoteAPI.requests.length, 8);
+                assert.equal(MockRemoteAPI.requests[6].method, 'GET');
+                assert.equal(MockRemoteAPI.requests[6].url, '/json/builders/some-builder-1/pendingBuilds');
+                MockRemoteAPI.requests[6].resolve([MockData.pendingBuild({buildRequestId: 711})]);
+                assert.equal(MockRemoteAPI.requests[7].method, 'GET');
+                assert.equal(MockRemoteAPI.requests[7].url, '/json/builders/some%20builder%202/pendingBuilds');
+                MockRemoteAPI.requests[7].resolve([MockData.pendingBuild({builder: 'some builder 2',buildRequestId: 701})]);
+                return MockRemoteAPI.waitForRequest();
+            }).then(function () {
+                assert.equal(MockRemoteAPI.requests.length, 10);
+                assert.equal(MockRemoteAPI.requests[8].method, 'GET');
+                assert.equal(MockRemoteAPI.requests[8].url, '/json/builders/some-builder-1/builds/?select=-1&select=-2');
+                MockRemoteAPI.requests[8].resolve({[-1]: MockData.runningBuild({buildRequestId: 710})});
+                assert.equal(MockRemoteAPI.requests[9].method, 'GET');
+                assert.equal(MockRemoteAPI.requests[9].url, '/json/builders/some%20builder%202/builds/?select=-1&select=-2');
+                MockRemoteAPI.requests[9].resolve({[-1]: MockData.runningBuild({builder: 'some builder 2', buildRequestId: 700})});
+                return syncPromise;
+            }).then(function () {
+                assert.equal(BuildRequest.all().length, 8);
+                assert.equal(BuildRequest.findById(700).status(), 'running');
+                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(), 'running');
+                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);
+            }).then(function () {
+                assert.equal(BuildRequest.all().length, 8);
+                assert.equal(BuildRequest.findById(700).status(), 'running');
+                assert.equal(BuildRequest.findById(700).statusUrl(), 'http://build.webkit.org/builders/some%20builder%202/builds/124');
+                assert.equal(BuildRequest.findById(701).status(), 'scheduled');
+                assert.equal(BuildRequest.findById(701).statusUrl(), 'http://build.webkit.org/builders/some%20builder%202/');
+                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(), 'running');
+                assert.equal(BuildRequest.findById(710).statusUrl(), 'http://build.webkit.org/builders/some-builder-1/builds/124');
+                assert.equal(BuildRequest.findById(711).status(), 'scheduled');
+                assert.equal(BuildRequest.findById(711).statusUrl(), 'http://build.webkit.org/builders/some-builder-1/');
+                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);
+                done();
+            }).catch(done);
+        });
+
         it('should update the status of a supposedly scheduled build that went missing', function (done) {
             let db = TestServer.database();
             let syncPromise;
index 95039dd..e6c9a81 100644 (file)
@@ -34,15 +34,14 @@ class BuildbotTriggerable {
 
         let self = this;
         this._logger.log(`Fetching build requests for ${this._name}...`);
-        return BuildRequest.fetchForTriggerable(this._name).then(function () {
-            let buildRequests = BuildRequest.all();
+        return BuildRequest.fetchForTriggerable(this._name).then(function (buildRequests) {
             self._validateRequests(buildRequests);
             buildReqeustsByGroup = BuildbotTriggerable._testGroupMapForBuildRequests(buildRequests);
             return self._pullBuildbotOnAllSyncers(buildReqeustsByGroup);
         }).then(function (updates) {
             self._logger.log('Scheduling builds');
             let promistList = [];
-            let testGroupList = Array.from(buildReqeustsByGroup.values()).sort(function (a, b) { return a.id - b.id; });
+            let testGroupList = Array.from(buildReqeustsByGroup.values()).sort(function (a, b) { return a.groupOrder - b.groupOrder; });
             for (let group of testGroupList) {
                 let promise = self._scheduleNextRequestInGroupIfSlaveIsAvailable(group, updates);
                 if (promise)
@@ -121,9 +120,8 @@ class BuildbotTriggerable {
 
     _scheduleNextRequestInGroupIfSlaveIsAvailable(groupInfo, pendingUpdates)
     {
-        let orderedRequests = groupInfo.requests.sort(function (a, b) { return a.order() - b.order(); });
         let nextRequest = null;
-        for (let request of orderedRequests) {
+        for (let request of groupInfo.requests) {
             if (request.isScheduled() || (request.id() in pendingUpdates && pendingUpdates[request.id()]['status'] == 'scheduled'))
                 break;
             if (request.isPending() && !(request.id() in pendingUpdates)) {
@@ -134,10 +132,9 @@ class BuildbotTriggerable {
         if (!nextRequest)
             return null;
 
-        let firstRequest = !nextRequest.order();
-        if (firstRequest) {
+        if (!!nextRequest.order()) {
             this._logger.log(`Scheduling build request ${nextRequest.id()} on ${groupInfo.slaveName} in ${groupInfo.syncer.builderName()}`);
-            return groupInfo.syncer.scheduleRequest(request, groupInfo.slaveName);
+            return groupInfo.syncer.scheduleRequest(nextRequest, groupInfo.slaveName);
         }
 
         for (let syncer of this._syncers) {
@@ -154,10 +151,11 @@ class BuildbotTriggerable {
     static _testGroupMapForBuildRequests(buildRequests)
     {
         let map = new Map;
+        let groupOrder = 0;
         for (let request of buildRequests) {
             let groupId = request.testGroupId();
             if (!map.has(groupId)) // Don't use real TestGroup objects to avoid executing postgres query in the server
-                map.set(groupId, {id: groupId, requests: [request], syncer: null, slaveName: null});
+                map.set(groupId, {id: groupId, groupOrder: groupOrder++, requests: [request], syncer: null, slaveName: null});
             else
                 map.get(groupId).requests.push(request);
         }