[PATCH] t9501: Skip testing load if we can't detect it

[PATCH] t9501: Skip testing load if we can't detect it

[Brian Gernhardt]

Currently gitweb only knows how to check for load using /proc/loadavg,
which isn't available on all systems.  We shouldn't fail the test just
because we don't know how to check the system load.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---
 t/t9501-gitweb-standalone-http-status.sh |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 7590f10..992d729 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -115,12 +115,19 @@ test_debug 'cat gitweb.output'
 # ----------------------------------------------------------------------
 # load checking
 
+if test -e /proc/loadavg
+then
+	test_set_prereq PROC_LOADAVG
+else
+	say 'skipping load tests (no /proc/loadavg found)'
+fi
+
 # always hit the load limit
 cat >>gitweb_config.perl <<\EOF
 our $maxload = 0;
 EOF
 
-test_expect_success 'load checking: load too high (default action)' '
+test_expect_success PROC_LOADAVG 'load checking: load too high (default action)' '
 	gitweb_run "p=.git" &&
 	grep "Status: 503 Service Unavailable" gitweb.headers &&
 	grep "503 - The load average on the server is too high" gitweb.body
-- 
1.7.0.rc1.141.gd3fd2

Re: [PATCH] t9501: Skip testing load if we can't detect it

[Jakub Narebski]

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

> Currently gitweb only knows how to check for load using /proc/loadavg,
> which isn't available on all systems.  We shouldn't fail the test just
> because we don't know how to check the system load.
> 
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>

NAK.  It is not necessary, and it would be hindrance (one more place
to update) if we are to extend get_loadavg() in gitweb to work without
/proc/loadavg, e.g. via BSD::loadavg module.


Let me explain how it currently works without /proc/loadavg.  

First, load average is by definition non-negative number (>= 0).

Second, the get_loadavg() subroutine is written to be resilent and
robust, and it returns 0 if it can't get load average from system
(which _currently_ means no well-formatted /proc/loadavg file)

Third, the test (as you can see below in context line in quoted diff
below) forces gitweb to go over maximum load by setting $maxload to 0.
This means that regardless of true load, and regardless whether gitweb
can detect system load (remember that if it cant get system load it
returns 0 instead) gitweb would be in "load too high" situation.

> ---
>  t/t9501-gitweb-standalone-http-status.sh |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
> index 7590f10..992d729 100755
> --- a/t/t9501-gitweb-standalone-http-status.sh
> +++ b/t/t9501-gitweb-standalone-http-status.sh
> @@ -115,12 +115,19 @@ test_debug 'cat gitweb.output'
>  # ----------------------------------------------------------------------
>  # load checking
>  
> +if test -e /proc/loadavg
> +then
> +	test_set_prereq PROC_LOADAVG
> +else
> +	say 'skipping load tests (no /proc/loadavg found)'
> +fi
> +
>  # always hit the load limit
>  cat >>gitweb_config.perl <<\EOF
>  our $maxload = 0;
>  EOF
>  
> -test_expect_success 'load checking: load too high (default action)' '
> +test_expect_success PROC_LOADAVG 'load checking: load too high (default action)' '
>  	gitweb_run "p=.git" &&
>  	grep "Status: 503 Service Unavailable" gitweb.headers &&
>  	grep "503 - The load average on the server is too high" gitweb.body
> -- 
> 1.7.0.rc1.141.gd3fd2
> 

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] t9501: Skip testing load if we can't detect it

[Brian Gernhardt]

On Feb 6, 2010, at 6:22 AM, Jakub Narebski wrote:

> Brian Gernhardt <brian@gernhardtsoftware.com> writes:
> 
>> Currently gitweb only knows how to check for load using /proc/loadavg,
>> which isn't available on all systems.  We shouldn't fail the test just
>> because we don't know how to check the system load.
>> 
>> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> 
> NAK.  It is not necessary, and it would be hindrance (one more place
> to update) if we are to extend get_loadavg() in gitweb to work without
> /proc/loadavg, e.g. via BSD::loadavg module.

