Skip to content

Only add disk support topologies if StorageClass parameter is true #2089

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

julianKatz
Copy link
Contributor

@julianKatz julianKatz commented May 12, 2025

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR makes the disk topology feature only add a disk support label to PV NodeAffinity if a new StorageClass parameter is specified: use-allowed-disk-topology: true.

The benefit of this is that GKE can enable the --disk-topology=true flag without immediately changing PDCSI's behavior. The feature becomes available, but not enabled. Customers can then choose to enable the feature when they are ready, plus they gain the granularity of enabling it for specific disk products at a time. Since most customers need this for Hyperdisk usage, that allows them to target the functionality to their use case.

I've manually tested this feature. See the steps recorded here.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Only add disk support topologies if StorageClass parameter is true

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 12, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: julianKatz
Once this PR has been reviewed and has the lgtm label, please assign amacaskill for approval. For more information see the 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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 12, 2025
@julianKatz
Copy link
Contributor Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 12, 2025
@julianKatz julianKatz force-pushed the enable-disk-topology-with-StorageClass-parameter branch from 9c74cce to 44abc6e Compare May 16, 2025 20:14
@julianKatz julianKatz marked this pull request as ready for review May 16, 2025 20:17
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 16, 2025
@k8s-ci-robot k8s-ci-robot requested review from mattcary and tonyzhc May 16, 2025 20:17
@mattcary
Copy link
Contributor

This should have a release note as it adds a user-visible feature.

@tonyzhc
Copy link
Contributor

tonyzhc commented May 19, 2025

Can you add an e2e test in test/e2e/tests/single_zone_e2e_test.go as well?

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 19, 2025
@sunnylovestiramisu
Copy link
Contributor

Is this at least manually end to end tested on GCE clusters? Maybe add the test cases you did in the description.

@julianKatz
Copy link
Contributor Author

Is this at least manually end to end tested on GCE clusters? Maybe add the test cases you did in the description.

Added! Find them here and also in the description.

@julianKatz
Copy link
Contributor Author

This should have a release note as it adds a user-visible feature.

Is this more than what's contained in the PR description? I have the release-note label and I'm looking for PRs that show changes to a separate file but not finding any.

@julianKatz julianKatz force-pushed the enable-disk-topology-with-StorageClass-parameter branch from 44abc6e to 23cab0c Compare May 22, 2025 00:01
@julianKatz
Copy link
Contributor Author

/retest

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants