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

Add GCE PD tests for windows cluster #74990

Merged
merged 1 commit into from
Mar 8, 2019
Merged

Add GCE PD tests for windows cluster #74990

merged 1 commit into from
Mar 8, 2019

Conversation

jingxu97
Copy link
Contributor

@jingxu97 jingxu97 commented Mar 5, 2019 •
edited
Loading

This PR is the first one to add a few GCE PD tests for windows cluster. Will
add more tests in later PRs

@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. 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. labels Mar 5, 2019
@k8s-ci-robot
Copy link
Contributor

@jingxu97: PR needs rebase.

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-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 5, 2019
@jingxu97 jingxu97 added sig/windows Categorizes an issue or PR as relevant to SIG Windows. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 5, 2019
@jingxu97 jingxu97 requested a review from yujuhong March 5, 2019 19:59
@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Mar 5, 2019
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 5, 2019
@jingxu97 jingxu97 requested a review from msau42 March 5, 2019 21:37
msau42
msau42 reviewed Mar 6, 2019
View reviewed changes
test/e2e/storage/drivers/in_tree.go Outdated
"ext3",
"ext4",
"xfs",
"ntfs",
Copy link
Member

Choose a reason for hiding this comment

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

we don't want this for linux?

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

Copy link
Member

Choose a reason for hiding this comment

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

ntfs is still here, we should remove it right?

test/e2e/storage/testsuites/provisioning.go Outdated
@@ -386,7 +387,7 @@ func (t StorageClassTest) TestDynamicProvisioning() *v1.PersistentVolume {
// This is a common test that can be called from a StorageClassTest.PvCheck.
func PVWriteReadSingleNodeCheck(client clientset.Interface, claim *v1.PersistentVolumeClaim, volume *v1.PersistentVolume, node NodeSelection) {
By(fmt.Sprintf("checking the created volume is writable and has the PV's mount options on node %+v", node))
command := "echo 'hello world' > /mnt/test/data"
command := "sleep 120; echo 'hello world' > /mnt/test/data"
Copy link
Member

Choose a reason for hiding this comment

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

is this for debugging?

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/provisioning.go Outdated
@@ -435,7 +436,7 @@ func PVMultiNodeCheck(client clientset.Interface, claim *v1.PersistentVolumeClai
}()

By(fmt.Sprintf("checking the created volume is writable and has the PV's mount options on node %+v", node))
command := "echo 'hello world' > /mnt/test/data"
command := "sleep 120; echo 'hello world' > /mnt/test/data"
Copy link
Member

Choose a reason for hiding this comment

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

for debugging?

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/framework/volume_util.go
var securityContext *v1.PodSecurityContext
var nodeSelector map[string]string
var image string
if !NodeOSDistroIs("windows") {
Copy link
Member

Choose a reason for hiding this comment

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

can we wrap some of these methods into util functions instead of having to replace them in every test case?

Like: getSecurityContext, getTestImage, generateWriteCmd, generateReadCommand, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For getSecurityContext, getTestImage etc, I think it works well to put into a util function. But for generate different commands it is hard because there are many different commands used in the test and hard to generalize into a few functions.

Copy link
Member

Choose a reason for hiding this comment

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

For the commands, is there a general pattern that the tests are doing?

Like if the test is trying to write a file, can we have a general writeCmd(filename, content) method?

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, I made a few utility functions to address this issue. Thanks!

test/e2e/framework/volume_util.go Outdated
"while(1) {cat /opt/0/index.html ; sleep 2 ; ls /opt/; sleep 2}",
}
fileCheckCommand = "powershell /c type"
nodeSelector = map[string]string{"beta.kubernetes.io/os": "windows"}
Copy link
Member

Choose a reason for hiding this comment

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

Can this be set in gce pd driver config instead of in the test case?

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

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/framework/volume_util.go Outdated
@@ -460,31 +480,40 @@ func TestVolumeClient(client clientset.Interface, config VolumeTestConfig, fsGro
}
clientPod, err := podsNamespacer.Create(clientPod)
if err != nil {
By(fmt.Sprint("failed to creat pod ", err))
Copy link
Member

Choose a reason for hiding this comment

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

Debugging messages should use Logf() instead of By()

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 pd_windows_test branch 3 times, most recently from bc119ba to dd50c89 Compare March 7, 2019 00:17
msau42
msau42 reviewed Mar 7, 2019
View reviewed changes
test/e2e/storage/drivers/in_tree.go
@@ -1134,6 +1144,9 @@ func (g *gcePdDriver) GetDriverInfo() *testsuites.DriverInfo {

func (g *gcePdDriver) SkipUnsupportedTest(pattern testpatterns.TestPattern) {
framework.SkipUnlessProviderIs("gce", "gke")
if pattern.FeatureTag == "sig-windows" {
Copy link
Member

Choose a reason for hiding this comment

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

I think you will need to add a check like this for the CSI GCE PD driver config here

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

@msau42
Copy link
Member

msau42 commented Mar 7, 2019

/approve
just some minor comments

@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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2019
msau42
msau42 reviewed Mar 7, 2019
View reviewed changes
test/e2e/storage/drivers/csi.go Outdated
@@ -461,6 +461,9 @@ func (g *gcePDExternalCSIDriver) SkipUnsupportedTest(pattern testpatterns.TestPa
if pattern.FsType == "xfs" {
framework.SkipUnlessNodeOSDistroIs("ubuntu", "custom")
}
if pattern.FeatureTag == "sig-windows" {
framework.SkipUnlessNodeOSDistroIs("windows")
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to skip entirely if it's windows since the CSI driver doesn't support it? Also, it's a little confusing, but there are two gcepd CSI drivers defined here, so this should go in both. (I eventually want to remove both pd csi driver definitions, tracked in #70258)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. fixed. Thanks!

@msau42
Copy link
Member

msau42 commented Mar 7, 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 Mar 7, 2019
@jingxu97
Add GCE PD tests for windows cluster
18be0a4 
This PR is the first one to add a few GCE PD tests for windows cluster. Will
add more tests in later PRs
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2019
@jingxu97 jingxu97 removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Mar 7, 2019
@msau42
Copy link
Member

msau42 commented Mar 7, 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 Mar 7, 2019
@jingxu97 jingxu97 added this to the v1.14 milestone Mar 7, 2019
@jingxu97 jingxu97 added the kind/bug Categorizes issue or PR as related to a bug. label Mar 7, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Mar 7, 2019
@jingxu97 jingxu97 added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 7, 2019
@k8s-ci-robot k8s-ci-robot merged commit 788f245 into kubernetes:master Mar 8, 2019
yujuhong
yujuhong reviewed Jul 16, 2019
View reviewed changes
test/e2e/storage/drivers/in_tree.go
}
if framework.NodeOSDistroIs("windows") {
config.ClientNodeSelector = map[string]string{
"beta.kubernetes.io/os": "windows",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also check kubernetes.io/os": "windows"
We've promoted the labels to GA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yujuhong yujuhong yujuhong left review comments

@msau42 msau42 msau42 left review comments

@madhusudancs madhusudancs Awaiting requested review from madhusudancs

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. 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
v1.14
Development

Successfully merging this pull request may close these issues.
4 participants
@jingxu97 @k8s-ci-robot @msau42 @yujuhong