Sets HostNetwork to False for tests which do not require it by claudiubelu · Pull Request #78731 · kubernetes/kubernetes

claudiubelu · 2019-06-05T12:12:33Z

What type of PR is this?

/kind failing-test

/sig testing

What this PR does / why we need it:

Some tests are setting HostNetwork=true, even if it is not required for them to pass.

This patch will set the HostNetwork to false for those tests, allowing them to be run on Windows nodes as well.

Running the tests affected by this PR on Linux nodes on different environments, yielded the following results:

azure-vnet-cni: https://paste.ubuntu.com/p/gYxsvWVqDc/
flannel l2bridge: https://paste.ubuntu.com/p/55YDFFHZzR/
flannel overlay: https://paste.ubuntu.com/p/mtrnP5JyxF/

Which issue(s) this PR fixes:

Fixes #69559
Original PR: #69525

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

MrHohn

Haven't looked deep into this, qq that why not reusing #69525 as I see valid comments there?

MrHohn · 2019-06-05T21:11:46Z

test/e2e/framework/util.go

I was reading the comments from previous thread (#69525 (comment)). Isn't that still true?

@BCLAU This function must be able to access kube-proxy running on the node, so running in non-hostNetwork doesn't work at all. You didn't see a problem because apparently RunHostCmd doesn't return an error when the curl command times out or gets a connection refused. So basically the test was then going on assuming it's not in user proxy mode. How is kube-proxy done in Windows, given there is no iptables? Does it run the user space proxy? If so then an "if windows, return userspace" might be the right solution for this function.

Alternately, the issue is that Windows doesn't support hostNetwork, and we need to talk to kube-proxy in the host network namespace in order to determine the proxy mode. So how do we get this info without hostNetwork? Does kube-proxy only make status available on localhost, or could we get it by passing status.hostIP via the downward API? If we can do that, then the pod can get this info using that IP instead of localhost.

Will remove this from the PR for the time being. Didn't notice that the test actually skips if it doesn't have 2 Ready nodes, so I've missed this.

Multiple nodes are required now.

No conformance tests should have Skips in them. If they do, they cannot be promoted.

claudiubelu · 2019-06-06T10:25:49Z

Haven't looked deep into this, qq that why not reusing #69525 as I see valid comments there?

True, but I cannot reopen that PR for some issue, I would have otherwise.

As for the comments, I should reply to them, but as you can see in this PR's pastes, I've run those tests on Linux with different networking plugins (azure-vnet-cni, flannel overlay, flannel l2bridge) and they passed.

MrHohn · 2019-06-06T17:50:17Z

As for the comments, I should reply to them, but as you can see in this PR's pastes, I've run those tests on Linux with different networking plugins (azure-vnet-cni, flannel overlay, flannel l2bridge) and they passed.

It is important to ensure that the test not only passes, but also that it verifies what it should be. Specifically for that ProxyMode() func, it makes a HTTP call to kube-proxy's status port and parse out what proxy mode it is using. kube-proxy runs in hostnetwork, using a exec pod without hostnetwork that curls from localhost:10249 won't give us the correct info.

That ProxyMode() is currently used in test that won't work with userspace proxy mode. They explicitly check if proxy mode is userspace and skip the test. They didn't fail in your setup just because userspace proxy mode is not used.

timothysc

I'm going to defer to @kubernetes/sig-node-pr-reviews
/assign @johnbelamaric

yujuhong · 2019-07-01T21:50:17Z

/cc @lzang

k8s-ci-robot · 2019-07-01T21:50:20Z

@yujuhong: GitHub didn't allow me to request PR reviews from the following users: lzang.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @lzang

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

In general, YES to removing hostNetwork from these intra-pod and node-pod tests. With hostNetwork enabled they don't even really test what they were supposed to be testing. But we need to figure out what to do about ProxyMode which definitely doesn't work as written when run in hostNetwork: false

johnbelamaric · 2019-07-02T09:41:51Z

test/e2e/framework/util.go

@BCLAU This function must be able to access kube-proxy running on the node, so running in non-hostNetwork doesn't work at all. You didn't see a problem because apparently RunHostCmd doesn't return an error when the curl command times out or gets a connection refused. So basically the test was then going on assuming it's not in user proxy mode. How is kube-proxy done in Windows, given there is no iptables? Does it run the user space proxy? If so then an "if windows, return userspace" might be the right solution for this function.

Alternately, the issue is that Windows doesn't support hostNetwork, and we need to talk to kube-proxy in the host network namespace in order to determine the proxy mode. So how do we get this info without hostNetwork? Does kube-proxy only make status available on localhost, or could we get it by passing status.hostIP via the downward API? If we can do that, then the pod can get this info using that IP instead of localhost.

johnbelamaric · 2019-07-02T09:44:32Z

/priority important-soon

johnbelamaric · 2019-07-02T09:44:59Z

/sig network

test/e2e/common/networking.go

claudiubelu · 2019-10-31T15:00:44Z

/test pull-kubernetes-e2e-gce

kikisdeliveryservice · 2019-10-31T17:28:48Z

As an update Bug triage had the incorrect date. Freeze is Nov 14.

alejandrox1 · 2019-11-07T15:26:42Z

ping @johnbelamaric
could you take a look please?

johnbelamaric · 2019-11-07T18:31:42Z

I will block out some time on Friday.

johnbelamaric · 2019-11-08T23:23:52Z

Ok. I think I have figured out why this is so confusing. It comes down to that PR #71468 should not have been merged as it is written. It separates "host neworking" from the HostTestContainerPod. But that's the whole raison d'être of the HostTestContainerPod.

Instead, it should have just added a boolean that defined whether or not to create the HostTestContainerPod at all. If you look at the tests where you are passing in false, they use DialFromTestContainer or DialFromEndpointContainer, neither of which use the HostTestContainerPod.

So, I cannot approve this. In fact, we should go back and fix the others, I could approve something that doesn't create the HostTestContainerPod in all cases.

kikisdeliveryservice · 2019-11-10T20:57:51Z

@alejandrox1 @johnbelamaric : Should this be bumped to 1.18?

-Bug Triage Team

johnbelamaric · 2019-11-11T20:01:31Z

yes, probably that would make sense.

kikisdeliveryservice · 2019-11-12T17:15:06Z

/milestone v1.18

claudiubelu · 2019-11-18T19:19:09Z

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-gce

Some tests are setting HostNetwork=true, even if it is not required for them to pass. This patch will set the HostNetwork to false for those tests, allowing them to be run on Windows nodes as well.

johnbelamaric · 2019-11-22T23:35:39Z

Not sure why it says approved from me, going to make sure that's cancelled until I hear back on my most recent comments.

/approve cancel

claudiubelu · 2019-11-26T14:40:44Z

Not sure why it says approved from me, going to make sure that's cancelled until I hear back on my most recent comments.

/approve cancel

Yeah, sorry. I've addressed your comments, but haven't commented on it.

Ok. I think I have figured out why this is so confusing. It comes down to that PR #71468 should not have been merged as it is written. It separates "host neworking" from the HostTestContainerPod. But that's the whole raison d'être of the HostTestContainerPod.

This PR now removes test/e2e/windows/network.go

Instead, it should have just added a boolean that defined whether or not to create the HostTestContainerPod at all. If you look at the tests where you are passing in false, they use DialFromTestContainer or DialFromEndpointContainer, neither of which use the HostTestContainerPod.

Added. HostTestContainerPod is no longer created if host networking is not required. Although, technically, even DialFromTestContainer and DialFromEndpointContainer were exec-ing into HostTestContainerPod (even though it was unnecessary). Had to address that as well.

So, I cannot approve this. In fact, we should go back and fix the others, I could approve something that doesn't create the HostTestContainerPod in all cases.

Done.

claudiubelu · 2019-12-09T15:56:31Z

@johnbelamaric So, is it alright with you now?

johnbelamaric · 2019-12-09T20:23:00Z

Yes!
/lgtm
/approve

spiffxp · 2019-12-09T21:17:33Z

/approve

k8s-ci-robot · 2019-12-09T21:18:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: claudiubelu, johnbelamaric, spiffxp

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~test/e2e/common/OWNERS~~ [spiffxp]
~~test/e2e/framework/OWNERS~~ [spiffxp]
~~test/e2e/network/OWNERS~~ [johnbelamaric,spiffxp]
~~test/e2e/windows/OWNERS~~ [spiffxp]

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 requested review from MrHohn and krmayankk June 5, 2019 12:13

k8s-ci-robot added area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test labels Jun 5, 2019

MrHohn reviewed Jun 5, 2019

View reviewed changes

claudiubelu mentioned this pull request Jun 6, 2019

Sets HostNetwork to False for tests which do not require it #69525

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2019

claudiubelu force-pushed the tests-hostnetwork branch from 94ae4a3 to 90b41e5 Compare June 19, 2019 13:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2019

k8s-ci-robot assigned johnbelamaric Jul 1, 2019

timothysc reviewed Jul 1, 2019

View reviewed changes

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jul 1, 2019

johnbelamaric reviewed Jul 2, 2019

View reviewed changes

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 2, 2019

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Jul 2, 2019

johnbelamaric reviewed Jul 2, 2019

View reviewed changes

test/e2e/common/networking.go Outdated Show resolved Hide resolved

claudiubelu force-pushed the tests-hostnetwork branch from 90b41e5 to b9c4858 Compare July 9, 2019 13:48

k8s-ci-robot modified the milestones: v1.17, v1.18 Nov 12, 2019

claudiubelu force-pushed the tests-hostnetwork branch from c1d9a6d to 6b00ba4 Compare November 18, 2019 16:23

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/windows Categorizes an issue or PR as relevant to SIG Windows. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 18, 2019

claudiubelu force-pushed the tests-hostnetwork branch 2 times, most recently from 236ade1 to 41760d2 Compare November 18, 2019 17:39

claudiubelu force-pushed the tests-hostnetwork branch from 41760d2 to 6ed08f1 Compare November 19, 2019 15:16

Sets HostNetwork to False for tests which do not require it

e465e10

Some tests are setting HostNetwork=true, even if it is not required for them to pass. This patch will set the HostNetwork to false for those tests, allowing them to be run on Windows nodes as well.

claudiubelu force-pushed the tests-hostnetwork branch from 6ed08f1 to e465e10 Compare November 26, 2019 14:42

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2019

k8s-ci-robot merged commit 3c4d2a5 into kubernetes:master Dec 9, 2019

claudiubelu mentioned this pull request Apr 23, 2024

[LinuxOnly] is egregiously misused at least in sig-network tests #124426

Open

Conversation

claudiubelu commented Jun 5, 2019

Uh oh!

MrHohn left a comment

Choose a reason for hiding this comment

Uh oh!

MrHohn Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

johnbelamaric Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

claudiubelu Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

bgrant0607 Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

claudiubelu commented Jun 6, 2019

Uh oh!

MrHohn commented Jun 6, 2019

Uh oh!

timothysc left a comment

Choose a reason for hiding this comment

Uh oh!

yujuhong commented Jul 1, 2019

Uh oh!

k8s-ci-robot commented Jul 1, 2019

Uh oh!

johnbelamaric left a comment

Choose a reason for hiding this comment

Uh oh!

johnbelamaric Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

johnbelamaric commented Jul 2, 2019

Uh oh!

johnbelamaric commented Jul 2, 2019

Uh oh!

Uh oh!

claudiubelu commented Oct 31, 2019

Uh oh!

kikisdeliveryservice commented Oct 31, 2019

Uh oh!

alejandrox1 commented Nov 7, 2019

Uh oh!

johnbelamaric commented Nov 7, 2019

Uh oh!

johnbelamaric commented Nov 8, 2019

Uh oh!

kikisdeliveryservice commented Nov 10, 2019

Uh oh!

johnbelamaric commented Nov 11, 2019

Uh oh!

kikisdeliveryservice commented Nov 12, 2019

Uh oh!

claudiubelu commented Nov 18, 2019

Uh oh!

johnbelamaric commented Nov 22, 2019

Uh oh!

claudiubelu commented Nov 26, 2019

Uh oh!

claudiubelu commented Dec 9, 2019

Uh oh!

johnbelamaric commented Dec 9, 2019

Uh oh!

spiffxp commented Dec 9, 2019

Uh oh!

k8s-ci-robot commented Dec 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants