Merge ~cjwatson/turnip:cgit-timeout into turnip:master

Proposed by Colin Watson on 2019-07-05
Status: Merged
Approved by: Colin Watson on 2019-07-08
Approved revision: acb5fafdd667cbafb5536403ab1d509632169585
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/turnip:cgit-timeout
Merge into: turnip:master
Diff against target: 25 lines (+5/-1)
2 files modified
charm/turnip-pack-frontend-http/config.yaml (+4/-0)
charm/turnip-pack-frontend-http/templates/cgitwrap.j2 (+1/-1)
Reviewer Review Type Date Requested Status
Jonathan Hartley (community) 2019-07-05 Approve on 2019-07-08
Review via email:

Commit message

Time out cgit processes

We don't want these to hang around forever if something is going wrong.
cgit can legitimately take a while to render large files, so I started
out with a relatively lenient value of 30 seconds and we can see how
things look in practice.

To post a comment you must log in.
Jonathan Hartley (tartley) wrote :

For my education: This is the kind of change that isn't amenable to unit tests, but could be amenable to end-to-end tests, of a kind I think that we don't have any of, around this bit of code. Is that correct?

Are there reasons we don't have them that I ought to know about, other than the generic 'It's hard and might not be worth the effort" ?

I'm only asking for my education, no judgement! :-)

Thank you!

review: Approve
Colin Watson (cjwatson) wrote :

We indeed don't have end-to-end tests of this sort of thing at present. It's certainly doable in principle but as you say would be a very considerable amount of effort, as well as being slow to run because deploying the full stack with Juju takes quite a while. At the moment it isn't clear that it would repay the investment.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/charm/turnip-pack-frontend-http/config.yaml b/charm/turnip-pack-frontend-http/config.yaml
2index 31f0476..56cac33 100644
3--- a/charm/turnip-pack-frontend-http/config.yaml
4+++ b/charm/turnip-pack-frontend-http/config.yaml
5@@ -35,6 +35,10 @@ options:
6 type: string
7 default: ''
8 description: Base64 encoded cgit session secret.
9+ cgit_timeout:
10+ type: int
11+ default: 30
12+ description: Time out cgit processes after this many seconds.
13 virtinfo_endpoint:
14 type: string
15 default: http://localhost:6543/githosting
16diff --git a/charm/turnip-pack-frontend-http/templates/cgitwrap.j2 b/charm/turnip-pack-frontend-http/templates/cgitwrap.j2
17index 7d1fcd2..0a0bdb9 100644
18--- a/charm/turnip-pack-frontend-http/templates/cgitwrap.j2
19+++ b/charm/turnip-pack-frontend-http/templates/cgitwrap.j2
20@@ -1,5 +1,5 @@
21 #! /bin/sh
22 set -e
24-exec sudo -u {{ cgit_user }} /usr/lib/cgit/cgit.cgi "$@"
25+exec timeout --kill-after=15 {{ cgit_timeout }} sudo -u {{ cgit_user }} /usr/lib/cgit/cgit.cgi "$@"


People subscribed via source and target branches