sync-buildbot.js can schedule more than one build per builder
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 31 Mar 2017 02:33:44 +0000 (02:33 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 31 Mar 2017 02:33:44 +0000 (02:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=170318

Reviewed by Saam Barati.

The bug was caused by _scheduleNextRequestInGroupIfSlaveIsAvailable not returning a promise when
scheduling the first build request of a test group. This resulted in _pullBuildbotOnAllSyncers
to prematurely resolve before POST'ing new build had finished. That in turn could result in the
next cycle of syncing to occur before POST'ing has actually taken place.

More precisely, when the nextRequest was the first request or its associated syncer object could
not be identified, we were supposed to find the first available syncer, schedule the request,
and then return the promise returned by scheduleRequestInGroupIfAvailable. However, the for loop
which called scheduleRequestInGroupIfAvailable on every syncer was declaring its own variable
named "promise" thereby shadowing the outer variable, which is returned to the caller.

Fixed the bug by not declaring a shadowing variable, and refactored the code. Namely, the only
reason we had such a complicated logic with two local variables, promise and syncer, was so that
we could log that we're scheduling a build. Extracted this code as _scheduleRequestWithLog.

_scheduleNextRequestInGroupIfSlaveIsAvailable can now simply exit early with a call to
_scheduleRequestWithLog when the syncer is readily identified. When looping over syncers, it can
simply return the first non-null result of _scheduleNextRequestInGroupIfSlaveIsAvailable.

* server-tests/tools-buildbot-triggerable-tests.js: Added a test case where we wait 10ms after
receiving the request to POST a build. There should be no new network request until we resolve
this request.

* tools/js/buildbot-triggerable.js:
(BuildbotTriggerable.prototype._scheduleNextRequestInGroupIfSlaveIsAvailable): Fixed the bug.
(BuildbotTriggerable.prototype._scheduleRequestWithLog): Extracted.

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js
Websites/perf.webkit.org/tools/js/buildbot-triggerable.js

index 0e8640c..430d14f 100644 (file)
@@ -1,5 +1,39 @@
 2017-03-30  Ryosuke Niwa  <rniwa@webkit.org>
 
+        sync-buildbot.js can schedule more than one build per builder
+        https://bugs.webkit.org/show_bug.cgi?id=170318
+
+        Reviewed by Saam Barati.
+
+        The bug was caused by _scheduleNextRequestInGroupIfSlaveIsAvailable not returning a promise when
+        scheduling the first build request of a test group. This resulted in _pullBuildbotOnAllSyncers
+        to prematurely resolve before POST'ing new build had finished. That in turn could result in the
+        next cycle of syncing to occur before POST'ing has actually taken place.
+
+        More precisely, when the nextRequest was the first request or its associated syncer object could
+        not be identified, we were supposed to find the first available syncer, schedule the request,
+        and then return the promise returned by scheduleRequestInGroupIfAvailable. However, the for loop
+        which called scheduleRequestInGroupIfAvailable on every syncer was declaring its own variable
+        named "promise" thereby shadowing the outer variable, which is returned to the caller.
+
+        Fixed the bug by not declaring a shadowing variable, and refactored the code. Namely, the only
+        reason we had such a complicated logic with two local variables, promise and syncer, was so that
+        we could log that we're scheduling a build. Extracted this code as _scheduleRequestWithLog.
+
+        _scheduleNextRequestInGroupIfSlaveIsAvailable can now simply exit early with a call to
+        _scheduleRequestWithLog when the syncer is readily identified. When looping over syncers, it can
+        simply return the first non-null result of _scheduleNextRequestInGroupIfSlaveIsAvailable.
+
+        * server-tests/tools-buildbot-triggerable-tests.js: Added a test case where we wait 10ms after
+        receiving the request to POST a build. There should be no new network request until we resolve
+        this request.
+
+        * tools/js/buildbot-triggerable.js:
+        (BuildbotTriggerable.prototype._scheduleNextRequestInGroupIfSlaveIsAvailable): Fixed the bug.
+        (BuildbotTriggerable.prototype._scheduleRequestWithLog): Extracted.
+
+2017-03-30  Ryosuke Niwa  <rniwa@webkit.org>
+
         Modernize BuildbotSyncer and BuildbotTriggerable
         https://bugs.webkit.org/show_bug.cgi?id=170310
 
index cae0bef..814b9aa 100644 (file)
@@ -610,6 +610,77 @@ describe('BuildbotTriggerable', function () {
             });
         });
 
