Skip to content

lkl: timeout dhcp request ealier if not available#324

Merged
thehajime merged 1 commit intolkl:masterfrom
libos-nuse:fix-dhcp-test
Feb 14, 2017
Merged

lkl: timeout dhcp request ealier if not available#324
thehajime merged 1 commit intolkl:masterfrom
libos-nuse:fix-dhcp-test

Conversation

@thehajime
Copy link
Copy Markdown
Member

@thehajime thehajime commented Feb 14, 2017

Signed-off-by: Hajime Tazaki thehajime@gmail.com


This change is Reviewable

@thehajime
Copy link
Copy Markdown
Member Author

This should fix #285.
it sets hard-coded 5-second timeout which I'm not sure this is sufficient or not, but at least circleci looks like fine.

Cc: @liuyuan10

Comment thread tools/lkl/tests/net.sh Outdated
sudo ./net-test raw ${IFNAME} ${DST} dhcp
sudo ./net-test raw ${IFNAME} ${DST} dhcp &
sleep 5
sudo killall -q net-test && (echo "killed, skipped")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how it works. If killall doesn't find net-test, it still return 1 which is considered a failure by the script?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if kilall doesn't find, it return 0, keep quiet (-q) and goto next
if killall finds, return 1 and report via echo ..., but no error.

hmm, there is no chance to report an error with dhcp test now..
if there were a way to predict that dhcp works, this check can be expanded but dunno how.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use an env variable to specificy if DHCP test should run? Then we can set it in those environments where we know it runs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, that's a good idea.
maybe we can introduce LKL_TEST_DHCP for circleci environment, then timeout should not be needed.

will resubmit a patch.

@thehajime thehajime force-pushed the fix-dhcp-test branch 2 times, most recently from 72c9393 to ac747c3 Compare February 14, 2017 10:10
By default, make test won't test with dhcp client.  If the environmental
variable LKL_TEST_DHCP is configured, it will be tested.

Signed-off-by: Hajime Tazaki <thehajime@gmail.com>
@thehajime thehajime merged commit 26be13b into lkl:master Feb 14, 2017
@thehajime thehajime deleted the fix-dhcp-test branch February 14, 2017 11:01
@liuyuan10
Copy link
Copy Markdown
Member

Just curious, killall -q xxx || echo $? prints 1 instead of 0 on my desktop. So killall -q xxx && echo "hello" won't print but is an error. What am I missing here?

@thehajime
Copy link
Copy Markdown
Member Author

All your results make sense to me.

Just curious, killall -q xxx || echo $?

This is an OR condition. The 2nd statement is not evaluated if the 1st one is true.

killall -q xxx && echo "hello"

This is an AND condition. The 2nd statement is not evaluated if the 1st one is false.

in those case, (return value = 0) is true while (return value !=0) is false (this is a bit complicated part to me).

  • killall(1)
killall returns a zero return code if at least one process has been killed for each listed command, or no commands were listed and at least one process matched the -u and -Z search criteria.  killall returns non-zero otherwise.

@liuyuan10
Copy link
Copy Markdown
Member

liuyuan10 commented Feb 15, 2017 via email

@thehajime
Copy link
Copy Markdown
Member Author

@liuyuan10 your curious is right.

in short, there was an error.

my intention of the original patch (now diminished) was

  • ignore net-test result with text report if there is not DHCP server (the test should be PASS)
  • pass if DHCP server is available (the test should be PASS)

so I didn't intended to report an error after this test.

OTOH, the circleci test passed my patch with the above 2nd case with the Exit code 0 (no error) so I was thinking that the above cases were working fine.
https://circleci.com/gh/libos-nuse/lkl-linux/501

the statement sudo killall -q net-test && (echo "killed, skipped") in above 501 test surely returns 1 (error), but the script (net.sh) didn't exit and keep going to next steps.

it looks like there was something odd.

if I changed my patch like below, it will report an error.

diff --git a/tools/lkl/tests/net.sh b/tools/lkl/tests/net.sh
index 896f9c0..bc7497d 100755
--- a/tools/lkl/tests/net.sh
+++ b/tools/lkl/tests/net.sh
@@ -45,7 +45,7 @@ if ! [ -z $DST ]; then
     sudo ip link set dev ${IFNAME} promisc on
     sudo ./net-test raw ${IFNAME} ${DST} dhcp &
     sleep 5
-    sudo killall -q net-test && (echo "killed, skipped")
+    (sudo killall -q net-test && (echo "killed, skipped"))
     sudo ip link set dev ${IFNAME} promisc off
 
     echo "== macvtap (LKL net) tests =="

so, again, you caught a good point.

@thehajime thehajime mentioned this pull request Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants