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

WIP Disable in-tree plugins when CSI Migration enabled #80587

Open
wants to merge 1 commit into
base: master
from

Conversation

@ddebroy
Copy link
Member

commented Jul 25, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
This PR disables the main entry points of the in-tree plugins whose code is being migrated to CSI. This paves the path to provide us more confidence that the CSI migration shims are indeed working and the in-tree code, after being disabled for a while (with feature flags) can be removed.

Which issue(s) this PR fixes:
Fixes # #77237

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Disable in-tree plugins when corresponding feature flags for CSI migration are enabled.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE
@ddebroy

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

/sig storage

@ddebroy

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

/test pull-kubernetes-e2e-gce

@roycaihw

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

/sig cloud-provider
/remove-sig api-machinery
/cc @cheftako

@k8s-ci-robot k8s-ci-robot requested a review from cheftako Jul 25, 2019
@ddebroy ddebroy changed the title Disable in-tree plugins when CSI Migration enabled [WIP] Disable in-tree plugins when CSI Migration enabled Jul 25, 2019
@ddebroy ddebroy changed the title [WIP] Disable in-tree plugins when CSI Migration enabled [WIP Do-not-merge] Disable in-tree plugins when CSI Migration enabled Jul 25, 2019
@ddebroy ddebroy force-pushed the ddebroy:disable-plugins branch from cc3c2c6 to 10138b4 Jul 26, 2019
@ddebroy ddebroy changed the title [WIP Do-not-merge] Disable in-tree plugins when CSI Migration enabled Disable in-tree plugins when CSI Migration enabled Jul 26, 2019
@ddebroy

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

@ddebroy ddebroy force-pushed the ddebroy:disable-plugins branch from 10138b4 to 5234406 Jul 26, 2019
@davidz627

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

/priority important-soon


allPlugins = append(allPlugins, awsebs.ProbeVolumePlugins()...)
allPlugins = append(allPlugins, gcepd.ProbeVolumePlugins()...)
if !utilfeature.DefaultFeatureGate.Enabled(features.CSIMigration) || !utilfeature.DefaultFeatureGate.Enabled(features.CSIMigrationAWS) {

This comment has been minimized.

Copy link
@davidz627

davidz627 Jul 29, 2019

Contributor

could a minor refactor be done so we don't need to duplicate this feature gate checking code? Looks like all these various Probe functions have generally the same code and the same set of plugins. Wonder if they could be consolidated for the most part

This comment has been minimized.

Copy link
@ddebroy

ddebroy Aug 1, 2019

Author Member

Sure. We can discuss a bit if we want to align this more with https://github.com/kubernetes/kubernetes/pull/80353/files#diff-0967aee5156ab01aec200f53706a7135. I think for now, I can group them under append*LegacyProviderVolumes like @BenTheElder did.

@ddebroy

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

/retest

@ddebroy ddebroy force-pushed the ddebroy:disable-plugins branch 3 times, most recently from eb59822 to ae6d205 Aug 1, 2019
@ddebroy

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

/test pull-kubernetes-integration

@ddebroy

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

The PR test failure in pull-kubernetes-integration :: TestVolumeProvision is due to #80528

@ddebroy ddebroy changed the title Disable in-tree plugins when CSI Migration enabled WIP Disable in-tree plugins when CSI Migration enabled Aug 1, 2019
@ddebroy

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

Needs to properly handle resizing: FindExpandablePluginBySpec will not return a plugin that is not registered/removed.

@ddebroy ddebroy force-pushed the ddebroy:disable-plugins branch from ae6d205 to 90ce31b Aug 6, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Aug 6, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ddebroy
To complete the pull request process, please assign dchen1107, saad-ali
You can assign the PR to them by writing /assign @dchen1107 @saad-ali in a comment when ready.

The full list of commands accepted by this bot can be found 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

@ddebroy ddebroy force-pushed the ddebroy:disable-plugins branch 6 times, most recently from 27c4eb6 to 036d15e Aug 6, 2019
@ddebroy

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

/retest

Signed-off-by: Deep Debroy <ddebroy@docker.com>
@ddebroy ddebroy force-pushed the ddebroy:disable-plugins branch from 036d15e to a9b0d74 Aug 16, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@ddebroy: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-bazel-test a9b0d74 link /test pull-kubernetes-bazel-test
pull-kubernetes-e2e-gce-100-performance a9b0d74 link /test pull-kubernetes-e2e-gce-100-performance
pull-kubernetes-e2e-gce-iscsi a9b0d74 link /test pull-kubernetes-e2e-gce-iscsi
pull-kubernetes-e2e-gce a9b0d74 link /test pull-kubernetes-e2e-gce
pull-kubernetes-e2e-gce-storage-slow a9b0d74 link /test pull-kubernetes-e2e-gce-storage-slow
pull-kubernetes-e2e-gce-csi-serial a9b0d74 link /test pull-kubernetes-e2e-gce-csi-serial
pull-kubernetes-e2e-gce-iscsi-serial a9b0d74 link /test pull-kubernetes-e2e-gce-iscsi-serial

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.