Without this patch, the test fails on my OS X machine (which doesn't have /proc).  So _something_ is necessary.  Skipping the test because we can't use the feature on the host machine seemed more in line with what the other tests do with things like symlinks and file modes.  And if get_loadavg() is updated to use BSD::loadavg, the test should still be skipped if the module isn't installed.

Furthermore, tests should always be updated when a feature is changed.

> Let me explain how it currently works without /proc/loadavg.  

I did check the code.

> Third, the test (as you can see below in context line in quoted diff
> below) forces gitweb to go over maximum load by setting $maxload to 0.
> This means that regardless of true load, and regardless whether gitweb
> can detect system load (remember that if it cant get system load it
> returns 0 instead) gitweb would be in "load too high" situation.

I did check the code.  Skipping the test seemed more in line with other tests.

sub get_loadavg {
    if( -e '/proc/loadavg' ){}
    return 0;
}

if (defined $maxload && get_loadavg() > $maxload) {
}

Setting $maxload to 0 does _not_ trigger failure because zero is not greater than 0.  Setting $maxload to -1 might work though.  I'll try it and test it in a little bit.  While I disagree that it's a good way to handle the situation, I will see if it works.

~~ Brian

Re: [PATCH] t9501: Skip testing load if we can't detect it

[Jakub Narebski]

Brian Gernhardt wrote:
> On Feb 6, 2010, at 6:22 AM, Jakub Narebski wrote:
>> Brian Gernhardt <brian@gernhardtsoftware.com> writes:
>> 
>>> Currently gitweb only knows how to check for load using /proc/loadavg,
>>> which isn't available on all systems.  We shouldn't fail the test just
>>> because we don't know how to check the system load.
>>> 
>>> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
>> 
>> NAK.  It is not necessary, and it would be hindrance (one more place
>> to update) if we are to extend get_loadavg() in gitweb to work without
>> /proc/loadavg, e.g. via BSD::loadavg module.
> 
> Without this patch, the test fails on my OS X machine (which doesn't
> have /proc).  So _something_ is necessary.

Right.

> Skipping the test because we can't use the feature on the host machine
> seemed more in line with what the other tests do with things like
> symlinks and file modes.

This is more test that gitweb returns correct HTTP response in 'load
too high' situation, than testing of it finding average system load.

> And if get_loadavg() is updated to use BSD::loadavg, the test should
> still be skipped if the module isn't installed.      

This test should succeed both when get_loadavg() returns true system
load and when it can't read load average and it returns 0.

>> Third, the test (as you can see below in context line in quoted diff
>> below) forces gitweb to go over maximum load by setting $maxload to 0.
>> This means that regardless of true load, and regardless whether gitweb
>> can detect system load (remember that if it cant get system load it
>> returns 0 instead) gitweb would be in "load too high" situation.
> 
> I did check the code.  Skipping the test seemed more in line with other tests.
> 
> sub get_loadavg {
>     if( -e '/proc/loadavg' ){}
>     return 0;
> }
> 
> if (defined $maxload && get_loadavg() > $maxload) {
> }
> 
> Setting $maxload to 0 does _not_ trigger failure because zero is not
> greater than 0.  Setting $maxload to -1 might work though.  I'll try
> it and test it in a little bit.  While I disagree that it's a good
> way to handle the situation, I will see if it works.   

Oops.


Either gitweb should be modified to read

  if (defined $maxload && get_loadavg() >= $maxload) {

or the test should use 

  $maxload = -1;

to artificially force 'load too high' situation.

-- 
Jakub Narebski
Poland

[PATCH] t9501: Correctly force over max load everywhere

[Brian Gernhardt]

The code to check for load returns 0 if it doesn't know how to find
load.  It also checks to see if the current load is higher than the
max load.  So to force the script to quit early by setting the maxload
variable negative which should work for systems where we can detect
load (which should be a positive number) and systems where we can't
(where detected load is 0)

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 Tested and works.

 t/t9501-gitweb-standalone-http-status.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 7590f10..d196cc5 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -117,7 +117,7 @@ test_debug 'cat gitweb.output'
 
 # always hit the load limit
 cat >>gitweb_config.perl <<\EOF
-our $maxload = 0;
+our $maxload = -1;
 EOF
 
 test_expect_success 'load checking: load too high (default action)' '
-- 
1.7.0.rc1.205.g52ece

Re: [PATCH] t9501: Skip testing load if we can't detect it

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> or the test should use 
>
>   $maxload = -1;
>
> to artificially force 'load too high' situation.

One related thing that bugs me somewhat is that there is no corresponding
test to make sure that "too loaded" does not trigger when it shouldn't.
But it is harder to arrange (there is no safe $maxload value you can
hardcode in the test and guarantee the system will not loaded that high),
so you need to teach the get_loadavg() to lie under test only for the sake
of running the test, which is backwards---so I'd say that is not worth it.

Here is my attempt to reword the commit log message from Brian (the "-1"
one, squashed into a revert of the one from yesterday, the latter of which
I already pushed out).  I just added the first paragraph to better justify
the reason why we are testing the codepath that would never be exercised
in real life on platforms that lack /proc/loadavg.

    Subject: t9501: Re-fix max load test

    Revert the previous attempt to skip this test on platforms where we
    currently cannot determine the system load.  We want to make sure that
    the max-load-limit codepath produces results cleanly, when gitweb is
    updated and becomes capable of reading the load average by some other
    method.

    The code to check for load returns 0 if it doesn't know how to find
    load.  It also checks to see if the current load is higher than the
    max load.  So to force the script to quit early by setting the maxload
    variable negative which should work for systems where we can detect
    load (which should be a positive number) and systems where we can't
    (where detected load is 0)

    Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>

Re: [PATCH] t9501: Correctly force over max load everywhere

[Jakub Narebski]

Dnia sobota 6. lutego 2010 15:50, Brian Gernhardt napisał:

> The code to check for load returns 0 if it doesn't know how to find
> load.  It also checks to see if the current load is higher than the
> max load.  So to force the script to quit early by setting the maxload
> variable negative which should work for systems where we can detect
> load (which should be a positive number) and systems where we can't
> (where detected load is 0)
> 
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>

Acked-by: Jakub Narebski <jnareb@gmail.com>

-- 
Jakub Narebski
Poland

Re: [PATCH] t9501: Skip testing load if we can't detect it

[Brian Gernhardt]

On Feb 6, 2010, at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Here is my attempt to reword the commit log message from Brian (the  
> "-1"
> one, squashed into a revert of the one from yesterday, the latter of  
> which
> I already pushed out).  I just added the first paragraph to better  
> justify
> the reason why we are testing the codepath that would never be  
> exercised
> in real life on platforms that lack /proc/loadavg.

Looks good to me.

>    Subject: t9501: Re-fix max load test
>
>    Revert the previous attempt to skip this test on platforms where we
>    currently cannot determine the system load.  We want to make sure  
> that
>    the max-load-limit codepath produces results cleanly, when gitweb  
> is
>    updated and becomes capable of reading the load average by some  
> other
>    method.
>
>    The code to check for load returns 0 if it doesn't know how to find
>    load.  It also checks to see if the current load is higher than the
>    max load.  So to force the script to quit early by setting the  
> maxload
>    variable negative which should work for systems where we can detect
>    load (which should be a positive number) and systems where we can't
>    (where detected load is 0)
>
>    Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>