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.

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

Skip GCE PD in-tree plugin tests if fstype is xfs and node distro is not ubuntu or custom #78459

Merged
merged 1 commit into from Jun 14, 2019

Conversation

davidz627
Copy link
Contributor

XFS Is not supported in other node OS distros. These tests are failing in multiple places

/kind failing-test
/sig storage
/priority important-soon

/assign @msau42 @mkimuram

NONE

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 29, 2019
@k8s-ci-robot k8s-ci-robot added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/storage Categorizes an issue or PR as relevant to SIG Storage. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 29, 2019
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels May 29, 2019
@gnufied
Copy link
Member

gnufied commented May 29, 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 29, 2019
@davidz627
Copy link
Contributor Author

/assign @saad-ali
/retest

@saad-ali
Copy link
Member

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, saad-ali

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 May 29, 2019
@davidz627
Copy link
Contributor Author

/retest

@mkimuram
Copy link
Contributor

@davidz627

If this PR fixes the issue, below comments aren't urgent, but it would be worth consider them later.

I thought that this was skipped by this code, as a result of the discussion in (#70823, #70823 (comment))
Isn't it enough? (Then, we might consider fixing these codes work correctly or removing them.)

Also, I guess that it would be better to add comments on

  • which combinations of providers and NodeOSDistros are tested against gcePdDriver,
  • which filesystem should be tested in which condition.

It seems that the combinations become more complex as windows node case is added, so it is difficult to see what is the right behavior just from the code.

@davidz627
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 29, 2019
@davidz627
Copy link
Contributor Author

@mkimuram thanks for bringing those other skips to my attention. I think it's better to add these FS skips there.

Looks like right now xfs is only skipped on gci, when in reality it should be skipped on everything except ubuntu.

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

/assign @mkimuram
PTAL at new change

@mkimuram
Copy link
Contributor

@davidz627

Could you let me know which particular NodeOSDistro actually fails?

I think that we should add the particular NodeOSDistro to the black list to skip xfs tests, instead of making it a white list for just "ubuntu" and "custom". (Xfs must be a common filesystem that can be supported in many linux distributions, so this change will affect tests for combinations of other drivers and other distributions. )

As I commented on the previous discussion, this code is shared in all storage testsuites across all drivers and will be tested by any NodeOSDistros, so we won't be able to whitelist all NodeOSDistros, here. (If it is only for gcepd driver code, like previous commit, it makes sense to just blacklisting some NodeOSDistros, because we can be sure that the driver will only be tested with some certain NodeOSDistros.)

@davidz627
Copy link
Contributor Author

@mkimuram I see, that's a fair point. I changed back it from a whitelist to a blacklist.

@davidz627
Copy link
Contributor Author

/retest

@mkimuram
Copy link
Contributor

@davidz627

Thank you for handling this.
The code looks good to me if the content of blacklist itself is correct. (And it must be correct, if all tests pass.)

@davidz627
Copy link
Contributor Author

/retest

@msau42
Copy link
Member

msau42 commented Jun 3, 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 Jun 3, 2019
@davidz627
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2019
@davidz627
Copy link
Contributor Author

Should probably fix: #71436

@davidz627
Copy link
Contributor Author

davidz627 commented Jun 6, 2019

@claurence this is a test fix only and might help improve signal on certain OS distributions (windows, COS, since this will remove irrelevant tests)
Just pinging in case you want to get this merged

@tpepper
Copy link
Member

tpepper commented Aug 8, 2019

Should probably fix: #71436

It may not have...OR it needs back ported to the prior branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. 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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants