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

claudiubelu · 2018-10-08T15:08:43Z

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.

/sig testing

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #69559

Special notes for your reviewer:

Release note:

NONE

neolit123 · 2018-10-09T03:25:20Z

SGTM

/ok-to-test
/kind cleanup
/release-note-none

claudiubelu · 2018-10-09T07:56:14Z

/test pull-kubernetes-e2e-gce

PatrickLang · 2018-10-09T16:01:28Z

/milestone 1.13
We need this to fix tests running on Windows so we can enable the periodic test runs

k8s-ci-robot · 2018-10-09T16:01:28Z

@PatrickLang: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

Details

In response to this:

/milestone 1.13
We need this to fix tests running on Windows so we can enable the periodic test runs

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.

feiskyer

/lgtm

feiskyer · 2018-10-10T09:11:35Z

/milestone 1.13
/retest

k8s-ci-robot · 2018-10-10T09:11:36Z

@feiskyer: The provided milestone is not valid for this repository. Milestones in this repository: [next-candidate, v1.10, v1.11, v1.12, v1.13, v1.4, v1.5, v1.6, v1.7, v1.8, v1.9]

Use /milestone clear to clear the milestone.

Details

In response to this:

/milestone 1.13
/retest

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.

feiskyer · 2018-10-10T09:11:48Z

/milestone v1.13

test/e2e/framework/util.go

caseydavenport · 2018-10-10T17:02:44Z

test/e2e/framework/util.go

Will this function work if not in hostNetwork?

It seems to me that it is relying on hitting the host at a given port via localhost, which won't resolve the same when run in hostNetwork vs pod networked.

Apparently so. The only place this is used in, is in the test should preserve source pod IP for traffic thru service cluster IP, which is even marked as passed in the pull-kubernetes-e2e-kops-aws and the pull-kubernetes-e2e-gce jobs.

Something is fishy about that. This is exec'ing into a pod and curling a localhost URL of a sleep pod.

kubernetes/test/e2e/framework/util.go

Line 3431 in 8616687

