Repository: screwdriver-cd/screwdriver

Issue:
Builds and Stage refer to jobs from different commits
**What happened**:
https://github.com/screwdriver-cd/screwdriver/issues/2413
In addition to the above problem, similar problems occur in Stages teardown trigger.
We suggest fixes to the these problems.

# Normal Build
## problem
[pipeline.sync](https://github.com/screwdriver-cd/models/blob/3c515d63a2d7e52a25684e2ae93b552e204c90c2/lib/pipeline.js#L993-L1189) is called during event.create.
Update the job table from the target sha's screwdriver.yaml in pipeline.sync 
So if you start events for different sha's at the same time, the build will start using different sha's job settings

## fix
### data-schema
- Add a varchar(40) sha column to the jobs table

#### Note
The sha in build and event have different data column types 
build is of type [text](https://github.com/screwdriver-cd/data-schema/blob/a602054510ea429eb010cf73f7e76e3fd7f93f5e/models/build.js#L60-L63), but event is [varchar(40)](https://github.com/screwdriver-cd/data-schema/blob/a602054510ea429eb010cf73f7e76e3fd7f93f5e/models/event.js#L49-L53) 
git's sha is 40 characters long, so it is aligned to event.

### models
1. Create a function to parse and retrieve the config of a specified job from screwdriver.yaml of a specified sh like [getConfiguration in Pipeline.sync](https://github.com/screwdriver-cd/models/blob/3c515d63a2d7e52a25684e2ae93b552e204c90c2/lib/pipeline.js#L1009).
2. Before referencing job.permutations, compare the added job's sha with config.sha and if they differ, call the function you created to use the jobs' permutations from screwdriver.yaml.

#### Note 
Only retrieves jobs data from screwdriver.yaml and does not update the jobs table.
This problem occurs when different sha events are running at the same time, so when updating the jobs table, frequent job table updates can occur from each event.
This can lead to performance degradation.


# Stages
## problem
The jobIds in the stage table are updated by [pipeline.sync](https://github.com/screwdriver-cd/data-schema/blob/a602054510ea429eb010cf73f7e76e3fd7f93f5e/models/stage.js#L18-L21) at event.create as well as jobs.
These jobIds are used to [determine the start of the teardown step](https://github.com/screwdriver-cd/screwdriver/blob/4c4873592f9d7b8a329de78b49ebdbafc285e5f5/plugins/builds/helper/updateBuild.js#L353)
Therefore, as in the normal build described above, if different sha events which the jobs in a stage have different are running at the same time, the teardown step will trigger even if there are jobs in the stage that have not yet been completed, causing unexpected behaviours.
## fix
- Use event.workflowGraph.nodes in the part of [updateBuild that refers to jobIds](https://github.com/screwdriver-cd/screwdriver/blob/4c4873592f9d7b8a329de78b49ebdbafc285e5f5/plugins/builds/helper/updateBuild.js#L353).
  - In event.workflowGraph.nodes, the jobs in a stage have the property stageName, so it is more reliable to get the IDs of the jobs belonging to the stage from this nodes to get the builds.
  - It is possible to add sha columns to stages as in a normal build, but we suggest the above solution for stages, which is easier to solve.


Discussion:

Solution patch:
diff --git a/plugins/builds/helper/updateBuild.js b/plugins/builds/helper/updateBuild.js
index 6b6f411..8fe8e76 100644
--- a/plugins/builds/helper/updateBuild.js
+++ b/plugins/builds/helper/updateBuild.js
@@ -137,13 +137,29 @@ async function getStage({ stageFactory, workflowGraph, jobName, pipelineId }) {
 /**
  * Get all builds in stage
  *
- * @param  {Stage}      stage   Stage
- * @param  {Event}      event   Event
- * @return {Promise<Build[]>}   Builds in stage
+ * @param  {Stage}             stage       Stage
+ * @param  {Event}             event       Event
+ * @param  {JobFactory}        jobFactory  Job Factory instance
+ * @return {Promise<Build[]>}              Builds in stage
  */
-async function getStageJobBuilds({ stage, event }) {
+async function getStageJobBuilds({ stage, event, jobFactory }) {
     // Get all jobIds for jobs in the stage
-    const stageJobIds = [...stage.jobIds, stage.setup];
+    const stageNodes = event.workflowGraph.nodes.filter(n => {
+        const jobName = n.name.split(':')[1];
+
+        return n.stageName === stage.name && jobName !== 'teardown';
+    });
+    const stageJobIds = await Promise.all(
+        stageNodes.map(async n => {
+            if (n.id) {
+                return n.id;
+            }
+
+            const job = await jobFactory.get({ pipelineId: event.pipelineId, name: n.name });
+
+            return job.id;
+        })
+    );
 
     // Get all builds in a stage for this event
     return event.getBuilds({ params: { jobId: stageJobIds } });
@@ -350,7 +366,7 @@ async function updateBuildAndTriggerDownstreamJobs(config, build, server, userna
 
         // Start stage teardown build if stage is done
         if (stageTeardownBuild && stageTeardownBuild.status === 'CREATED') {
-            const stageJobBuilds = await getStageJobBuilds({ stage, event: newEvent });
+            const stageJobBuilds = await getStageJobBuilds({ stage, event: newEvent, jobFactory });
             const stageIsDone = isStageDone(stageJobBuilds);
 
             if (stageIsDone) {



Test patch:
diff --git a/test/plugins/builds.test.js b/test/plugins/builds.test.js
index af7157c..335b0f6 100644
--- a/test/plugins/builds.test.js
+++ b/test/plugins/builds.test.js
@@ -2102,6 +2102,9 @@ describe('build plugin test', () => {
 
                     return server.inject(options).then(reply => {
                         assert.equal(reply.statusCode, 200);
+                        const { params } = eventMock.getBuilds.getCall(0).args[0];
+
+                        assert.sameMembers(params.jobId, [stageMock.setup, ...stageMock.jobIds]);
                         assert.notCalled(buildFactoryMock.create);
                         assert.notCalled(stageBuildFactoryMock.create);
                     });
@@ -2258,6 +2261,10 @@ describe('build plugin test', () => {
 
                     return server.inject(options).then(reply => {
                         assert.equal(reply.statusCode, 200);
+
+                        const { params } = eventMock.getBuilds.getCall(0).args[0];
+
+                        assert.sameMembers(params.jobId, [stageMock.setup, ...stageMock.jobIds]);
                         assert.notCalled(buildFactoryMock.create);
                         assert.notCalled(stageBuildFactoryMock.create);
                         assert.calledOnce(stageTeardownBuildMock.start);
@@ -5684,7 +5691,11 @@ describe('build plugin test', () => {
 
                     // event
                     eventMock.workflowGraph = testWorkflowGraphWithStages;
+                    eventMock.workflowGraph.nodes.push({ name: 'append-alpha-job', stageName: 'alpha' });
                     eventFactoryMock.get.resolves(eventMock);
+                    jobFactoryMock.get
+                        .withArgs({ pipelineId: pipelineMock.id, name: 'append-alpha-job' })
+                        .resolves({ id: 999 });
 
                     // stage
                     stageFactoryMock.get.resolves(stageAlphaMock);
@@ -5795,6 +5806,9 @@ describe('build plugin test', () => {
                         assert.calledOnce(stageBuildMock.update);
                         assert.equal(stageBuildMock.status, 'FAILURE');
 
+                        const { params } = eventMock.getBuilds.getCall(0).args[0];
+
+                        assert.sameMembers(params.jobId, [stageMock.setup, ...stageMock.jobIds, 999]);
                         assert.calledOnce(buildFactoryMock.create);
                         assert.calledWith(buildFactoryMock.create, {
                             jobId: 55,
@@ -5947,8 +5961,12 @@ describe('build plugin test', () => {
 
                     return newServer.inject(localOptions).then(() => {
                         assert.notCalled(stageBuildMock.update);
-
                         assert.notCalled(buildFactoryMock.create);
+                        assert.calledTwice(eventMock.getBuilds);
+
+                        const { params } = eventMock.getBuilds.getCall(0).args[0];
+
+                        assert.sameMembers(params.jobId, [stageGammaMock.setup, ...stageGammaMock.jobIds]);
                         assert.calledTwice(buildGammaTeardown.update);
                         assert.deepEqual(buildGammaTeardown.parentBuildId, [7001, 7002, 7003, 12345]);
                         assert.deepEqual(buildGammaTeardown.parentBuilds, {
diff --git a/test/plugins/data/workflowGraphWithStages.json b/test/plugins/data/workflowGraphWithStages.json
index 5396d98..5af905e 100644
--- a/test/plugins/data/workflowGraphWithStages.json
+++ b/test/plugins/data/workflowGraphWithStages.json
@@ -2,23 +2,23 @@
     "nodes": [
         { "name": "~pr" },
         { "name": "~commit" },
-        { "name": "stage@alpha:setup", "stageName": "alpha" },
-        { "name": "alpha-deploy", "stageName": "alpha" },
-        { "name": "alpha-test", "stageName": "alpha" },
-        { "name": "alpha-certify", "stageName": "alpha" },
-        { "name": "stage@alpha:teardown", "stageName": "alpha" },
+        { "name": "stage@alpha:setup", "stageName": "alpha", "id": 11 },
+        { "name": "alpha-deploy", "stageName": "alpha", "id": 22 },
+        { "name": "alpha-test", "stageName": "alpha", "id": 33 },
+        { "name": "alpha-certify", "stageName": "alpha", "id": 44 },
+        { "name": "stage@alpha:teardown", "stageName": "alpha", "id": 55 },
         { "name": "stage@beta:setup", "stageName": "beta" },
         { "name": "beta-deploy", "stageName": "beta" },
         { "name": "beta-test", "stageName": "beta" },
         { "name": "beta-certify", "stageName": "beta" },
         { "name": "stage@beta:teardown", "stageName": "beta" },
-        { "name": "stage@gamma:setup", "stageName": "gamma" },
+        { "name": "stage@gamma:setup", "stageName": "gamma", "id": 771 },
         { "name": "triggering-a-stage" },
-        { "name": "gamma-deploy", "stageName": "gamma" },
-        { "name": "gamma-test-integration", "stageName": "gamma" },
-        { "name": "gamma-test-functional", "stageName": "gamma" },
-        { "name": "gamma-certify", "stageName": "gamma" },
-        { "name": "stage@gamma:teardown", "stageName": "gamma" },
+        { "name": "gamma-deploy", "stageName": "gamma", "id": 772 },
+        { "name": "gamma-test-integration", "stageName": "gamma", "id": 773 },
+        { "name": "gamma-test-functional", "stageName": "gamma", "id": 774 },
+        { "name": "gamma-certify", "stageName": "gamma", "id": 775 },
+        { "name": "stage@gamma:teardown", "stageName": "gamma", "id": 776 },
         { "name": "triggered-by-a-stage-job" },
         { "name": "triggered-after-a-stage" }
     ],