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

fix azure disk translation issue due to lower case managed kind #103439

Merged
merged 2 commits into from Jul 28, 2021
Merged

fix azure disk translation issue due to lower case managed kind #103439

merged 2 commits into from Jul 28, 2021

Conversation

andyzhangx
Copy link
Member

@andyzhangx andyzhangx commented Jul 2, 2021 •
edited

What type of PR is this?

/kind bug

What this PR does / why we need it:

fix azure disk translation issue due to lower case managed kind

Which issue(s) this PR fixes:

Fixes #103433

Special notes for your reviewer:

Does this PR introduce a user-facing change?

fix azure disk translation issue due to lower case `managed` kind

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

fix azure disk translation issue due to lower case `managed` kind
 - deprecation notice:
1) `Shared`, `Dedicated` kind is not supported for Azure Disk CSI driver, only `managed` kind is supported in CSI migration.
2) `Shared`, `Dedicated` kind is deprecated in in-tree Azure Disk driver, and will receive error when "CSIMigrationAzureDisk" is turned on
 - about converting unmanaged(`Shared`, `Dedicated` kind) disk to managed disk
follow guide: https://docs.microsoft.com/en-us/azure/virtual-machines/windows/convert-unmanaged-to-managed-disks

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 2, 2021
@andyzhangx
Copy link
Member Author

/kind bug
/assign @Jiawei0227
/priority important-soon
/sig cloud-provider
/area provider/azure
/triage accepted

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. area/provider/azure Issues or PRs related to azure provider triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 2, 2021
@andyzhangx andyzhangx mentioned this pull request Jul 2, 2021
Closed
@Jiawei0227
Copy link
Contributor

/lgtm
Does this mean if we put lower case intree, it will still translate to upper case?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2021
msau42
msau42 reviewed Jul 14, 2021
View reviewed changes
staging/src/k8s.io/csi-translation-lib/plugins/azure_disk.go
if azureSource.Kind != nil {
pv.Spec.PersistentVolumeSource.CSI.VolumeAttributes[azureDiskKind] = string(*azureSource.Kind)
}
pv.Spec.PersistentVolumeSource.CSI.VolumeAttributes[azureDiskKind] = managed
Copy link
Member

Choose a reason for hiding this comment

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

Can something not managed be specified?

Copy link
Member Author

Choose a reason for hiding this comment

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

since CSI driver only supports managed, we can only support managed in csi migration.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we make the CSI driver case insensitive?

In theory, we could fix it in volume validation, while since this issue only exists in csi migration, may be not necessary now

Copy link
Member

Choose a reason for hiding this comment

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

What happens if someone is using a volume that is unmanaged with CSI migration?

Copy link
Member Author

Choose a reason for hiding this comment

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

What happens if someone is using a volume that is unmanaged with CSI migration?

it's not supported, volume would be in unattached state forever. unmanaged disk is actually deprecated in Azure, if user still wants to use unmanaged disk, they need to turn off csi migration for disk.

Copy link
Member

Choose a reason for hiding this comment

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

Can a user use unmanaged disks with in-tree driver today? If so, we may need to also send a deprecation notice for this.

Also, should we return an error if the kind is not supported?

Copy link
Member Author

Choose a reason for hiding this comment

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

already added the error code and depreciation notice in doc notes if kind is not supported.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, can you also clarify, is there any documentation we can point to users on how to migrate their disks from unmanaged to managed?

staging/src/k8s.io/csi-translation-lib/plugins/azure_disk.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 19, 2021
@andyzhangx
Copy link
Member Author

/retest

@andyzhangx
Copy link
Member Author

/retest

2 similar comments
@andyzhangx
Copy link
Member Author

/retest

@andyzhangx
Copy link
Member Author

/retest

@gnufied
Copy link
Member

gnufied commented Jul 20, 2021

/assign

@andyzhangx
Copy link
Member Author

@msau42 could you approve this PR? thanks.

