{"thread":{"id":"22543","subject":"[PATCH] t9501: Skip testing load if we can't detect it","startedAt":"2010-02-06T05:00:40Z","lastAt":"2010-02-06T20:09:39Z","messageCount":8,"participants":["Brian Gernhardt","Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"133779","messageId":"1265432440-52146-1-git-send-email-brian@gernhardtsoftware.com","threadId":"22543","inReplyTo":null,"subject":"[PATCH] t9501: Skip testing load if we can't detect it","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2010-02-06T05:00:40Z","receivedAt":"2010-02-06T05:00:40Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"Currently gitweb only knows how to check for load using /proc/loadavg,\nwhich isn't available on all systems. We shouldn't fail the test just\nbecause we don't know how to check the system load.\n\nSigned-off-by: Brian Gernhardt \n---\n t/t9501-gitweb-standalone-http-status.sh | 9 ++++++++-\n 1 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex 7590f10..992d729 100755\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -115,12 +115,19 @@ test_debug 'cat gitweb.output'\n # ----------------------------------------------------------------------\n # load checking\n \n+if test -e /proc/loadavg\n+then\n+\ttest_set_prereq PROC_LOADAVG\n+else\n+\tsay 'skipping load tests (no /proc/loadavg found)'\n+fi\n+\n # always hit the load limit\n cat >>gitweb_config.perl <<\\EOF\n our $maxload = 0;\n EOF\n \n-test_expect_success 'load checking: load too high (default action)' '\n+test_expect_success PROC_LOADAVG 'load checking: load too high (default action)' '\n \tgitweb_run \"p=.git\" &&\n \tgrep \"Status: 503 Service Unavailable\" gitweb.headers &&\n \tgrep \"503 - The load average on the server is too high\" gitweb.body\n-- \n1.7.0.rc1.141.gd3fd2\n"},{"id":"133790","messageId":"m3eikya9tn.fsf@localhost.localdomain","threadId":"22543","inReplyTo":"1265432440-52146-1-git-send-email-brian@gernhardtsoftware.com","subject":"Re: [PATCH] t9501: Skip testing load if we can't detect it","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-02-06T11:22:44Z","receivedAt":"2010-02-06T11:22:44Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Brian Gernhardt writes:\n\n> Currently gitweb only knows how to check for load using /proc/loadavg,\n> which isn't available on all systems. We shouldn't fail the test just\n> because we don't know how to check the system load.\n> \n> Signed-off-by: Brian Gernhardt \n\nNAK. It is not necessary, and it would be hindrance (one more place\nto update) if we are to extend get_loadavg() in gitweb to work without\n/proc/loadavg, e.g. via BSD::loadavg module.\n\n\nLet me explain how it currently works without /proc/loadavg. \n\nFirst, load average is by definition non-negative number (>= 0).\n\nSecond, the get_loadavg() subroutine is written to be resilent and\nrobust, and it returns 0 if it can't get load average from system\n(which _currently_ means no well-formatted /proc/loadavg file)\n\nThird, the test (as you can see below in context line in quoted diff\nbelow) forces gitweb to go over maximum load by setting $maxload to 0.\nThis means that regardless of true load, and regardless whether gitweb\ncan detect system load (remember that if it cant get system load it\nreturns 0 instead) gitweb would be in \"load too high\" situation.\n\n> ---\n> t/t9501-gitweb-standalone-http-status.sh | 9 ++++++++-\n> 1 files changed, 8 insertions(+), 1 deletions(-)\n> \n> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\n> index 7590f10..992d729 100755\n> --- a/t/t9501-gitweb-standalone-http-status.sh\n> +++ b/t/t9501-gitweb-standalone-http-status.sh\n> @@ -115,12 +115,19 @@ test_debug 'cat gitweb.output'\n> # ----------------------------------------------------------------------\n> # load checking\n> \n> +if test -e /proc/loadavg\n> +then\n> +\ttest_set_prereq PROC_LOADAVG\n> +else\n> +\tsay 'skipping load tests (no /proc/loadavg found)'\n> +fi\n> +\n> # always hit the load limit\n> cat >>gitweb_config.perl <<\\EOF\n> our $maxload = 0;\n> EOF\n> \n> -test_expect_success 'load checking: load too high (default action)' '\n> +test_expect_success PROC_LOADAVG 'load checking: load too high (default action)' '\n> \tgitweb_run \"p=.git\" &&\n> \tgrep \"Status: 503 Service Unavailable\" gitweb.headers &&\n> \tgrep \"503 - The load average on the server is too high\" gitweb.body\n> -- \n> 1.7.0.rc1.141.gd3fd2\n> \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"133800","messageId":"0CD6B283-3181-4FAB-A6B2-13AFF9E5071C@gernhardtsoftware.com","threadId":"22543","inReplyTo":"m3eikya9tn.fsf@localhost.localdomain","subject":"Re: [PATCH] t9501: Skip testing load if we can't detect it","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2010-02-06T13:46:00Z","receivedAt":"2010-02-06T13:46:00Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"\nOn Feb 6, 2010, at 6:22 AM, Jakub Narebski wrote:\n\n> Brian Gernhardt writes:\n> \n>> Currently gitweb only knows how to check for load using /proc/loadavg,\n>> which isn't available on all systems. We shouldn't fail the test just\n>> because we don't know how to check the system load.\n>> \n>> Signed-off-by: Brian Gernhardt \n> \n> NAK. It is not necessary, and it would be hindrance (one more place\n> to update) if we are to extend get_loadavg() in gitweb to work without\n> /proc/loadavg, e.g. via BSD::loadavg module.\n\nWithout 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.\n\nFurthermore, tests should always be updated when a feature is changed.\n\n> Let me explain how it currently works without /proc/loadavg. \n\nI did check the code.\n\n> Third, the test (as you can see below in context line in quoted diff\n> below) forces gitweb to go over maximum load by setting $maxload to 0.\n> This means that regardless of true load, and regardless whether gitweb\n> can detect system load (remember that if it cant get system load it\n> returns 0 instead) gitweb would be in \"load too high\" situation.\n\nI did check the code. Skipping the test seemed more in line with other tests.\n\nsub get_loadavg {\n if( -e '/proc/loadavg' ){}\n return 0;\n}\n\nif (defined $maxload && get_loadavg() > $maxload) {\n}\n\nSetting $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.\n\n~~ Brian"},{"id":"133802","messageId":"201002061505.13886.jnareb@gmail.com","threadId":"22543","inReplyTo":"0CD6B283-3181-4FAB-A6B2-13AFF9E5071C@gernhardtsoftware.com","subject":"Re: [PATCH] t9501: Skip testing load if we can't detect it","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-02-06T14:05:13Z","receivedAt":"2010-02-06T14:05:13Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Brian Gernhardt wrote:\n> On Feb 6, 2010, at 6:22 AM, Jakub Narebski wrote:\n>> Brian Gernhardt writes:\n>> \n>>> Currently gitweb only knows how to check for load using /proc/loadavg,\n>>> which isn't available on all systems. We shouldn't fail the test just\n>>> because we don't know how to check the system load.\n>>> \n>>> Signed-off-by: Brian Gernhardt \n>> \n>> NAK. It is not necessary, and it would be hindrance (one more place\n>> to update) if we are to extend get_loadavg() in gitweb to work without\n>> /proc/loadavg, e.g. via BSD::loadavg module.\n> \n> Without this patch, the test fails on my OS X machine (which doesn't\n> have /proc). So _something_ is necessary.\n\nRight.\n\n> Skipping the test because we can't use the feature on the host machine\n> seemed more in line with what the other tests do with things like\n> symlinks and file modes.\n\nThis is more test that gitweb returns correct HTTP response in 'load\ntoo high' situation, than testing of it finding average system load.\n\n> And if get_loadavg() is updated to use BSD::loadavg, the test should\n> still be skipped if the module isn't installed. \n\nThis test should succeed both when get_loadavg() returns true system\nload and when it can't read load average and it returns 0.\n\n>> Third, the test (as you can see below in context line in quoted diff\n>> below) forces gitweb to go over maximum load by setting $maxload to 0.\n>> This means that regardless of true load, and regardless whether gitweb\n>> can detect system load (remember that if it cant get system load it\n>> returns 0 instead) gitweb would be in \"load too high\" situation.\n> \n> I did check the code. Skipping the test seemed more in line with other tests.\n> \n> sub get_loadavg {\n> if( -e '/proc/loadavg' ){}\n> return 0;\n> }\n> \n> if (defined $maxload && get_loadavg() > $maxload) {\n> }\n> \n> Setting $maxload to 0 does _not_ trigger failure because zero is not\n> greater than 0. Setting $maxload to -1 might work though. I'll try\n> it and test it in a little bit. While I disagree that it's a good\n> way to handle the situation, I will see if it works. \n\nOops.\n\n\nEither gitweb should be modified to read\n\n if (defined $maxload && get_loadavg() >= $maxload) {\n\nor the test should use \n\n $maxload = -1;\n\nto artificially force 'load too high' situation.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"133805","messageId":"1265467803-31210-1-git-send-email-brian@gernhardtsoftware.com","threadId":"22543","inReplyTo":"201002061505.13886.jnareb@gmail.com","subject":"[PATCH] t9501: Correctly force over max load everywhere","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2010-02-06T14:50:03Z","receivedAt":"2010-02-06T14:50:03Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"The code to check for load returns 0 if it doesn't know how to find\nload. It also checks to see if the current load is higher than the\nmax load. So to force the script to quit early by setting the maxload\nvariable negative which should work for systems where we can detect\nload (which should be a positive number) and systems where we can't\n(where detected load is 0)\n\nSigned-off-by: Brian Gernhardt \n---\n\n Tested and works.\n\n t/t9501-gitweb-standalone-http-status.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex 7590f10..d196cc5 100755\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -117,7 +117,7 @@ test_debug 'cat gitweb.output'\n \n # always hit the load limit\n cat >>gitweb_config.perl <<\\EOF\n-our $maxload = 0;\n+our $maxload = -1;\n EOF\n \n test_expect_success 'load checking: load too high (default action)' '\n-- \n1.7.0.rc1.205.g52ece\n"},{"id":"133817","messageId":"7vwryq1aid.fsf@alter.siamese.dyndns.org","threadId":"22543","inReplyTo":"201002061505.13886.jnareb@gmail.com","subject":"Re: [PATCH] t9501: Skip testing load if we can't detect it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-06T18:31:38Z","receivedAt":"2010-02-06T18:31:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> or the test should use \n>\n> $maxload = -1;\n>\n> to artificially force 'load too high' situation.\n\nOne related thing that bugs me somewhat is that there is no corresponding\ntest to make sure that \"too loaded\" does not trigger when it shouldn't.\nBut it is harder to arrange (there is no safe $maxload value you can\nhardcode in the test and guarantee the system will not loaded that high),\nso you need to teach the get_loadavg() to lie under test only for the sake\nof running the test, which is backwards---so I'd say that is not worth it.\n\nHere is my attempt to reword the commit log message from Brian (the \"-1\"\none, squashed into a revert of the one from yesterday, the latter of which\nI already pushed out). I just added the first paragraph to better justify\nthe reason why we are testing the codepath that would never be exercised\nin real life on platforms that lack /proc/loadavg.\n\n Subject: t9501: Re-fix max load test\n\n Revert the previous attempt to skip this test on platforms where we\n currently cannot determine the system load. We want to make sure that\n the max-load-limit codepath produces results cleanly, when gitweb is\n updated and becomes capable of reading the load average by some other\n method.\n\n The code to check for load returns 0 if it doesn't know how to find\n load. It also checks to see if the current load is higher than the\n max load. So to force the script to quit early by setting the maxload\n variable negative which should work for systems where we can detect\n load (which should be a positive number) and systems where we can't\n (where detected load is 0)\n\n Signed-off-by: Brian Gernhardt \n"},{"id":"133821","messageId":"201002062045.50943.jnareb@gmail.com","threadId":"22543","inReplyTo":"1265467803-31210-1-git-send-email-brian@gernhardtsoftware.com","subject":"Re: [PATCH] t9501: Correctly force over max load everywhere","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-02-06T19:45:50Z","receivedAt":"2010-02-06T19:45:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia sobota 6. lutego 2010 15:50, Brian Gernhardt napisał:\n\n> The code to check for load returns 0 if it doesn't know how to find\n> load. It also checks to see if the current load is higher than the\n> max load. So to force the script to quit early by setting the maxload\n> variable negative which should work for systems where we can detect\n> load (which should be a positive number) and systems where we can't\n> (where detected load is 0)\n> \n> Signed-off-by: Brian Gernhardt \n\nAcked-by: Jakub Narebski \n\n-- \nJakub Narebski\nPoland\n"},{"id":"133823","messageId":"378C5218-C859-4E82-B39E-40BE5D7A572C@gernhardtsoftware.com","threadId":"22543","inReplyTo":"7vwryq1aid.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t9501: Skip testing load if we can't detect it","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2010-02-06T20:09:39Z","receivedAt":"2010-02-06T20:09:39Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"On Feb 6, 2010, at 1:31 PM, Junio C Hamano wrote:\n\n> Here is my attempt to reword the commit log message from Brian (the \n> \"-1\"\n> one, squashed into a revert of the one from yesterday, the latter of \n> which\n> I already pushed out). I just added the first paragraph to better \n> justify\n> the reason why we are testing the codepath that would never be \n> exercised\n> in real life on platforms that lack /proc/loadavg.\n\nLooks good to me.\n\n> Subject: t9501: Re-fix max load test\n>\n> Revert the previous attempt to skip this test on platforms where we\n> currently cannot determine the system load. We want to make sure \n> that\n> the max-load-limit codepath produces results cleanly, when gitweb \n> is\n> updated and becomes capable of reading the load average by some \n> other\n> method.\n>\n> The code to check for load returns 0 if it doesn't know how to find\n> load. It also checks to see if the current load is higher than the\n> max load. So to force the script to quit early by setting the \n> maxload\n> variable negative which should work for systems where we can detect\n> load (which should be a positive number) and systems where we can't\n> (where detected load is 0)\n>\n> Signed-off-by: Brian Gernhardt \n"}]}