Adds [LinuxOnly] tag to conformance tests that cannot be run on Windows by claudiubelu · Pull Request #73204 · kubernetes/kubernetes

claudiubelu · 2019-01-23T10:35:31Z

What type of PR is this?

/kind failing-test
/kind feature

/sig testing
/sig windows

What this PR does / why we need it:

Some of the tests cannot pass using Windows nodes due to various reasons:

seLinuxOptions are not supported on Windows.
Running as an UID / GID is not supported on Windows.
file permissions work differently on Windows, and they cannot be set in
the same manner as on Linux.
individual files cannot be mounted in Windows Containers.
Cannot create container using Linux image (e.g.: alpine) on Windows.

Because of this, it has been decided [1][2] to use the "[LinuxOnly]" tag for the
tests which cannot run on Windows because of the mentioned reasons. This way,
when running tests using Windows nodes, those tests can simply be skipped by
adding the "[LinuxOnly]" tag to the ginkgo.skip argument.

[1] https://groups.google.com/forum/#!topic/kubernetes-sig-testing/ii5584-Tkqk
[2] https://docs.google.com/document/d/1z8MQpr_jTwhmjLMUaqQyBk1EYG_Y_3D4y4YdMJ7V1Kk/edit#heading=h.ukbaidczvy3r

Which issue(s) this PR fixes:

Fixes #69871

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-01-23T10:35:38Z

Hi @BCLAU. 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.

Details

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.

If a test is known to be using Linux-specific features (e.g.: seLinuxOptions) or is unable to run on Windows nodes, it is labeled `[LinuxOnly]`. When using Windows nodes, this tag should be added to the `skip` argument. This tag was proposed during [1][2]. Depends-On: kubernetes/kubernetes#73204 [1] https://groups.google.com/forum/#!topic/kubernetes-sig-testing/ii5584-Tkqk [2] https://docs.google.com/document/d/1z8MQpr_jTwhmjLMUaqQyBk1EYG_Y_3D4y4YdMJ7V1Kk/edit#heading=h.ukbaidczvy3r

dims · 2019-01-23T12:15:00Z

/ok-to-test

bgrant0607 · 2019-01-24T06:26:37Z

test/e2e/common/networking.go

Why is this LinuxOnly?

These tests are spawning pods with HostNetwork enabled, which is unsuported on Windows, so they won't be able to start on Windows. Additionally, we're adding Windows-equivalent tests, which just spawns the pods without the HostNetwork set[ 1]. They are currently passing.

[1] #71468

@BCLAU let's add a note in the description. say This test is marked LinuxOnly since HostNetwork is not supported on other platforms like Windows?

Please see #69525

I'm concerned about changes to networking tests without review of SIG Network. We can mark this LinuxOnly for now, but I wouldn't assume that running without host networking does what the test intended

For the time being, that PR you've mentioned is shelved in favor of another PR: #71468

The PR I mentioned adds a commit [0] which adds the same networking tests in question but spawning the pods without HostNetworking and leaving the original tests' behaviour unchanged.

Ideally, we wouldn't have to have separate tests for the same thing, but until an agreement is reached, I think it should be acceptable.

And fwiw, those tests added in the mentioned PR are passing when using Windows Server 2019 with the 3 types of networking solutions I've tried recently: azure-vnet-cni, flannel l2bridge, flannel overlay.

[0] d0ff2f4

claudiubelu · 2019-01-24T12:04:22Z

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-integration
/test pull-kubernetes-kubemark-e2e-gce-big

adelina-t · 2019-01-25T17:02:53Z

@PatrickLang

test/e2e/common/kubelet_etc_hosts.go

bgrant0607 · 2019-01-25T23:41:48Z

test/e2e/common/runtime.go

Why is this LinuxOnly? The image says alpine, but doesn't actually exist

Same question for all the following test cases also

Good point. this passes.

bgrant0607 · 2019-01-25T23:44:18Z

Thank you for documenting the reasons these don't run on Windows

michmike · 2019-01-28T06:11:27Z

adding a hold. this PR looks good to me, but i would like to also see answers to some questions Brian raised
/hold

michmike · 2019-01-28T06:11:51Z

/hold

michmike · 2019-01-31T19:33:41Z

/hold cancel

claudiubelu · 2019-02-01T12:08:20Z

/assign @timothysc

Some of the tests cannot pass using Windows nodes due to various reasons: - seLinuxOptions are not supported on Windows. - Running as an UID / GID is not supported on Windows. - file permissions work differently on Windows, and they cannot be set in the same manner as on Linux. - individual files cannot be mounted in Windows Containers. - Cannot create container using Linux image (e.g.: alpine) on Windows. Because of this, it has been decided to use the "[LinuxOnly]" tag for the tests which cannot run on Windows because of the mentioned reasons. This way, when running tests using Windows nodes, those tests can simply be skipped by adding the "[LinuxOnly]" tag to the ginkgo.skip argument.

bgrant0607 · 2019-02-01T22:23:15Z

