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

add KEP to remove dynamic provisioning from in-tree plugins #4548

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carlory
Copy link
Member

@carlory carlory commented Mar 14, 2024

  • One-line PR description: remove dynamic provisioning from in-tree plugins
  • Other comments:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Mar 14, 2024
@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 14, 2024
@carlory
Copy link
Member Author

carlory commented May 30, 2024

/assign @xing-yang

@xing-yang
Copy link
Contributor

that support it.

Once the migration plan of the portworxVolume plugin is completed, only the hostPath
in-tree volume plugin will support dynamic provisioning. The dynamic provisioning support
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there tests currently using the in-tree hostpath plugin for dynamic provisioning?

Copy link
Member Author

Choose a reason for hiding this comment

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

No.

For e2e tests, both hostPathDriver and hostPathSymlinkDriver don't implement the storageframework.DynamicPVTestDriver interface in the test/e2e/storage/drivers/in_tree.go.

For unit tests, mockVolumePlugin defined in pkg/controller/volume/persistentvolume/framework_test.go is used.

For intergation tests, FakeVolumePlugin defined in pkg/volume/testing/testing.go is used in test/integration/volume/persistent_volumes_test.go


Not needed for this KEP. no new features are added.

### Graduation Criteria
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have conformance tests for dynamic provisioning that will need to be removed?

Copy link
Member Author

@carlory carlory Jun 7, 2024

Choose a reason for hiding this comment

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

No, this KEP is aimed to remove dynamic provisioning for in-tree volume plugins. But host-path drivers in e2e tests don't implement the storageframework.DynamicPVTestDriver interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some unit and integration tests need to be removed. see #4548 (comment)

@carlory carlory force-pushed the remove-intree-volume-dynamic-provisioning branch from 7fb1911 to e1462f8 Compare June 7, 2024 09:57
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: carlory
Once this PR has been reviewed and has the lgtm label, please ask for approval from xing-yang and additionally assign deads2k for approval. For more information see the Kubernetes Code Review Process.

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

@carlory carlory force-pushed the remove-intree-volume-dynamic-provisioning branch from e1462f8 to 6d3283e Compare June 7, 2024 10:01
@carlory carlory force-pushed the remove-intree-volume-dynamic-provisioning branch from 6d3283e to 3518457 Compare June 7, 2024 10:23
@k8s-ci-robot
Copy link
Contributor

@carlory: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-enhancements-verify 3518457 link true /test pull-enhancements-verify

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

Comment on lines +519 to +521
- [ ] Feature gate (also fill in values in `kep.yaml`)
- Feature gate name:
- Components depending on the feature gate:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a deprecation feature gate for this change? I.e. add a feature gate that is off-by-default but provides any cluster administrators that run into problems with the feature removal with a way to re-enable it for one release before it is removed permanently?

cc @msau42

Copy link
Contributor

@jpbetz jpbetz Jun 7, 2024

Choose a reason for hiding this comment

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

PRR reviewers discussed this as well. Our general agreement is that deprecations should be feature gated (this means introducing a feature gate specifically to aid the deprecation in this case) to allow for a softer landing: "featuregate disables this soft-removed functionality by default, but you can re-enable for one release by setting this gate. The next release, the gate will locked to removed".

EDIT: @deads2k also pointed out: "where "next release" may end up informed by a future skip level upgrade policy" (note that node skew policy is N-3 compatibility with apiserver)

@jpbetz
Copy link
Contributor

jpbetz commented Jun 13, 2024

PRR reviewer here, #4548 (review), just wanted to point out that is still open, and would be required for this enhancement, and enhancement freeze is tomorrow.

@carlory
Copy link
Member Author

carlory commented Jun 13, 2024

@jpbetz I have to postpone it to 1.32 because of a personal issue. I will update the KEP according to the feedback when I have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants