Re: [PATCH 1/6] GITWEB - Load Checking
flat view
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:47:53
"John 'Warthog9' Hawley" [off-list ref] writes:
This changes the behavior, slightly, of gitweb so that it verifies that the box isn't inundated with before attempting to serve gitweb. If the box is overloaded, it basically returns a 503 server unavailable until the load falls below the defined threshold. This helps dramatically if you have a box that's I/O bound, reaches a certain load and you don't want gitweb, the I/O hog that it is, increasing the pain the server is already undergoing. adds $maxload configuration variable. Default is a load of 300, which for most cases should never be hit.
Your patch doesn't allow for *turning off* this feature. Reasonable solution would be to use 'undef' or negative number to turn off this check (this feature).
Please note this makes the assumption that /proc/loadavg exists as there is no good way to read load averages on a great number of platforms [READ: Windows], or that it's reasonably accurate.
What about MacOS X, or FreeBSD, or OpenSolaris? You should mention that it is intended that if gitweb cannot read load average (for example /proc/loadavg does not exist), then the feature is turned off, i.e. the check always succeeds. Which is reasonable.
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
Why signoff is different from author (warthog9@kernel.org)? Why this email for signoff? Just curious...
quoted hunk ↗ jump to hunk
--- gitweb/gitweb.perl | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+), 0 deletions(-)
Please post patches inline, not as attachement.
quoted hunk ↗ jump to hunk
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 7e477af..813e48f 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl@@ -221,6 +221,11 @@ our %avatar_size = ( 'double' => 32 ); +# Used to set the maximum load that we will still respond to gitweb queries. +# if we exceed this than we do the processing to figure out if there's a mirror +# and redirect to it, or to just return 503 server busy
I'd probably say: +# Used to set the maximum load that we will still respond to gitweb queries. +# If server load exceed this value then return "503 server busy" error, +# (it is also possible to redirect to mirror, if it exists, instead).
quoted hunk ↗ jump to hunk
+our $maxload = 300; + # You define site-wide feature defaults here; override them with # $GITWEB_CONFIG as necessary. our %feature = (@@ -551,6 +556,25 @@ if (-e $GITWEB_CONFIG) { do $GITWEB_CONFIG_SYSTEM if -e $GITWEB_CONFIG_SYSTEM; } +# loadavg throttle +sub get_loadavg() { + my $load; + my @loads; + + open($load, '<', '/proc/loadavg') or return 0;
Why not use one of existing CPAN modules: Sys::Info::Device::CPU, BSD::getloadavg, Sys::CpuLoad? Style: + open (my $load, '<', '/proc/loadavg') or return 0; and of course no "my $load" at beginning. Also perhaps $fh, or $loadfh instead of $load? But this is a minor nit.
quoted hunk ↗ jump to hunk
+ @loads = split(/\s+/, scalar <$load>); + close($load); + return $loads[0]; +} + +if (get_loadavg() > $maxload) { + print "Content-Type: text/plain\n"; + print "Status: 503 Excessive load on server\n"; + print "\n"; + print "The load average on the server is too high\n"; + exit 0;
Why not use die_error subroutine? Is it to have generate absolutely minimal load, and that is why you do not use die_error(), or even $cgi->header()? Wouldn't a better solution be to use here-doc syntax? + print <<'EOF'; +Content-Type: text/plain; charset=utf-8 +Status: 503 Excessive load on server + +The load average on the server is too high +EOF + exit 0;
quoted hunk ↗ jump to hunk
+} + # version of the core git binary our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown"; $number_of_git_cmds++;
-- Jakub Narebski Poland ShadeHawk on #git