test/e2e/common/empty_dir.go

As came up in the KEP, Windows doesn't support medium = Memory, either

bgrant0607 · 2019-02-01T22:23:41Z

test/e2e/common/empty_dir.go

or medium = Memory

bgrant0607 · 2019-02-01T22:24:02Z

test/e2e/common/empty_dir.go

or medium = Memory

bgrant0607 · 2019-02-01T22:24:09Z

test/e2e/common/empty_dir.go

or medium = Memory

bgrant0607 · 2019-02-01T22:24:15Z

test/e2e/common/empty_dir.go

or medium = Memory

bgrant0607 · 2019-02-01T22:24:29Z

test/e2e/common/empty_dir.go

or medium = Memory

bgrant0607 · 2019-02-01T22:24:40Z

test/e2e/common/empty_dir.go

or medium = Memory

bgrant0607 · 2019-02-01T22:29:51Z

test/e2e/common/security_context.go

Please write this comment in a similar form as the others: This test is marked LinuxOnly since...

bgrant0607 · 2019-02-01T22:30:40Z

test/e2e/common/security_context.go

and does not support privilege escalation

bgrant0607 · 2019-02-01T22:32:48Z

@BCLAU I made another pass over the comments. I have a better understanding now after reviewing the KEP. The updates should be pretty simple. I'll approve after they are made.

Thanks for your work on this. It will others on the project who don't understand windows as well to avoid making incorrect changes to the tags and/or tests.

bgrant0607 · 2019-02-04T15:37:49Z

@BCLAU Thanks for the changes. In the future, please wait to squash commits until after lgtm, to make it possible for reviewers to distinguish the most recent changes from already-reviewed changes.

bgrant0607 · 2019-02-04T15:43:10Z

/lgtm
/approve

k8s-ci-robot · 2019-02-04T15:46:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bclau, bgrant0607, michmike

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~~ [bgrant0607]

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

Considering this PR got merged: kubernetes/kubernetes#73204 we can now skip tests that don't run on Linux by just using the LinuxOnly tag.

The test "should write entries to /etc/hosts" should have the [LinuxOnly] tag as it cannot pass on Windows; individual files cannot be mounted in Windows Containers. This test was missed in the original PR (kubernetes#73204)

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 23, 2019

k8s-ci-robot requested review from MrHohn and verult January 23, 2019 10:36

This was referenced Jan 23, 2019

Adds the [LinuxOnly] tag kubernetes/community#3140

Merged

Windows incompatible tests should be skippable #69871

Closed

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 Jan 23, 2019

bgrant0607 reviewed Jan 24, 2019

View reviewed changes

claudiubelu force-pushed the tests/adds-linuxonly-tag branch from 80db9e6 to 9e95a35 Compare January 24, 2019 15:17

bgrant0607 reviewed Jan 25, 2019

View reviewed changes

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

bgrant0607 reviewed Jan 25, 2019

View reviewed changes

This was referenced Jan 26, 2019

Starting test plan section of Windows KEP kubernetes/enhancements#685

Merged

Adding API section to Windows KEP kubernetes/enhancements#732

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2019

k8s-ci-robot assigned timothysc Feb 1, 2019

bgrant0607 reviewed Feb 1, 2019

View reviewed changes

claudiubelu force-pushed the tests/adds-linuxonly-tag branch from 76624ff to 5daa088 Compare February 4, 2019 12:24

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2019

k8s-ci-robot assigned bgrant0607 Feb 4, 2019

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

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

k8s-ci-robot merged commit 1a0a841 into kubernetes:master Feb 4, 2019

adelina-t mentioned this pull request Feb 5, 2019

Update sig windows job kubernetes/test-infra#11110

Merged

adelina-t pushed a commit to adelina-t/test-infra that referenced this pull request Feb 5, 2019

Change skip regex in sig-windows job

a1028ac

Considering this PR got merged: kubernetes/kubernetes#73204 we can now skip tests that don't run on Linux by just using the LinuxOnly tag.

claudiubelu mentioned this pull request Feb 11, 2019

tests: Adds [LinuxOnly] tag to conformance test #73922

Merged

claudiubelu mentioned this pull request Feb 20, 2019

REQUEST: New membership for bclau kubernetes/org#509

Closed

6 tasks

Conversation

claudiubelu commented Jan 23, 2019

Uh oh!

k8s-ci-robot commented Jan 23, 2019

Uh oh!

dims commented Jan 23, 2019

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!

claudiubelu Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claudiubelu commented Jan 24, 2019

Uh oh!

adelina-t commented Jan 25, 2019

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!

bgrant0607 commented Jan 25, 2019

Uh oh!

michmike commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michmike commented Jan 28, 2019

Uh oh!

michmike commented Jan 31, 2019

Uh oh!

claudiubelu commented Feb 1, 2019

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

bgrant0607 commented Feb 1, 2019

claudiubelu Jan 30, 2019 •

edited

Loading

michmike commented Jan 28, 2019 •

edited

Loading