Skip to content

Conversation

@MartinForReal
Copy link
Contributor

@MartinForReal MartinForReal commented Dec 13, 2023

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Refactor: migrate blobclient to track2 sdk and fix ut cases

Which issue(s) this PR fixes:

Fixes #

Requirements:

Special notes for your reviewer:
gomock controller is not closed after ut test is complete. This may miss unexpected function call error.

Release note:

none

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 13, 2023
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 13, 2023
@MartinForReal MartinForReal changed the title refactor:migrate blobclient to track2 sdk Refactor:migrate blobclient to track2 sdk Dec 13, 2023
@MartinForReal MartinForReal changed the title Refactor:migrate blobclient to track2 sdk Refactor: migrate blobclient to track2 sdk and fix ut cases Dec 13, 2023
@MartinForReal
Copy link
Contributor Author

/retest

1 similar comment
@MartinForReal
Copy link
Contributor Author

/retest

Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

@cvvz could you also take a look at this PR? thanks.

@andyzhangx
Copy link
Member

all the 3 Azure CSI drivers supports empty cloud provider config, e.g.

if allowEmptyCloudConfig {
klog.V(2).Infof("no cloud config provided, error: %v, driver will run without cloud config", err)

we need to make sure if there is no cloud provider config provided, this driver won't crash.

@MartinForReal
Copy link
Contributor Author

If the cloud config is nil, the blob client is nil. How do we check volume capabilities in this scenario? @andyzhangx

@MartinForReal MartinForReal changed the title Refactor: migrate blobclient to track2 sdk and fix ut cases [WIP]Refactor: migrate blobclient to track2 sdk and fix ut cases Dec 14, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 14, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 30, 2023
Copy link
Member

@cvvz cvvz left a comment

Choose a reason for hiding this comment

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

If the cloud config is nil, the blob client is nil. How do we check volume capabilities in this scenario? @andyzhangx

I believe k8s and sidecars actually never call ValidateVolumeCapabilities, although CSI spec requires CSI driver to implement it...

@andyzhangx
Copy link
Member

If the cloud config is nil, the blob client is nil. How do we check volume capabilities in this scenario? @andyzhangx

I believe k8s and sidecars actually never call ValidateVolumeCapabilities, although CSI spec requires CSI driver to implement it...

If the cloud config is nil, then user should provide secrets(bring your own account key scenario), ValidateVolumeCapabilities would work well with secrets.

@cvvz
Copy link
Member

cvvz commented Jan 9, 2024

If the cloud config is nil, then user should provide secrets(bring your own account key scenario), ValidateVolumeCapabilities would work well with secrets.

Yes, but it can be very easy to make csi driver crash (rather than throw error) once csi driver walk into the code that use d.cloud.BlobClient.

@andyzhangx
Copy link
Member

If the cloud config is nil, then user should provide secrets(bring your own account key scenario), ValidateVolumeCapabilities would work well with secrets.

Yes, but it can be very easy to make csi driver crash (rather than throw error) once csi driver walk into the code that use d.cloud.BlobClient.

then we should check whether d.cloud.BlobClient is nil, if it's nil, then return error directly instead of crashing.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 18, 2024
@MartinForReal MartinForReal changed the title [WIP]Refactor: migrate blobclient to track2 sdk and fix ut cases Refactor: migrate blobclient to track2 sdk and fix ut cases Jan 18, 2024
@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 Jan 18, 2024
@MartinForReal
Copy link
Contributor Author

/retest

1 similar comment
@MartinForReal
Copy link
Contributor Author

/retest

@andyzhangx
Copy link
Member

If the cloud config is nil, then user should provide secrets(bring your own account key scenario), ValidateVolumeCapabilities would work well with secrets.

Yes, but it can be very easy to make csi driver crash (rather than throw error) once csi driver walk into the code that use d.cloud.BlobClient.

@cvvz could you take a look at this PR? any concern about blobclient issue?

Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Jan 19, 2024
@k8s-ci-robot k8s-ci-robot merged commit fb1b499 into kubernetes-sigs:master Jan 19, 2024
@MartinForReal MartinForReal deleted the shafan/token branch January 19, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

4 participants