+        it('should wait for POST to complete before trying to poll buildbot again', () => {
+            const db = TestServer.database();
+            const requests = MockRemoteAPI.requests;
+            let syncPromise;
+            return Promise.all([
+                MockData.addMockData(db, ['pending', 'pending', 'pending', 'pending']),
+                MockData.addAnotherMockTestGroup(db, ['pending', 'pending', 'pending', 'pending'])
+            ]).then(() => Manifest.fetch()).then(() => {
+                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);
+                syncPromise = triggerable.syncOnce();
+                return MockRemoteAPI.waitForRequest();
+            }).then(() => {
+                assert.equal(requests.length, 1);
+                assert.equal(requests[0].method, 'GET');
+                assert.equal(requests[0].url, '/json/builders/some-builder-1/pendingBuilds');
+                MockRemoteAPI.requests[0].resolve([]);
+                return MockRemoteAPI.waitForRequest();
+            }).then(() => {
+                assert.equal(requests.length, 2);
+                assert.equal(requests[1].method, 'GET');
+                assert.equal(requests[1].url, '/json/builders/some-builder-1/builds/?select=-1&select=-2');
+                requests[1].resolve({});
+                return MockRemoteAPI.waitForRequest();
+            }).then(() => {
+                assert.equal(requests.length, 3);
+                assert.equal(requests[2].method, 'POST');
+                assert.equal(requests[2].url, '/builders/some-builder-1/force');
+                assert.deepEqual(requests[2].data, {'wk': '191622', 'os': '10.11 15A284', 'build-request-id': '700'});
+                return new Promise((resolve) => setTimeout(resolve, 10));
+            }).then(() => {
+                assert.equal(requests.length, 3);
+                requests[2].resolve('OK');
+                return MockRemoteAPI.waitForRequest();
+            }).then(() => {
+                assert.equal(requests.length, 4);
+                assert.equal(requests[3].method, 'GET');
+                assert.equal(requests[3].url, '/json/builders/some-builder-1/pendingBuilds');
+                MockRemoteAPI.requests[3].resolve([MockData.pendingBuild({buildRequestId: 700})]);
+                return MockRemoteAPI.waitForRequest();
+            }).then(() => {
+                assert.equal(requests.length, 5);
+                assert.equal(requests[4].method, 'GET');
+                assert.equal(requests[4].url, '/json/builders/some-builder-1/builds/?select=-1&select=-2');
+                requests[4].resolve({});
+                return syncPromise;
+            }).then(() => {
+                return BuildRequest.fetchForTriggerable(MockData.mockTestSyncConfigWithTwoBuilders().triggerableName);
+            }).then(() => {
+                assert.equal(BuildRequest.all().length, 8);
+                assert.equal(BuildRequest.findById(700).status(), 'scheduled');
+                assert.equal(BuildRequest.findById(700).statusUrl(), 'http://build.webkit.org/builders/some-builder-1/');
+                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(), 'pending');
+                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);
+            });
+        });
+
         it('should recover from multiple test groups running simultenously', () => {
             const db = TestServer.database();
             let syncPromise;
index c1274d6..5d9f7e6 100644 (file)
@@ -143,32 +143,28 @@ class BuildbotTriggerable {
         if (!nextRequest)
             return null;
 
-        let promise;
-        let syncer;
         if (!!nextRequest.order()) {
-            syncer = groupInfo.syncer;
-            if (!syncer)
-                this._logger.error(`Could not identify the syncer for ${nextRequest.id()}.`);
-            else
-                promise = syncer.scheduleRequestInGroupIfAvailable(nextRequest, groupInfo.slaveName);
+            const syncer = groupInfo.syncer;
+            if (syncer)
+                return this._scheduleRequestWithLog(syncer, nextRequest, groupInfo.slaveName);
+            this._logger.error(`Could not identify the syncer for ${nextRequest.id()}.`);
         }
 
-        if (!syncer) {
-            for (syncer of this._syncers) {
-                // FIXME: This shouldn't be a new variable.
-                let promise = syncer.scheduleRequestInGroupIfAvailable(nextRequest);
-                if (promise)
-                    break;
-            }
+        for (const syncer of this._syncers) {
+            const promise = this._scheduleRequestWithLog(syncer, nextRequest, null);
+            if (promise)
+                return promise;
         }
+        return null;
+    }
 
-        if (promise) {
-            let slaveName = groupInfo.slaveName ? ` on ${groupInfo.slaveName}` : '';
-            this._logger.log(`Scheduling build request ${nextRequest.id()}${slaveName} in ${syncer.builderName()}`);
+    _scheduleRequestWithLog(syncer, request, slaveName)
+    {
+        const promise = syncer.scheduleRequestInGroupIfAvailable(request, slaveName);
+        if (!promise)
             return promise;
-        }
-
-        return null;
+        this._logger.log(`Scheduling build request ${request.id()}${slaveName ? ' on ' + slaveName : ''} in ${syncer.builderName()}`);
+        return promise;
     }
 
     static _testGroupMapForBuildRequests(buildRequests)