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

feat: support VolumeExpansion in OFFLINE scenario #121

Merged
merged 1 commit into from
Oct 16, 2019
Merged

feat: support VolumeExpansion in OFFLINE scenario #121

merged 1 commit into from
Oct 16, 2019

Conversation

ZeroMagic
Copy link
Member

What type of PR is this?
/kind feature

What this PR does / why we need it:
support VolumeExpansion in OFFLINE scenario

Which issue(s) this PR fixes:

Fixes #43

Special notes for your reviewer:

Release note:

feat: support VolumeExpansion in OFFLINE scenario

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 28, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 28, 2019
@ZeroMagic
Copy link
Member Author

/hold
Because the current version of CSI sanity test(v1.1.0) does not support ControllerServiceCapability_RPC_EXPAND_VOLUME, we need to update csi-test.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2019
andyzhangx
andyzhangx reviewed Sep 29, 2019
View reviewed changes
pkg/azurefile/controllerserver.go Outdated
volumeID := req.VolumeId
shareURL, err := d.getShareUrl(volumeID)
if err != nil {
klog.V(4).Infof("failed to get share url with (%s): %v, returning with success", volumeID, err)
Copy link
Member

Choose a reason for hiding this comment

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

klog.Errorf

Copy link
Member

Choose a reason for hiding this comment

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

why returning success here? by sanity test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, it is my fault. It should fail here.

pkg/azurefile/controllerserver.go Outdated
return &csi.ControllerExpandVolumeResponse{}, nil
}

_, err = shareURL.SetQuota(ctx, requestGiB)
Copy link
Member

Choose a reason for hiding this comment

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

if _, err = shareURL.SetQuota(ctx, requestGiB); err != nil {
...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@ZeroMagic
Copy link
Member Author

ZeroMagic commented Sep 30, 2019 •
edited
Loading

I will update the version of CSI sanity test to v2.2.0. The number of specs increases from 57 to 72. So some new bugs appear. For example, since we enabled csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT, we should check the snapshot if it exists when creating the volume.

They will be fixed in another PR.

@ZeroMagic ZeroMagic mentioned this pull request Oct 7, 2019
Merged
@ZeroMagic ZeroMagic force-pushed the resize branch 3 times, most recently from 18b1309 to 2464c31 Compare October 14, 2019 08:08
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 14, 2019
@ZeroMagic
Copy link
Member Author

/ok-to-test

@k8s-ci-robot
Copy link
Contributor

@ZeroMagic: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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/test-infra repository.

@andyzhangx
Copy link
Member

/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 Oct 14, 2019
@ZeroMagic
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2019
@andyzhangx
Copy link
Member

/retest

andyzhangx
andyzhangx approved these changes Oct 16, 2019
View reviewed changes
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
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Oct 16, 2019
@andyzhangx
Copy link
Member

btw, what's the OFFLINE scenario, I thought it's online scenario
@ZeroMagic

@k8s-ci-robot k8s-ci-robot merged commit b4ce4ae into kubernetes-sigs:master Oct 16, 2019
@ZeroMagic
Copy link
Member Author

btw, what's the OFFLINE scenario, I thought it's online scenario
@ZeroMagic

According to the developer documentation(https://kubernetes-csi.github.io/docs/volume-expansion.html#implementing-volume-expansion-functionality)
ONLINE : Volume is currently published or available on a node.
OFFLINE : Volume is currently not published or available on a node.

So we will call ControllerExpandVolume RPC in the OFFLINE scenario, and call the NodeExpandVolume RPC in the ONLINE scenario.

@andyzhangx
Copy link
Member

@ZeroMagic azure file should support online scenario, while azure disk only supports offline

levimm pushed a commit to levimm/azurefile-csi-driver that referenced this pull request Dec 25, 2019
@k8s-ci-robot
Merge pull request kubernetes-sigs#121 from ZeroMagic/none_exist_id
80b8c5e 
fix: return empty when the specify snapshot id does not exist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyzhangx andyzhangx andyzhangx approved these changes

@feiskyer feiskyer Awaiting requested review from feiskyer

Assignees

@andyzhangx andyzhangx

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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support volume resize in k8s v1.14
3 participants
@ZeroMagic @k8s-ci-robot @andyzhangx