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

Update subpath e2e test for windows #77220

Merged
merged 1 commit into from
May 18, 2019
Merged

Update subpath e2e test for windows #77220

merged 1 commit into from
May 18, 2019

Conversation

jingxu97
Copy link
Contributor

Modify the current subpath e2e file to allow it run in windows clusters.

@k8s-ci-robot
Copy link
Contributor

@jingxu97: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. 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 Apr 29, 2019
@jingxu97
Copy link
Contributor Author

jingxu97 commented Apr 29, 2019 •
edited

cc @yujuhong @msau42

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 Apr 29, 2019
neolit123
neolit123 reviewed Apr 29, 2019
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/release-note-none
/kind cleanup
/priority important-longterm
/sig windows

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/windows Categorizes an issue or PR as relevant to SIG Windows. release-note-none Denotes a PR that doesn't merit a release note. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 29, 2019
@yujuhong
Copy link
Contributor

yujuhong commented May 6, 2019

@jingxu97 rebase? :-)

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2019
@jingxu97
Copy link
Contributor Author

@yujuhong @msau42 the tests are passing. PTAL

davidz627
davidz627 reviewed May 14, 2019
View reviewed changes
Copy link
Contributor

@davidz627 davidz627 left a comment

Choose a reason for hiding this comment

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

just some nits :)

test/e2e/storage/testsuites/subpath.go Outdated
@@ -901,5 +905,9 @@ func formatVolume(f *framework.Framework, pod *v1.Pod) {
}

func podContainerExec(pod *v1.Pod, containerIndex int, bashExec string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: variable should be scriptCmd or something instead of bashExec now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

test/e2e/storage/testsuites/subpath.go Outdated
if framework.NodeOSDistroIs("windows") {
return framework.RunKubectl("exec", fmt.Sprintf("--namespace=%s", pod.Namespace), pod.Name, "--container", pod.Spec.Containers[containerIndex].Name, "--", "powershell", "/c", bashExec)
} else {
return framework.RunKubectl("exec", fmt.Sprintf("--namespace=%s", pod.Namespace), pod.Name, "--container", pod.Spec.Containers[containerIndex].Name, "--", "/bin/sh", "-c", bashExec)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use a variable for powershell bash toggle, the rest of the command seems the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

test/e2e/storage/testsuites/subpath.go
@@ -421,7 +434,7 @@ func (s *subPathTestSuite) defineTests(driver TestDriver, pattern testpatterns.T
gomega.Expect(err).ToNot(gomega.HaveOccurred(), "while waiting for pod to be running")

// Exec into container that mounted the volume, delete subpath directory
rmCmd := fmt.Sprintf("rm -rf %s", l.subPathDir)
rmCmd := fmt.Sprintf("rm -r %s", l.subPathDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we remove -f? I think this swill now fail if subpath dir is for some reason non-existent. If that is desired behavior please add comment so future ppl dont un-fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it because windows does not has -f option. From the test workflow, subPathDir should not be empty. Do we need -f option in this case?

test/e2e/storage/testsuites/subpath.go Outdated
setInitCommand(l.pod, fmt.Sprintf("ln -s ../ %s", l.subPathDir))

if framework.NodeOSDistroIs("windows") {
setInitCommand(l.pod, fmt.Sprintf("New-Item -ItemType SymbolicLink -Path %s -value ..\\", l.subPathDir))
Copy link
Contributor

Choose a reason for hiding this comment

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

general nits: use a variable for the part that changes, avoid copying over function call and rest of arguments

msau42
msau42 reviewed May 14, 2019
View reviewed changes
test/e2e/storage/testsuites/subpath.go Outdated
@@ -282,17 +291,21 @@ func (s *subPathTestSuite) defineTests(driver TestDriver, pattern testpatterns.T
testMultipleReads(f, l.pod, 0, filepath1, filepath2)
})

ginkgo.It("should support restarting containers using directory as subpath [Slow]", func() {
ginkgo.It("should support restarting containers using directory as subpath [Slow][LinuxOnly]", func() {
Copy link
Member

Choose a reason for hiding this comment

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

this test is linux only, but we still have windows specific logic inside it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@jingxu97 jingxu97 force-pushed the March/windows_test branch 2 times, most recently from 02b45a9 to 4d93956 Compare May 16, 2019 21:15
@msau42
Copy link
Member

msau42 commented May 16, 2019

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, msau42

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

@davidz627
Copy link
Contributor

/retest
test infra seems like its slower than usual today

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

4 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@jingxu97
Update subpath e2e test for windows
e570d27 
Modify the current subpath e2e file to allow it run in windows clusters.

Change-Id: I921dfbbae9480c718853a97a76cc0a95b1af9790
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2019
@jingxu97
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-100-performance

@msau42
Copy link
Member

msau42 commented May 17, 2019

/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 17, 2019
@k8s-ci-robot k8s-ci-robot merged commit 21448ea into kubernetes:master May 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@davidz627 davidz627 davidz627 left review comments

@msau42 msau42 msau42 left review comments

@jeffvance jeffvance Awaiting requested review from jeffvance

Assignees

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jingxu97 @k8s-ci-robot @yujuhong @msau42 @davidz627 @fejta-bot @neolit123