Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use TCP instead of ICMP to check outbound connectivity #77794

Merged
merged 2 commits into from
Aug 21, 2019
Merged

Use TCP instead of ICMP to check outbound connectivity #77794

merged 2 commits into from
Aug 21, 2019

Conversation

saiyan86
Copy link
Contributor

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation

/kind failing-test

/kind feature
/kind flake

What this PR does / why we need it:
This PR updates the networking e2e test on Azure to use nc instead of ping for IPv4 tests. This is because Azure's LB drops ICMP packets for security reasons.

Which issue(s) this PR fixes:

Fixes #75172

Special notes for your reviewer:

This PR only includes fix for IPv4 test, because the current busybox image doesn't have nc support for IPv6. Will prepare an image with nc support for IPv6 and update the rest tests in the future.

Does this PR introduce a user-facing change?:

None

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 13, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @saiyan86. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 13, 2019
@saiyan86
Copy link
Contributor Author

/assign feiskyer

@feiskyer
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 13, 2019
feiskyer
feiskyer reviewed May 13, 2019
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

Since ping is not supported for Azure LB ips, the change is required for running those tests on Azure

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2019
@feiskyer
Copy link
Member

/retest

@saiyan86
Copy link
Contributor Author

saiyan86 commented May 16, 2019 •
edited

/sig network
/sig testing

@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label May 16, 2019
@saiyan86
Copy link
Contributor Author

/assign thockin

@saiyan86
Copy link
Contributor Author

@bowei Could you please take a look? You wrote the original tests 😄

@saiyan86
Copy link
Contributor Author

/assign bowei

@k8s-ci-robot k8s-ci-robot removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Aug 16, 2019
@k8s-ci-robot
Copy link
Contributor

@johnbelamaric: Those labels are not set on the issue: sig/

In response to this:

/remove-area apiserver cloudprovider code-generation dependency ipvs kubeadm kubectl kubelet release-eng
/remove-kind api-change
/remove-sig api-machinery apps cli cloud-provider cluster-lifecycle instrumentation node release scheduling storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@johnbelamaric
Copy link
Member

/retest

@johnbelamaric
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2019
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 16, 2019 •
edited

@saiyan86: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-publishing-bot-validate 7407497dca0344e29a9a10d6ee123de3b4aa420d link /test pull-publishing-bot-validate
pull-kubernetes-e2e-gce-iscsi-serial 7407497dca0344e29a9a10d6ee123de3b4aa420d link /test pull-kubernetes-e2e-gce-iscsi-serial
pull-kubernetes-cross 7407497dca0344e29a9a10d6ee123de3b4aa420d link /test pull-kubernetes-cross

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@saiyan86
Copy link
Contributor Author

/retest

@johnbelamaric
Copy link
Member

/approve
for networking but someone else needs to do framework

@saiyan86
Copy link
Contributor Author

@andrewsykim Could you please take a look? Thanks!

oomichi
oomichi reviewed Aug 20, 2019
View reviewed changes
Copy link
Member

@oomichi oomichi left a comment

Choose a reason for hiding this comment

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

Most part is good for me, just few questions.

test/e2e/framework/util.go Show resolved Hide resolved
test/e2e/framework/util.go
"-c", "3", // send 3 pings
"-W", "2", // wait at most 2 seconds for a reply
"nc",
"-vz",
Copy link
Member

Choose a reason for hiding this comment

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

-u option is not specified, so TCP is always used instead of ICMP and UDP, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. By default is TCP.

Copy link
Member

Choose a reason for hiding this comment

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

Then the PR title should be Use TCP instead of ICMP to check outbound connectivity without UDP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I have changed the PR title. Will submit another PR is UDP is needed later.

@oomichi
Copy link
Member

oomichi commented Aug 20, 2019

/cc @oomichi

@saiyan86 saiyan86 changed the title Use TCP/UDP instead of ICMP to check outbound connectivity Use TCP instead of ICMP to check outbound connectivity Aug 20, 2019
@oomichi
Copy link
Member

oomichi commented Aug 20, 2019

Thanks for updating

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric, oomichi, saiyan86

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2019
@k8s-ci-robot k8s-ci-robot merged commit 7a81ecd into kubernetes:master Aug 21, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 21, 2019
@saiyan86 saiyan86 deleted the fixAzureE2ETest branch August 21, 2019 20:29
@saiyan86 saiyan86 mentioned this pull request Aug 22, 2019
Closed
6 tasks
@aojea aojea mentioned this pull request Oct 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer feiskyer left review comments

@bowei bowei bowei left review comments

@thockin thockin thockin left review comments

@aojea aojea aojea left review comments

@andrewsykim andrewsykim andrewsykim left review comments

@johnbelamaric johnbelamaric johnbelamaric requested changes

@nicksardo nicksardo Awaiting requested review from nicksardo

@pohly pohly Awaiting requested review from pohly

@oomichi oomichi Awaiting requested review from oomichi

Assignees

@feiskyer feiskyer

@bowei bowei

@johnbelamaric johnbelamaric

@andrewsykim andrewsykim

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.16
Development

Successfully merging this pull request may close these issues.

Network internet test for pod fails on azure
10 participants
@saiyan86 @k8s-ci-robot @feiskyer @thockin @andrewsykim @johnbelamaric @aojea @fejta-bot @oomichi @bowei