Skip to content

test/helpers: allow passing custom number of requests to helpers.Ping()#11897

Merged
borkmann merged 1 commit intocilium:masterfrom
qmonnet:pr/qmonnet/ping_with_interval
Jun 5, 2020
Merged

test/helpers: allow passing custom number of requests to helpers.Ping()#11897
borkmann merged 1 commit intocilium:masterfrom
qmonnet:pr/qmonnet/ping_with_interval

Conversation

@qmonnet
Copy link
Copy Markdown
Member

@qmonnet qmonnet commented Jun 4, 2020 •
edited
Loading

Let's increase the interval between ICMP requests for the cases where the setup might not be ready to process them just yet. Introduce a new PingWithInterval() helper that takes an integer passed to ping's -i option.

[Edit: switched to interval between requests (-i) to number of requests (-c)]

Fixes: #11741 (hopefully)

@qmonnet qmonnet requested a review from a team as a code owner June 4, 2020 17:10
@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Jun 4, 2020

Please set the appropriate release note label.

@qmonnet qmonnet added area/CI Continuous Integration testing issue or flake ci/flake This is a known failure that occurs in the tree. Please investigate me! release-note/ci This PR makes changes to the CI. labels Jun 4, 2020
@qmonnet qmonnet requested review from aanm and jrajahalme June 4, 2020 17:12
@qmonnet
Copy link
Copy Markdown
Member Author

qmonnet commented Jun 4, 2020

test-me-please

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 4, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.02%) to 36.945% when pulling ebef9ee on qmonnet:pr/qmonnet/ping_with_interval into 6bd5b9c on cilium:master.

@qmonnet
Copy link
Copy Markdown
Member Author

qmonnet commented Jun 5, 2020

Ouch, the ping tool from BusyBox used for Runtime-4.9 does not support the -i option :(

     unable able to ping "10.0.2.15"
     Expected command: docker exec -i  app1 ping -i 1 -W 5 -c 5 10.0.2.15
     To succeed, but it failed:
     Exitcode: 1
     Stdout:

     Stderr:
      	 ping: unrecognized option: i
     	 BusyBox v1.24.2 (2017-01-19 08:02:06 GMT) multi-call binary.

     	 Usage: ping [OPTIONS] HOST

     	 Send ICMP ECHO_REQUEST packets to network hosts

     	 	-4,-6		Force IP or IPv6 name resolution
     	 	-c CNT		Send only CNT pings
     	 	-s SIZE		Send SIZE data bytes in packets (default:56)
     	 	-t TTL		Set TTL
     	 	-I IFACE/IP	Use interface or IP address as source
     	 	-W SEC		Seconds to wait for the first response (default:10)
     	 			(after all -c CNT packets are sent)
     	 	-w SEC		Seconds until ping exits (default:infinite)
     	 			(can exit earlier with -c CNT)
     	 	-q		Quiet, only display output at start
     	 			and when finished
     	 	-p		Pattern to use for payload

We could probably run ping -c 1 in a loop, with sleeps between the different attempts, but it doesn't look really clean. Another solution could be to rework the new helper to take a custom count for the requests (-c option, currently at 5) with an unchanged delay (i.e. 25 requests, one every second instead of 5 requests, one every 5 seconds). I'll update the PR in that direction.

@qmonnet qmonnet force-pushed the pr/qmonnet/ping_with_interval branch from dc05d55 to 10bec6a Compare June 5, 2020 09:30
@qmonnet qmonnet changed the title test/helpers: allow passing interval between requests to helpers.Ping() test/helpers: allow passing custom number of requests to helpers.Ping() Jun 5, 2020
@qmonnet
Copy link
Copy Markdown
Member Author

qmonnet commented Jun 5, 2020

test-me-please

@qmonnet
test/helpers: allow passing custom number of requests to helpers.Ping()
ebef9ee 
Let's offer to pass a custom number of ICMP requests for the cases where
the setup might not be ready to process them just yet. Introduce a new
PingWithCount() helper that takes an integer passed to ping's -c option.

The initial idea was to pass an interval through the "-i" option
instead, but the ping provided by BusyBox does not support it, leading
to failures on some environments.

Fixes: cilium#11741
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet force-pushed the pr/qmonnet/ping_with_interval branch from 10bec6a to ebef9ee Compare June 5, 2020 12:37
@qmonnet
Copy link
Copy Markdown
Member Author

qmonnet commented Jun 5, 2020

test-me-please

@borkmann borkmann merged commit e1e1f50 into cilium:master Jun 5, 2020
@qmonnet qmonnet deleted the pr/qmonnet/ping_with_interval branch June 5, 2020 20:29
@christarazi christarazi mentioned this pull request Jun 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nebril nebril nebril approved these changes

@pchaigno pchaigno pchaigno approved these changes

@aanm aanm aanm approved these changes

@jrajahalme jrajahalme Awaiting requested review from jrajahalme

Assignees

No one assigned

Labels

area/CI Continuous Integration testing issue or flake ci/flake This is a known failure that occurs in the tree. Please investigate me! release-note/ci This PR makes changes to the CI.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CI: Test Denies traffic with cnp default-deny ingress policy

7 participants

@qmonnet @coveralls @nebril @pchaigno @aanm @borkmann @christarazi