Screwdriver: Performance issue of webhook

[catto]

What happened:
I have a big github repository that has thousands of pull requests in it. When I open a new pull request in that repository, it takes tens of seconds to process webhook and sometimes ends with timeout by github webhook service (30s).

What you expected to happen:
Each webhook is processed in a few seconds.

How to reproduce it:

1. Create so many PRs in your repository. It doesn't matter keep them open or close
2. Open new PR or push new commit to the PR

References

1. https://github.com/screwdriver-cd/screwdriver/issues/1769
2. https://github.com/screwdriver-cd/screwdriver/issues/1737

[catto]

I had a quick look into this issue and found some points that we might be able to fix.

1. Running pipeline.sync on every PR event

Webhook plugin runs pipeline.sync operation every time after PR is opened, synced or closed. I think it doesn't needed for these events. Is it enough to call pipeline.syncPR?
https://github.com/screwdriver-cd/screwdriver/blob/5c6c4870f539a04ae829926ba52c642d70bfb4f0/plugins/webhooks/index.js#L210
https://github.com/screwdriver-cd/screwdriver/blob/5c6c4870f539a04ae829926ba52c642d70bfb4f0/plugins/webhooks/index.js#L267
https://github.com/screwdriver-cd/screwdriver/blob/5c6c4870f539a04ae829926ba52c642d70bfb4f0/plugins/webhooks/index.js#L296

2. Fetching all jobs belonging to the pipeline and instantiate them

This is related to 1.
pipeline.jobs operation fetches all jobs stored in database and creates/instantiates them even if there are thousands of archived jobs. pipelines.jobs is called by pipeline.sync and this issue got happen.
Can we avoid scanning all records for jobs? for instance skipping archived: 1 records

https://github.com/screwdriver-cd/models/blob/412f74eb2cdf6db5b65f2f940e3cb6505bab25c3/lib/pipeline.js#L639
https://github.com/screwdriver-cd/models/blob/412f74eb2cdf6db5b65f2f940e3cb6505bab25c3/lib/pipeline.js#L875

The most heaviest function call during fetching jobs is here:
https://github.com/screwdriver-cd/datastore-sequelize/blob/9c396a178a9e3254887720488b4d48a40483d847/index.js#L458-L459

The second time accessing pipeline.jobs should be faster since it already has jobs instances but it costs a lot because of the number of instances.
https://github.com/screwdriver-cd/screwdriver/blob/master/plugins/webhooks/index.js#L309

3. magic wait for 1.8 secs every time

Currently webhook plugin waits for 1.8 seconds on every PR webhooks.
I have read the original issue and found it occurs only when synchronized event. Should we run it only when event type is synchronized?

[catto]

We should check if a result of listFiles API includes the desired SHA or not instead of putting 1.8sec wait so that webhook endpoint will be faster for most cases and every PR build ensures the latest changed files are reflected to github.
https://developer.github.com/v3/pulls/#list-pull-requests-files

edit: key sha in a result of listFiles API doesn't represent a commit object 😞

[catto]

No.2 is being fixed via #1603

[jithine]

@catto one other issue I see is wrt scm.getBranchList() https://github.com/screwdriver-cd/screwdriver/blob/master/plugins/webhooks/index.js#L174

We have that for branch filtering feature for identifying triggered pipelines. However we could refactor it such that it does not reach out to Git and instead query our database for triggered pipelines.
By not calling git we are more resilient against Git errors.

[jithine]

https://github.com/screwdriver-cd/screwdriver/pull/1643/files addresses my previous comment

[jithine]

Last work has been done at https://github.com/screwdriver-cd/screwdriver/pull/1653/files which addresses https://github.com/screwdriver-cd/screwdriver/issues/1468#issuecomment-460035553

[catto]

Reopening the issue because "3. magic wait for 1.8 secs every time" I wrote is probably fixable now. Also sometimes I see issues like screwdriver.yaml not found because of magic branch is not updated. #593
Let's replace 1.8 sec wait with polling pull_request API.
https://developer.github.com/v3/git/#checking-mergeability-of-pull-requests

[tanoda]

according to GitHub doc,

• if mergeable is null, wait a sec and retry
• if mergeable is true, we can use merge refs to get yaml.
• if mergeable is false, ... maybe we want to use pipeline branch to get yaml? stop the process and failed to start building?

[jithine]

we were discussing the other day https://github.com/screwdriver-cd/screwdriver/issues/1769

current thought process is that we are doing too many processing during webhooks and should push heavy processing to an offline flow

[catto]

@jithin1987 Yes I read the issue #1769. However there are two problems around webhooks.

1. SD.cd has a lot of things to do during processing webhooks and the webhooks go timeout sometimes.
2. GitHub sends webhooks asynchronously. It doesn't ensure magic branch has been updated or not when SD.cd receives a webhook. And every build triggered by webhook wait 1.8sec even if some of them don't need to wait for the magic branches.

1769 will resolve the problem 1 by responding webhook asap but problem 2 will still remain. It also makes unit tests slow since it waits 1.8sec for each test case.

In this ticket, we will focus on fixing problem 2 so that the performance and stability can be improved by trimming 1.8s wait and ensuring existing of magic branches.
Does it make sense?

[jithine]

@catto agree. Let's tackle both angles.

[jithine]

Closing this one since we already have #1769 to tackle pending performance improvements for webhook API