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

Upgrade CSIMigrationGCE feature gate to GA #111301

Merged
merged 1 commit into from
Aug 2, 2022
Merged

Upgrade CSIMigrationGCE feature gate to GA #111301

merged 1 commit into from
Aug 2, 2022

Conversation

mattcary
Copy link
Contributor

Upgrade CSIMigrationGCE feature gate to GA.

KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1488-csi-migration-gce-pd
/kind feature
For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->

CSIMigrationGCE upgraded to GA and locked to true.

/sig storage
/assign @msau42
/cc @xing-yang
/cc @saad-ali

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 21, 2022
@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. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 21, 2022
@msau42
Copy link
Member

msau42 commented Jul 21, 2022

Do you have a link to a passing testgrid?

@rhockenbury rhockenbury mentioned this pull request Jul 21, 2022
Closed
2 tasks
@mattcary
Copy link
Contributor Author

The tests should have been running in cloud-provider-gcp, but weren't. A fix is in process: kubernetes/test-infra#26890.

It turns out the kubetest2 isn't compatible with env vars, so the strategy in the e2e tests in #109541 doesn't actually work :-(

@xing-yang
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 24, 2022
@mattcary
Copy link
Contributor Author

@msau42 The cloud-provider-gcp tests are difficult, it turns out kubetest2 doesn't work with the env var based test config :-( That may take a while to fix.

In the meantime, the pd csi migration tests have still been running and are green: https://testgrid.k8s.io/provider-gcp-compute-persistent-disk-csi-driver#Migration%20Kubernetes%20Master%20Driver%20Stable It's not the full e2e test suite afaict, but it seems to be a good smoke test?

@msau42
Copy link
Member

msau42 commented Jul 27, 2022

The cloud-provider-gcp tests are difficult, it turns out kubetest2 doesn't work with the env var based test config :-( That may take a while to fix.

Running the full k8s storage e2e tests is a requirement we've been enforcing for all drivers. Should we revisit #109541 and make it a flag instead?

@mattcary
Copy link
Contributor Author

Running the full k8s storage e2e tests is a requirement we've been enforcing for all drivers. Should we revisit #109541 and make it a flag instead?

There's ongoing conversation on kubernetes/test-infra#26890. I don't think it will be resolved before the code freeze, but I should be able to get that done in the next few weeks.

@msau42
Copy link
Member

msau42 commented Jul 27, 2022

In the meantime, could you share screenshots of gke running the oss tests with csi migration? Or maybe @gnufied @jsafrane are there openshift test results we could share?

@mattcary
Copy link
Contributor Author

Attached is a screenshot of our internal test on 1.23 clusters, which has GCE migration switched on.

image

@mattcary
Copy link
Contributor Author

/retest

@dims
Copy link
Member

dims commented Aug 1, 2022

/milestone v1.25

@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Aug 1, 2022
@xing-yang
Copy link
Contributor

/assign @gnufied

@gnufied
Copy link
Member

gnufied commented Aug 1, 2022

The unit test related failures looks genuine. Here is Openshift tests running on GCE with CSI migration enabled - https://testgrid.k8s.io/redhat-openshift-ocp-release-4.11-informing#periodic-ci-openshift-release-master-nightly-4.11-e2e-gcp-csi-migration

I know this is 4.11 which is basically v1.24 release. So I think e2e tests are good. The failures you seen in those tests are from Openshift operator behaviour tests but all the storage tests are passing.

What we need to fix is unit tests though.

@mattcary
Copy link
Contributor Author

mattcary commented Aug 2, 2022

The unit test related failures looks genuine. Here is Openshift tests running on GCE with CSI migration enabled - https://testgrid.k8s.io/redhat-openshift-ocp-release-4.11-informing#periodic-ci-openshift-release-master-nightly-4.11-e2e-gcp-csi-migration

I know this is 4.11 which is basically v1.24 release. So I think e2e tests are good. The failures you seen in those tests are from Openshift operator behaviour tests but all the storage tests are passing.

What we need to fix is unit tests though.

Yeah, I don't know how I missed those failures. I'm no it.

@mattcary mattcary force-pushed the migration-feature branch from 4223736 to 78b0c1c Compare August 2, 2022 15:01
@k8s-ci-robot k8s-ci-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 2, 2022
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/kubelet sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels Aug 2, 2022
@mattcary mattcary force-pushed the migration-feature branch from 78b0c1c to 6155ca3 Compare August 2, 2022 15:15
@mattcary
Copy link
Contributor Author

mattcary commented Aug 2, 2022

\o/ I got the unit tests passing.

@mattcary
Upgrade CSIMigrationGCE feature gate to GA
e5d387c 
Change-Id: I620bc4913765c0d6562eb1008216a72e8b0a2970
@mattcary mattcary force-pushed the migration-feature branch from 6155ca3 to e5d387c Compare August 2, 2022 16:14
@xing-yang
Copy link
Contributor

/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 Aug 2, 2022
@dims
Copy link
Member

dims commented Aug 2, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, mattcary, xing-yang

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 Aug 2, 2022
@k8s-ci-robot k8s-ci-robot merged commit 369a465 into kubernetes:master Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@xing-yang xing-yang Awaiting requested review from xing-yang

Assignees

@gnufied gnufied

@xing-yang xing-yang

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.
6 participants
@mattcary @msau42 @xing-yang @dims @gnufied @k8s-ci-robot