@gnufied
Copy link
Member

gnufied commented Jul 20, 2021

@andyzhangx I think deprecation warning should really say, shared and dedicated types are deprecated for azure disk volumes. The CSI migration or not condition is implicit.

@andyzhangx
Copy link
Member Author

@andyzhangx I think deprecation warning should really say, shared and dedicated types are deprecated for azure disk volumes. The CSI migration or not condition is implicit.

changed to

 - deprecation notice:
`Shared`, `Dedicated` kind is not supported for Azure Disk CSI driver, only `managed` kind is supported in CSI migration.

@msau42
Copy link
Member

msau42 commented Jul 23, 2021

@andyzhangx I think @gnufied's suggestion is to deprecate the support for in-tree plugin too. Can we also provide a link to tell users how to migrate to managed disks?

@andyzhangx
Copy link
Member Author

@andyzhangx I think @gnufied's suggestion is to deprecate the support for in-tree plugin too. Can we also provide a link to tell users how to migrate to managed disks?

@msau42 added a link about how to migrate to managed disks. The in-tree plugin is already in maintainace mode and deprecated soon, I think that's a common sense.

fix azure disk translation issue due to lower case `managed` kind
 - deprecation notice:
`Shared`, `Dedicated` kind is not supported for Azure Disk CSI driver, only `managed` kind is supported in CSI migration.
 - convert unmanaged(`Shared`, `Dedicated` kind) disk to managed disk
follow guide: https://docs.microsoft.com/en-us/azure/virtual-machines/windows/convert-unmanaged-to-managed-disks

@msau42
Copy link
Member

msau42 commented Jul 26, 2021

@andyzhangx I think there is a subtle distinction. The in-tree driver is deprecated, however the in-tree API is still supported. So since this is something users used to be able to specify in the API, and will not be able to in the future, it should be called out.

@andyzhangx
Copy link
Member Author

@andyzhangx I think there is a subtle distinction. The in-tree driver is deprecated, however the in-tree API is still supported. So since this is something users used to be able to specify in the API, and will not be able to in the future, it should be called out.

@msau42 I added sth. here, is that ok?

fix azure disk translation issue due to lower case `managed` kind
 - deprecation notice:
1) `Shared`, `Dedicated` kind is not supported for Azure Disk CSI driver, only `managed` kind is supported in CSI migration.
2) `Shared`, `Dedicated` kind is deprecated in in-tree Azure Disk driver, and will receive error when "CSIMigrationAzureDisk" is turned on
 - about converting unmanaged(`Shared`, `Dedicated` kind) disk to managed disk
follow guide: https://docs.microsoft.com/en-us/azure/virtual-machines/windows/convert-unmanaged-to-managed-disks

@gnufied
Copy link
Member

gnufied commented Jul 27, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2021
@msau42
Copy link
Member

msau42 commented Jul 27, 2021

/approve

Thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, msau42

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 Jul 27, 2021
@feiskyer
Copy link
Member

/milestone v1.22

@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jul 28, 2021
@k8s-ci-robot k8s-ci-robot merged commit dadecb2 into kubernetes:master Jul 28, 2021
@andyzhangx andyzhangx mentioned this pull request Jul 28, 2021
Merged
k8s-ci-robot added a commit that referenced this pull request Jul 28, 2021
@k8s-ci-robot
Merge pull request #103968 from andyzhangx/automated-cherry-pick-of-#…
d655d00 
…103439-upstream-release-1.22

Automated cherry pick of #103439: fix azure disk translation issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@ddebroy ddebroy Awaiting requested review from ddebroy

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

Assignees

@gnufied gnufied

@Jiawei0227 Jiawei0227

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. 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.22
Development

Successfully merging this pull request may close these issues.

csi-translation-lib: Azure disk type is not properly translated
6 participants
@andyzhangx @Jiawei0227 @gnufied @msau42 @k8s-ci-robot @feiskyer