Re: [PATCH 1/7] gitweb: Load checking

Re: [PATCH 1/7] gitweb: Load checking

[Jakub Narebski]

On Wed, 13 Jan 2010, John 'Warthog9' Hawley wrote:
> 
> v3 - warthog9: small additional adjustment to indicate where new load
>      checking should be added for future improvements
> v2 - Jakub: switching to using

"switching to using" what?

> v1 - warthog9: Initial creation
> 
> Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---

Also, such comments should be put in the comment section, here i.e. between
"---\n" separator and the diffstat, and not in the commit message.

>  gitweb/README      |    7 ++++++-
>  gitweb/gitweb.perl |   45 +++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 47 insertions(+), 5 deletions(-)

Other that this minor nick (which I guess could be fixed by Junio when
applying), this one looks good.

-- 
Jakub Narebski
Poland

Re: [PATCH 2/7] gitweb: Add option to force version match

[Jakub Narebski]

On Wed, 13 Jan 2010, John 'Warthog9' Hawley wrote:
> From: John 'Warthog9' Hawley <warthog9@kernel.org>
> 
> This adds $git_versions_must_match variable, which is set to true

s/is set to true value/if set to true,/

> value checks that we are running on the same version of git that we
> shipped with, and if not throw '500 Internal Server Error' error.
> What is checked is the version of gitweb (embedded in building
> gitweb.cgi), against version of runtime git binary used.
> 
> Gitweb can usually run with a mismatched git install.  This is more
> here to give an obvious warning as to whats going on vs. silently
> failing.
> 
> By default this feature is turned off.

[moved version info below, after "---" separator]

> Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---

Moved to the correct place: information about patch versioning should
be put in comment section[1], not in commit message.
 
[1] Which means between "---" separator and diffstat.

> v3 - warthog9: adjust to use die_error instead of recreating it
> v2 - Jakub: Changes to make non-default, and change naming
> v1 - warthog9: Initial

>  gitweb/README      |    3 +++
>  gitweb/gitweb.perl |   28 ++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 0 deletions(-)

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl

> @@ -587,6 +590,31 @@ if (defined $maxload && get_loadavg() > $maxload) {
>  our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
>  $number_of_git_cmds++;
>  
> +# Throw an error if git versions does not match, if $git_versions_must_match is true.
> +if ($git_versions_must_match &&
> +    $git_version ne $version) {
> +	my $admin_contact =
> +		defined $ENV{'SERVER_ADMIN'} ? ", $ENV{'SERVER_ADMIN'}," : '';
> +	my $err_msg = <<EOT;
> +Internal Server Error
> +<br />
> +</div>
> +<hr />
> +<div class="readme">
> +<h1 align="center">*** Warning ***</h1>
> +<p>
> +This version of gitweb was compiled for <b>@{[esc_html($version)]}</b>,
> +however git version <b>@{[esc_html($git_version)]}</b> was found on server,
> +and administrator requested strict version checking.
> +</p>
> +<p>
> +Please contact the server administrator${admin_contact} to either configure
> +gitweb to allow mismatched versions, or update git or gitweb installation.
> +</p>
> +EOT
> +	die_error(500, $err_msg);
> +}

I'm sorry to be this picky, but don't you think that caller should not need
to know "intimate" details on how die_error works.  In particular please
notice that you must have known that die_error wraps its output in <div>...
which is not even mentioned in comments.

The second argument of die_error() subroutine is meant to be alternative
description of error condition: short and one line.  For situations such
like this one, where we want to describe error in more details, it would
be better, I think, to change signature of die_error() to take (optionally)
three arguments, and use 'die_error(500, undef, $err_msg);', with $err_msg
starting from '<h1>...</h1>'.

@@ -3460,6 +3460,7 @@
 sub die_error {
 	my $status = shift || 500;
 	my $error = shift || "Internal server error";
+	my $extra = shift;
 
 	my %http_responses = (
 		400 => '400 Bad Request',
@@ -3475,8 +3476,13 @@ sub die_error {
 <br /><br />
 $status - $error
 <br />
-</div>
 EOF
+	if (defined $extra) {
+		print "<hr />\n" .
+		      "$extra\n";
+	}
+	print "</div>\n";
+
 	git_footer_html();
 	exit;
 }


Other that this minor issue it looks good.

-- 
Jakub Narebski
Poland

[PATCH 0/7] Gitweb caching v4

[John 'Warthog9' Hawley]

Evening everyone,
 
This is the latest incarnation of gitweb w/ caching.  This is
finally at the point where it should probably start either being
considered for inclusion or mainline, or I need to accept that this
will never get in and more perminantely fork (as is the case with
Fedora where this is going in as gitweb-caching as a parrallel rpm
package).

That said this brings the base up to mainline (again), it updates a
number of elements in the caching engine, and this is a much cleaner
break-out of the tree vs. what I am currently developing against.

v4:
	- major re-working of the caching layer to use file handle
	  redirection instead of buffering output
	- other minor improvements
v3:
	- various minor re-works based on mailing list feedback,
	  this series was not sent to the mailing list.
v2:
	- Better breakout
	- You can actually disable the cache now

- John 'Warthog9' Hawley 


John 'Warthog9' Hawley (7):
  gitweb: Load checking
  gitweb: Add option to force version match
  gitweb: Makefile improvements
  gitweb: Optionally add "git" links in project list page
  gitweb: Convert output to using indirect file handle
  gitweb: add a get function to compliment print_local_time
  gitweb: File based caching layer (from git.kernel.org)

 Makefile           |   91 ++---
 gitweb/Makefile    |  129 +++++++
 gitweb/README      |   14 +-
 gitweb/cache.pm    |  283 +++++++++++++++
 gitweb/gitweb.css  |    6 +
 gitweb/gitweb.perl | 1021 ++++++++++++++++++++++++++++++----------------------
 6 files changed, 1052 insertions(+), 492 deletions(-)
 create mode 100644 gitweb/Makefile
 create mode 100644 gitweb/cache.pm

[PATCH 1/7] gitweb: Load checking

[John 'Warthog9' Hawley]

From: John 'Warthog9' Hawley <warthog9@kernel.org>

This changes slightly the behavior 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.

This behavior is controlled by $maxload configuration variable.
Default is a load of 300, which for most cases should never be hit.
Unset it (set it to undefined value, i.e. undef) to turn off checking.

Currently it requires that '/proc/loadavg' file exists, otherwise the
load check is bypassed (load is taken to be 0).  So platforms that do
not implement '/proc/loadavg' currently cannot use this feature.
(provisions are included for additional checks to be added by others)

v3 - warthog9: small additional adjustment to indicate where new load
     checking should be added for future improvements
v2 - Jakub: switching to using
v1 - warthog9: Initial creation

Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/README      |    7 ++++++-
 gitweb/gitweb.perl |   45 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/gitweb/README b/gitweb/README
index e34ee79..6c2c8e1 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -174,7 +174,7 @@ not include variables usually directly set during build):
    Base URL for relative URLs in pages generated by gitweb,
    (e.g. $logo, $favicon, @stylesheets if they are relative URLs),
    needed and used only for URLs with nonempty PATH_INFO via
-   <base href="$base_url>.  Usually gitweb sets its value correctly,
+   <base href="$base_url">.  Usually gitweb sets its value correctly,
    and there is no need to set this variable, e.g. to $my_uri or "/".
  * $home_link
    Target of the home link on top of all pages (the first part of view
@@ -228,6 +228,11 @@ not include variables usually directly set during build):
    repositories from launching cross-site scripting (XSS) attacks.  Set this
    to true if you don't trust the content of your repositories. The default
    is false.
+ * $maxload
+   Used to set the maximum load that we will still respond to gitweb queries.
+   If server load exceed this value then return "503 Service Unavaliable" error.
+   Server load is taken to be 0 if gitweb cannot determine its value.  Set it to
+   undefined value to turn it off.  The default is 300.
 
 
 Projects list file format
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7e477af..0a07d3a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -221,6 +221,12 @@ our %avatar_size = (
 	'double'  => 32
 );
 
+# 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.
+# If gitweb cannot determined server load, it is taken to be 0.
+# Leave it undefined (or set to 'undef') to turn off load checking.
+our $maxload = 300;
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -551,6 +557,32 @@ if (-e $GITWEB_CONFIG) {
 	do $GITWEB_CONFIG_SYSTEM if -e $GITWEB_CONFIG_SYSTEM;
 }
 
+# Get loadavg of system, to compare against $maxload.
+# Currently it requires '/proc/loadavg' present to get loadavg;
+# if it is not present it returns 0, which means no load checking.
+sub get_loadavg {
+	if( -e '/proc/loadavg' ){
+		open my $fd, '<', '/proc/loadavg'
+			or return 0;
+		my @load = split(/\s+/, scalar <$fd>);
+		close $fd;
+
+		# The first three columns measure CPU and IO utilization of the last one,
+		# five, and 10 minute periods.  The fourth column shows the number of
+		# currently running processes and the total number of processes in the m/n
+		# format.  The last column displays the last process ID used.
+		return $load[0] || 0;
+	}
+	# additional checks for load average should go here for things that don't export
+	# /proc/loadavg
+
+	return 0;
+}
+
+if (defined $maxload && get_loadavg() > $maxload) {
+	die_error(503, "The load average on the server is too high");
+}
+
 # version of the core git binary
 our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
 $number_of_git_cmds++;
@@ -3354,14 +3386,19 @@ sub git_footer_html {
 # 500: The server isn't configured properly, or
 #      an internal error occurred (e.g. failed assertions caused by bugs), or
 #      an unknown error occurred (e.g. the git binary died unexpectedly).
+# 503: The server is currently unavailable (because it is overloaded,
+#      or down for maintenance).  Generally, this is a temporary state.
 sub die_error {
 	my $status = shift || 500;
 	my $error = shift || "Internal server error";
 
-	my %http_responses = (400 => '400 Bad Request',
-			      403 => '403 Forbidden',
-			      404 => '404 Not Found',
-			      500 => '500 Internal Server Error');
+	my %http_responses = (
+		400 => '400 Bad Request',
+		403 => '403 Forbidden',
+		404 => '404 Not Found',
+		500 => '500 Internal Server Error',
+		503 => '503 Service Unavailable',
+	);
 	git_header_html($http_responses{$status});
 	print <<EOF;
 <div class="page_body">
-- 
1.6.5.2