Re: [PATCH 1/6] GITWEB - Load Checking

[Jakub Narebski <jnareb@gmail.com>]

"John 'Warthog9' Hawley" <warthog9@kernel.org> 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...

> ---
>  gitweb/gitweb.perl |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-) 

Please post patches inline, not as attachement.

> 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).

> +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.

> +    @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;


> +}
> +
>  # 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