Skip to content

OSDOCS-13459#Add cross-subscription support for Azure File #92108

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 1 commit into
base: main
Choose a base branch
from

Conversation

lpettyjo
Copy link
Contributor

@lpettyjo lpettyjo commented Apr 11, 2025

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 11, 2025
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Apr 11, 2025

@lpettyjo lpettyjo added branch/enterprise-4.19 peer-review-needed Signifies that the peer review team needs to review this PR labels Apr 11, 2025
@lpettyjo lpettyjo added this to the Planned for 4.19 GA milestone Apr 11, 2025
@mburke5678 mburke5678 added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Apr 11, 2025
@mburke5678
Copy link
Contributor

@lpettyjo A few suggestions. Otherwise LGTM

@mburke5678 mburke5678 removed the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Apr 11, 2025
@mburke5678 mburke5678 added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Apr 11, 2025
@lpettyjo lpettyjo force-pushed the OSDOCS-13459 branch 2 times, most recently from 15688e3 to af08584 Compare April 23, 2025 12:38
@lpettyjo lpettyjo force-pushed the OSDOCS-13459 branch 2 times, most recently from 1a5413b to cc9e7f4 Compare May 13, 2025 12:39
----
<1> The name of the PV.
<2> The size of the PV.
<3> The storage class name.
Copy link

Choose a reason for hiding this comment

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

In case of static provisioning the SC name does not really matter it does not need to actually exists right? Should we call it out explicitly to avoid confusion ? i.e avoid customers wondering if they should create one.

@duanwei33

Choose a reason for hiding this comment

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

I think it matters, the sc name in PV and PVC helps binding, it could help avoid unexpected behaviour. (Like there are other azure-disk PVs without sc name and the size is the same)
But yes, the storage class itself (with the same name) exists or not will not impact the binding, I think for customer scenario, non-admin users have no access to list PV, so usually an admin will create a sc which a meaningful name and create pvs with the same sc name inside, then the non-admin users create the PVC with the sc for expected pvs.

In short, let's keep sc name in both pvc and pv.

Copy link

Choose a reason for hiding this comment

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

ok to keep it but it can be misleading as customers will not know what to set here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so.....leave as is or change?

Copy link

Choose a reason for hiding this comment

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

I was wondering if we should mention that the SC does not need to exist.
What happens if a user sets an existing (unrelated) storage class?

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think we should keep the SC in the example + comment that it's used to disable dynamic provisioning for such PV / PVC.

What happens if a user sets an existing (unrelated) storage class?

In theory nothing, the PVC will try to bind to an existing PV. In practice, when the manually created PV and PVC accidentally don't match, Kubernetes will try to dynamically provision a new PV. Which is not what the user wants. Either a dedicated SC with provisioner: kubernetes.io/no-provisioner or non-existing SC will ensure the dynamic provisioning does not happen. Mis-matched PV + PVC stay Pending and user can fix their errors.

Copy link

Choose a reason for hiding this comment

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

Agree to keep the SC in the PVC/PV definition i was just wondering if we could add an extra note on the fact that the SC should not actually to avoid potential user errors (or as Jan said create a no-provisioner one). Not mentioning anything can be error prone i think

Copy link

openshift-ci bot commented May 13, 2025

@lpettyjo: all tests passed!

Full PR test history. Your PR dashboard.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.19 peer-review-done Signifies that the peer review team has reviewed this PR 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.

7 participants