1596439 - Scriptworker on GCP: Bad status 429 when GET https://github.com/{owner}/{repo}/branch_commits/{rev}

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P1)

Description

[Johan Lorenzo [:jlorenzo]]

About 20 beetmover jobs are failing each time we trigger a new snapshot or release graph. They fail this way[1]:

2019-11-14T11:29:12    DEBUG -  GET https://github.com/mozilla-mobile/android-components/branch_commits/c83baffe6d2815098add68ca1b8e5e88596ee630
2019-11-14T11:29:12    DEBUG -  GET https://github.com/mozilla-mobile/android-components/branch_commits/c83baffe6d2815098add68ca1b8e5e88596ee630
2019-11-14T11:29:12    DEBUG -  GET https://github.com/mozilla-mobile/android-components/branch_commits/c83baffe6d2815098add68ca1b8e5e88596ee630
2019-11-14T11:29:12    DEBUG -  GET https://github.com/mozilla-mobile/android-components/branch_commits/c83baffe6d2815098add68ca1b8e5e88596ee630
2019-11-14T11:29:13    DEBUG -  GET https://github.com/mozilla-mobile/android-components/branch_commits/c83baffe6d2815098add68ca1b8e5e88596ee630
2019-11-14T11:29:13    DEBUG -  GET https://github.com/mozilla-mobile/android-components/branch_commits/c83baffe6d2815098add68ca1b8e5e88596ee630
2019-11-14T11:29:13    DEBUG -  Status 200
2019-11-14T11:29:14    DEBUG -  Status 429
2019-11-14T11:29:14    DEBUG -  Status 429
2019-11-14T11:29:14    DEBUG -  Status 200
2019-11-14T11:29:14    DEBUG -  Status 200
2019-11-14T11:29:14    DEBUG -  Status 200
2019-11-14T11:29:14    ERROR - Hit ScriptWorkerException: Bad status 429

We did have this issue when we were on AWS[2]. Although, we mitigated it by implementing a cache[3]. To be more explicit, this very endpoint is not part of the Github API, so using a Github token (like added in bug 1595740) is not going to work[4]. So, we have to work around it.

I think the fact that GCP is auto-scaling instances display this problem more than AWS did. That said, there's something I don't understand in the logs: we still call that endpoint 5 times, so I wonder if the cache we implemented is actually effective. Maybe there's some race condition with async code and the way has_commit_landed_on_repository() is currently implemented. This function did have a lock, but it was removed in [6].

I think we should bring this lock back. What do you think, Aki?

Setting highest priority because Android-Components ends up in a non-shippable state, without human interaction (which includes snapshots built twice a day).

[1] https://firefox-ci-tc.services.mozilla.com/tasks/YDtEBW7WTE29GvKLRuxrKQ/runs/0/logs/https%3A%2F%2Ffirefox-ci-tc.services.mozilla.com%2Fapi%2Fqueue%2Fv1%2Ftask%2FYDtEBW7WTE29GvKLRuxrKQ%2Fruns%2F0%2Fartifacts%2Fpublic%2Flogs%2Fchain_of_trust.log#L275
[2] https://github.com/mozilla-releng/scriptworker/issues/331
[3] https://github.com/mozilla-releng/scriptworker/pull/332
[4] https://github.com/mozilla-releng/scriptworker/pull/330#pullrequestreview-229932915
[5] https://github.com/mozilla-releng/scriptworker/blob/aec959c1c661a84e75b62c373fecefb28fdc19e2/scriptworker/github.py#L109-L147
[6] https://github.com/mozilla-releng/scriptworker/pull/369/files#diff-535d7e6afe5996726583ce040715ca13L122

Comment 1

[Aki Sasaki (not active)]

I wonder if memoizing the status would help? E.g.

• https://pypi.org/project/py-memoize/
• https://pypi.org/project/aiomemoize/
• https://pypi.org/project/aiomemoizettl/

We also could use an LRU cache if we don't want to roll our own.

Agreed, if we're calling it 5 times concurrently the cache may not help as much as it could. Ideally we can figure out what that is, and fix. Forcing that call to be synchronous is one solution, though it's a bit heavy handed. Another solution could involve an object that accepts async url requests, but translates that to a single aiohttp call per url.

We could also somehow send these requests through a caching proxy which would allow for grabbing the results from github once per url. We'd have to implement this securely and robustly. I don't love the solution, but the above solutions only solve for caching inside a single instance; this solution would cache across all scriptworker instances. Maybe let's solve for the per-instance cache first, and look at this if we still have problems.

Comment 2

[Mihai Tabara [:mtabara]⌚️GMT]

Would it make sense to downgrade the number of beetmover workers back to 2-3 while we address this?

Comment 3

[Johan Lorenzo [:jlorenzo]]

Great idea, Mihai! 😃 Let's reduce the number of instances for the weekend.

Thanks a lot for the links, Aki! https://pypi.org/project/aiomemoizettl/ seems to fit our very use case. I'll give it a try today. I agree on solving this per-instance, first.

Comment 4

[Johan Lorenzo [:jlorenzo]]

Attachment: [scriptworker] Cache calls to github's branch_commits

Comment 5

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Johan Lorenzo [:jlorenzo] from comment #3)

Great idea, Mihai! 😃 Let's reduce the number of instances for the weekend.

We're now at 2 beetmoverworkers, for now - https://github.com/mozilla-releng/k8s-autoscale/pull/65

Comment 6

[Johan Lorenzo [:jlorenzo]]

Attachment: [scriptworker-scripts] Update dependencies for scriptworker 29.0.1

Comment 7

[Johan Lorenzo [:jlorenzo]]

Attachment: [k8s-autoscale] Bring the number of mobile BMs back to 20

Comment 8

[Johan Lorenzo [:jlorenzo]]

All pieces have been deployed. We're now back at 20 beetmover instances max, and each instance is caching their calls to branch_commits. We should be good. Please let me know if you see something wrong.