func RunHostCmd(ns, name, cmd string) (string, error) {

kubernetes/test/e2e/network/networking.go

Line 87 in 7f23a74

It("should check kube-proxy urls", func() {

kubernetes/cmd/kube-proxy/app/server.go

Line 507 in ef19542

mux.HandleFunc("/proxyMode", func(w http.ResponseWriter, r *http.Request) {

And why isn't this using the same exec pod spec / image?

I can't speak to history of this implementation, but there's been a lot of evolution in network tests, so it would not surprise me that this didn't get updated at some point to a consistent pod spec.

This function can not possibly work without hostNetwork.

The good news is that the calling test doesn't abort in this case, but just logs "the rest of this might fail". I guess you are NOT failing so this is a lot of noise about nothing.

But for this PR: you can't parameterize this function - it doesn't make sense.

Better fix: fix the calling test to skip this path entirely on windows, with a big comment about why

dims · 2018-10-30T17:58:51Z

/lgtm

Thanks @BCLAU

timothysc

I like it!

/lgtm

timothysc · 2018-10-30T19:13:27Z

/approve

k8s-ci-robot · 2018-10-30T19:13:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bclau, timothysc

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/OWNERS~~ [timothysc]

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

bgrant0607 · 2018-10-30T19:31:34Z

@thockin @caseydavenport @bowei: This needs a thorough review by someone from SIG Network.

Which tests are intending to test service reachability from a pod perspective, and which ones, if any, are deliberately testing host reachability?

bgrant0607 · 2018-10-30T19:32:38Z

I removed lgtm and approval until we hear from SIG Network.

timothysc · 2018-11-01T18:36:48Z

/assign @thockin

thockin

I think a lot of these cannot be changed as you propose, but I also think a lot of the kube-proxy details should not be conformance tests.

thockin · 2018-11-01T22:46:26Z

test/e2e/framework/util.go

I can't speak to history of this implementation, but there's been a lot of evolution in network tests, so it would not surprise me that this didn't get updated at some point to a consistent pod spec.

This function can not possibly work without hostNetwork.

The good news is that the calling test doesn't abort in this case, but just logs "the rest of this might fail". I guess you are NOT failing so this is a lot of noise about nothing.

But for this PR: you can't parameterize this function - it doesn't make sense.

Better fix: fix the calling test to skip this path entirely on windows, with a big comment about why

thockin · 2018-11-01T22:58:49Z

test/e2e/network/networking.go


 		It("should function for node-Service: http", func() {
-			config := framework.NewNetworkingTestConfig(f)
+			config := framework.NewNetworkingTestConfig(f, false)


I don't see how this can pass - it is specifically about testing from the node...

thockin · 2018-11-01T22:59:01Z

test/e2e/network/networking.go


 		It("should function for node-Service: udp", func() {
-			config := framework.NewNetworkingTestConfig(f)
+			config := framework.NewNetworkingTestConfig(f, false)


thockin · 2018-11-01T23:00:02Z

test/e2e/network/networking.go

 		// Slow because we confirm that the nodePort doesn't serve traffic, which requires a period of polling.
 		It("should update nodePort: http [Slow]", func() {
-			config := framework.NewNetworkingTestConfig(f)
+			config := framework.NewNetworkingTestConfig(f, false)


This too - DialFromNode should not work without HostNetwork

thockin · 2018-11-01T23:01:09Z

test/e2e/network/service.go

 		Expect(err).NotTo(HaveOccurred())

-		hostExec := framework.LaunchHostExecPod(f.ClientSet, f.Namespace.Name, "hostexec")
+		hostExec := framework.LaunchHostExecPod(f.ClientSet, f.Namespace.Name, "hostexec", false)


How would this work without hostNet? It expects ss to be able to run and see kube-proxy

PatrickLang · 2018-11-14T00:55:43Z

This needs to move to v1.14 so it can be updated once we have a decision on Windows conformance profiles and how they will be implemented.

dims · 2018-11-14T01:08:15Z

ack @PatrickLang moving to 1.14

/milestone v1.14

fejta-bot · 2019-02-12T01:58:09Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

nikopen · 2019-03-01T17:54:57Z

@PatrickLang any chance this is merged in the following 2 weeks?
Code Freeze is in effect from next Friday.

claudiubelu · 2019-03-05T09:43:59Z

Closed in favor of: #71468

claudiubelu · 2019-06-06T10:26:57Z

For some reason, I cannot reopen this PR, but i've sent another PR for it. #78731

Additional information:

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/

k8s-ci-robot requested review from caseydavenport and erictune October 8, 2018 15:08

k8s-ci-robot added 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 Oct 8, 2018

k8s-ci-robot assigned feiskyer Oct 10, 2018

feiskyer reviewed Oct 10, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2018

k8s-ci-robot added this to the v1.13 milestone Oct 10, 2018

caseydavenport reviewed Oct 10, 2018

View reviewed changes

test/e2e/framework/util.go Outdated Show resolved Hide resolved

caseydavenport reviewed Oct 10, 2018

View reviewed changes

claudiubelu force-pushed the tests-hostnetwork branch from f745331 to b4293a3 Compare October 16, 2018 15:50

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018

k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Oct 30, 2018

k8s-ci-robot assigned dims Oct 30, 2018

timothysc approved these changes Oct 30, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2018

bgrant0607 removed approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 30, 2018

k8s-ci-robot assigned thockin Nov 1, 2018

thockin reviewed Nov 1, 2018

View reviewed changes

bgrant0607 mentioned this pull request Nov 8, 2018

Add new reviewers for conformance #70608

Merged

k8s-ci-robot modified the milestones: v1.13, v1.14 Nov 14, 2018

spiffxp mentioned this pull request Dec 5, 2018

Windows networking tests #71468

Merged

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

bgrant0607 mentioned this pull request Jan 30, 2019

Adds [LinuxOnly] tag to conformance tests that cannot be run on Windows #73204

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 12, 2019

spiffxp mentioned this pull request Mar 1, 2019

Deprecate conformance project board here favor of kubernetes/cncf-k8s-conformance-wg kubernetes-retired/architecture-tracking#55

Closed

claudiubelu closed this Mar 5, 2019

claudiubelu mentioned this pull request Jun 5, 2019

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

Merged

Conversation

claudiubelu commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neolit123 commented Oct 9, 2018

Uh oh!

claudiubelu commented Oct 9, 2018

Uh oh!

PatrickLang commented Oct 9, 2018

Uh oh!

k8s-ci-robot commented Oct 9, 2018

Uh oh!

feiskyer left a comment

Choose a reason for hiding this comment

Uh oh!

feiskyer commented Oct 10, 2018

Uh oh!

k8s-ci-robot commented Oct 10, 2018

Uh oh!

feiskyer commented Oct 10, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dims commented Oct 30, 2018

Uh oh!

timothysc left a comment

Choose a reason for hiding this comment

Uh oh!

timothysc commented Oct 30, 2018

Uh oh!

k8s-ci-robot commented Oct 30, 2018

Uh oh!

bgrant0607 commented Oct 30, 2018

Uh oh!

bgrant0607 commented Oct 30, 2018

Uh oh!

timothysc commented Nov 1, 2018

Uh oh!

thockin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PatrickLang commented Nov 14, 2018

Uh oh!

dims commented Nov 14, 2018

Uh oh!

fejta-bot commented Feb 12, 2019

Uh oh!

nikopen commented Mar 1, 2019

Uh oh!

claudiubelu commented Mar 5, 2019

Uh oh!

claudiubelu commented Jun 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

claudiubelu commented Oct 8, 2018 •

edited

Loading