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

Enable e2e test for Alpha Feature AnyVolumeDataSource #25709

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Enable e2e test for Alpha Feature AnyVolumeDataSource #25709

merged 1 commit into from
Mar 23, 2022

Conversation

mkimuram
Copy link
Contributor

This PR enables e2e test for Alpha Feature AnyVolumeDataSource in job pull-kubernetes-e2e-gce-alpha-features.

xref: kubernetes/kubernetes#108664

/sig storage
@bswartz @xing-yang

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 21, 2022
@k8s-ci-robot k8s-ci-robot added area/config Issues or PRs related to code in /config area/jobs area/provider/gcp Issues or PRs related to gcp provider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 21, 2022
@mkimuram mkimuram mentioned this pull request Mar 21, 2022
Merged
3 tasks
bswartz
bswartz approved these changes Mar 21, 2022
View reviewed changes
Copy link
Contributor

@bswartz bswartz left a comment

Choose a reason for hiding this comment

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

/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 21, 2022
@mkimuram
Copy link
Contributor Author

/assign @cheftako

Could you approve this PR?

andrewsykim
andrewsykim reviewed Mar 22, 2022
View reviewed changes
config/jobs/kubernetes/sig-cloud-provider/gcp/gcp-gce.yaml
@@ -423,7 +423,7 @@ presubmits:
- --provider=gce
- --runtime-config=api/all=true
- --stage=gs://kubernetes-release-pull/ci/pull-kubernetes-e2e-gce-alpha-features
- --test_args=--ginkgo.focus=\[Feature:(GRPCContainerProbe|InPlacePodVerticalScaling|ProbeTerminationGracePeriod|APIServerTracing|StorageVersionAPI|PodPreset|StatefulSetMinReadySeconds|CustomResourceValidationExpressions)\]|Networking --ginkgo.skip=\[Feature:(SCTPConnectivity|Volumes|Networking-Performance)\]|IPv6|csi-hostpath-v0 --minStartupPods=8
- --test_args=--ginkgo.focus=\[Feature:(GRPCContainerProbe|InPlacePodVerticalScaling|ProbeTerminationGracePeriod|APIServerTracing|StorageVersionAPI|PodPreset|StatefulSetMinReadySeconds|CustomResourceValidationExpressions|AnyVolumeDataSource)\]|Networking --ginkgo.skip=\[Feature:(SCTPConnectivity|Volumes|Networking-Performance)\]|IPv6|csi-hostpath-v0 --minStartupPods=8
Copy link
Member

Choose a reason for hiding this comment

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

Is there a Kubernetes PR validating that this test passes?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see kubernetes/kubernetes#108664, but do you know if this test ran? I think there's a way to configure a Kubernetes PR to run with a prow config from a PR, but I can't recall the details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewsykim Thank you for your review. IIUC, without --ginkgo.focus option for "[Feature:AnyVolumeDataSource]", the e2e tests containing "[Feature:AnyVolumeDataSource]" string are skipped. So, this PR is needed to make this job not to skip these tests. In the PR, the job is enabled with /test pull-kubernetes-e2e-gce-alpha-features, but the tests seem to be skipped.

Copy link
Member

Choose a reason for hiding this comment

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

This is kind of hacky, but could you temporary update the ginkgo tag in your PR to something else to have the test run and see if it passes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last commit which changed this line was #24393, and it enables [Feature:CustomResourceValidationExpressions] for the tests in k/k by this way. So, I think that this is the way the tests for alpha features have been enabled (and I agree that it looks hacky).

Copy link
Member

Choose a reason for hiding this comment

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

I'm not disagreeing with the approach to enable the tests, just asking if we can have a successful run of the test before enabling this for all alpha jobs. The (hacky) approach I mentioned is just a temporary hack to run the test in presubmit by using a known filter that works (just pick any of the existing alpha filters that are used).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test's PR is tested manually like this. The test requires alpha features, or AnyVolumeDataSource feature gate, to be enabled, which means removing [Feature:AnyVolumeDataSource] will make this test run in other jobs which don't enable the feature gate and fail.

So, I will update the test's PR if it is really required before merging this PR, however, I don't think that it should be a blocker for this PR (This PR could be merged without any side effects and the test's PR can be merged later without any dependencies).

Copy link
Member

Choose a reason for hiding this comment

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

The test's PR is tested manually like kubernetes/kubernetes#108664 (comment). The test requires alpha features, or AnyVolumeDataSource feature gate, to be enabled, which means removing [Feature:AnyVolumeDataSource] will make this test run in other jobs which don't enable the feature gate and fail.

This seems sufficient, thanks

andrewsykim
andrewsykim reviewed Mar 23, 2022
View reviewed changes
Copy link
Member

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, bswartz, mkimuram

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 23, 2022
@k8s-ci-robot k8s-ci-robot merged commit 6579214 into kubernetes:master Mar 23, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Mar 23, 2022
@k8s-ci-robot
Copy link
Contributor

@mkimuram: Updated the job-config configmap in namespace default at cluster test-infra-trusted using the following files:

  • key gcp-gce.yaml using file config/jobs/kubernetes/sig-cloud-provider/gcp/gcp-gce.yaml

In response to this:

This PR enables e2e test for Alpha Feature AnyVolumeDataSource in job pull-kubernetes-e2e-gce-alpha-features.

xref: kubernetes/kubernetes#108664

/sig storage
@bswartz @xing-yang

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.

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

@andrewsykim andrewsykim andrewsykim left review comments

@bswartz bswartz bswartz approved these changes

@cheftako cheftako Awaiting requested review from cheftako

Assignees

@cheftako cheftako

@bswartz bswartz

@andrewsykim andrewsykim

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config area/jobs area/provider/gcp Issues or PRs related to gcp provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. 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
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.
5 participants
@mkimuram @k8s-ci-robot @bswartz @andrewsykim @cheftako