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.

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

[cinder-csi-plugin] Resize filesystem of a larger volume created from a snapshot/volume #1563

Merged
merged 1 commit into from Jun 21, 2021

Conversation

alibo
Copy link
Contributor

@alibo alibo commented Jun 4, 2021

What this PR does / why we need it:

If CO requests a volume to be created from existing snapshot or volume and the requested size of the volume is larger than the original snapshotted (or cloned volume), the Plugin can either refuse such a call with OUT_OF_RANGE error or MUST provide a volume that, when presented to a workload by NodePublish call, has both the requested (larger) size and contains data from the snapshot (or original volume). Explicitly, it's the responsibility of the Plugin to resize the filesystem of the newly created volume at (or before) the NodePublish call, if the volume has VolumeCapability access type MountVolume and the filesystem resize is required in order to provision the requested capacity.
container-storage-interface/spec#452

Which issue this PR fixes(if applicable):
fixes #1539

Special notes for reviewers:

  • NeedResize() is added to k8s.io/mount-utils@v0.21.1, so I had to update it.

Release note:

[cinder-csi-plugin] Fixed the issue when expanding volume that is created from a snapshot or another volume.

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Jun 4, 2021
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 4, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @alibo!

It looks like this is your first PR to kubernetes/cloud-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @alibo. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 4, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 4, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 4, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 4, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 4, 2021

Build succeeded.

@alibo
Copy link
Contributor Author

alibo commented Jun 4, 2021

I signed it

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 4, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 4, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 4, 2021

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 4, 2021

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 4, 2021

Build failed.

@alibo
Copy link
Contributor Author

alibo commented Jun 4, 2021

Worth mentioning I tried writing a new test in nodeserver_test.go, but I faced issues with existing mocked methods/structs:

  • OpenStackMock.GetVolume always returns
volumes.Volume{
	ID:               "261a8b81-3660-43e5-bab8-6470b65ee4e9",
	Name:             "fake-duplicate",
	Status:           "available",
	AvailabilityZone: "nova",
	Size:             1,
}

I've tried to fix it, but many tests failed

  • GetVolume in TestCreateVolumeFromSourceVolume should be mocked explicitly to avoid returning default data defined in OpenStackMock
  • Sometimes, the returned object is cached from previous tests because mocked objects are defined in init().
  • ResizeFS in mount-utils is better to be mocked and injected into nodeserver.go, so its method calls can be easily tested in NodeStageVolume and NodeExpandVolume methods by mocking them instead of using FakeExec commands (which is also used in mocked version of Mounter() method) which (IMHO) would test the functionality of resize2fs pkg itself.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 4, 2021

Build failed.

@alibo
Copy link
Contributor Author

alibo commented Jun 4, 2021

Build failed.

* [cloud-provider-openstack-multinode-csi-migration-test ](https://logs.openlabtesting.org/logs/63/1563/0fcf03706514d1c3d2261259915b60746f5c180f/cloud-provider-openstack-multinode-csi-migration-test/cloud-provider-openstack-multinode-csi-migration-test/f1a9c8a/) : FAILURE in 24m 50s
  Warning  Failed     3m55s (x3 over 4m46s)  kubelet            Failed to pull image "192.168.0.6:32000/k8scloudprovider/cinder-csi-plugin:latest": rpc error: code = Unknown desc = Error response from daemon: Get http://192.168.0.6:32000/v2/: dial tcp 192.168.0.6:32000: connect: no route to host
  Warning  Failed     3m55s (x3 over 4m46s)  kubelet            Error: ErrImagePull
  Normal   BackOff    3m28s (x4 over 4m46s)  kubelet            Back-off pulling image "192.168.0.6:32000/k8scloudprovider/cinder-csi-plugin:latest"
  Warning  Failed     3m28s (x4 over 4m46s)  kubelet            Error: ImagePullBackOff

it's weired 🤔

@alibo
Copy link
Contributor Author

alibo commented Jun 4, 2021

/test cloud-provider-openstack-multinode-csi-migration-test

@k8s-ci-robot
Copy link
Contributor

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

In response to this:

/test cloud-provider-openstack-multinode-csi-migration-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.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 5, 2021

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 5, 2021

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 5, 2021

Build failed.

@jichenjc
Copy link
Contributor

jichenjc commented Jun 7, 2021

/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 Jun 7, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 18, 2021

Build succeeded.

@ramineni
Copy link
Contributor

/test cloud-provider-openstack-multinode-csi-migration-test

@k8s-ci-robot
Copy link
Contributor

@ramineni: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/test cloud-provider-openstack-multinode-csi-migration-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.

@ramineni
Copy link
Contributor

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

@k8s-ci-robot
Copy link
Contributor

@ramineni: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

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.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 18, 2021

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 18, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 18, 2021

Build succeeded.

@alibo
Copy link
Contributor Author

alibo commented Jun 19, 2021

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19 🙏📿🛐

@k8s-ci-robot
Copy link
Contributor

@alibo: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19 🙏📿🛐

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.

@alibo
Copy link
Contributor Author

alibo commented Jun 19, 2021

@ramineni do you have any idea how I can fix cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19 test? It's the only one is failed and I guess it's not related to this change since it's for another branch.

@alibo
Copy link
Contributor Author

alibo commented Jun 19, 2021

@jichenjc @lingxiankong can you please review it again?

@lingxiankong
Copy link
Contributor

/override openlab/cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

@alibo Don't worry about that job, it needs to be fixed.

@k8s-ci-robot
Copy link
Contributor

@lingxiankong: Overrode contexts on behalf of lingxiankong: openlab/cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

In response to this:

/override openlab/cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

@alibo Don't worry about that job, it needs to be fixed.

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.

@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 release-note-none Denotes a PR that doesn't merit a release note. labels Jun 19, 2021
@lingxiankong
Copy link
Contributor

/lgtm

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

@alibo Thanks for the PR.

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ramineni

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 Jun 21, 2021
@k8s-ci-robot k8s-ci-robot merged commit 6c34dc4 into kubernetes:master Jun 21, 2021
@alibo alibo deleted the fix/snapshot-expansion branch June 21, 2021 05:36
@lingxiankong
Copy link
Contributor

@ramineni Is this one a good candidate to backport?

@ramineni
Copy link
Contributor

@lingxiankong I'm not sure the updated package 1.21.1 will be compatible with 1.20 kubernetes and other packages. Lets hold it until someone needs it.

@lingxiankong
Copy link
Contributor

@lingxiankong I'm not sure the updated package 1.21.1 will be compatible with 1.20 kubernetes and other packages. Lets hold it until someone needs it.

ack

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. 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.

[cinder-csi-plugin] filesystem of a larger volume created from a snapshot/volume is not expanded
5 participants