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

[cinder-csi-plugin] Fix filesystem resize #1434

Merged
merged 1 commit into from
Mar 11, 2021
Merged

[cinder-csi-plugin] Fix filesystem resize #1434

merged 1 commit into from
Mar 11, 2021

Conversation

Fedosin
Copy link
Contributor

@Fedosin Fedosin commented Mar 9, 2021 •
edited

Now we use "findmnt" command to find the device path. Unfortunately, this command may return multiple entries of the same mount, but in fact we need just the first one. To prevent this issue we add "--first-only" flag to the command.

Fixes #1436

[cinder-csi-plugin] Fixed the issue where the file system size stays the same when expanding the volume.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 9, 2021
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 9, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 9, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 9, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 9, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 9, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 9, 2021

Build succeeded.

ramineni
ramineni reviewed Mar 10, 2021
View reviewed changes
pkg/csi/cinder/nodeserver.go Outdated
// If Kubernetes version < v1.19.0 the volume_path would be
// having the staging_target_path information
volumePath = req.GetVolumePath()
}
Copy link
Contributor

@ramineni ramineni Mar 10, 2021 •
edited

Choose a reason for hiding this comment

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

@Fedosin could you explain your issue in more detail .
NodeExpandVolume can be called after nodePublish , in that case, in that case we cant use staging_target_path, volumepath stores the right value. please check here: https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/csi/csi_client.go#L101

It would be more helpful if you could log the issue with details and the values you are receiving in volumePath and staging_target_path

@ramineni
Copy link
Contributor

From Kubernetes v1.19 onwards NodeRequest is getting volume path in StagingTargetPath instead of VolumePath, Cinder CSI plugin should also use the same.

@Fedosin pls add reference docs for the above as well would be helpful. From the CSI spec and the https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/csi/csi_client.go#L101, we are using the right field IMO

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 10, 2021

@ramineni something like this:

I0824 09:24:45.735515 507082 utils.go:159] ID: 66 Req-ID: 0001-0011-openshift-storage-0000000000000001-cce6ebe3-e5ea-11ea-a920-0a580a810229 GRPC call: /csi.v1.Node/NodeExpandVolume
I0824 09:24:45.737321 507082 utils.go:160] ID: 66 Req-ID: 0001-0011-openshift-storage-0000000000000001-cce6ebe3-e5ea-11ea-a920-0a580a810229 GRPC request: {"capacity_range":{"required_bytes":2147483648},"staging_target_path":"/var/lib/kubelet/plugins/kubernetes.io/csi/pv/pvc-3c6426fa-7476-478f-b685-f8b7831ce70b/globalmount","volume_capability":{"AccessType":{"Mount":{"fs_type":"ext4"}},"access_mode":{"mode":1}},"volume_id":"0001-0011-openshift-storage-0000000000000001-cce6ebe3-e5ea-11ea-a920-0a580a810229","volume_path":"/var/lib/kubelet/pods/43de2997-5f4e-4fb3-a0e4-7e0d303c6c37/volumes/kubernetes.io~csi/pvc-3c6426fa-7476-478f-b685-f8b7831ce70b/mount"}

Because of this, expansion is a no-op as getDevidePath() is not able to find the device path based on volume_path.

@gnufied
Copy link
Member

gnufied commented Mar 10, 2021

shouldn't /var/lib/kubelet/pods/43de2997-5f4e-4fb3-a0e4-7e0d303c6c37/volumes/kubernetes.io~csi/pvc-3c6426fa-7476-478f-b685-f8b7831ce70b/mount be a bind mount of staging path?

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 10, 2021

/hold

@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 Mar 10, 2021
@Fedosin
[cinder-csi-plugin] Fix filesystem resize
0ab91dc 
Now we use "findmnt" command to find the device path. Unfortunately,
this command may return multiple entries of the same mount, but in
fact we need just the first one.

To prevent this issue we add "--first-only" flag to the command.
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 10, 2021
@Fedosin Fedosin changed the title [cinder-csi-plugin] NodeExpandVolume() should use StagingTargetPath [cinder-csi-plugin] Fix filesystem resize Mar 10, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 10, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 10, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 10, 2021

Build succeeded.

@gnufied
Copy link
Member

gnufied commented Mar 10, 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 Mar 10, 2021
@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 10, 2021

/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 Mar 10, 2021
@gnufied
Copy link
Member

gnufied commented Mar 10, 2021

@Fedosin can you add release notes please?

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 10, 2021

@gnufied as I know release notes are for public facing features. and this is just a bugfix, so I think fixes #1436 is enough.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 10, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 10, 2021

Build failed.

@gnufied
Copy link
Member

gnufied commented Mar 11, 2021

I am not sure how release notes are prepared for openstack cloudprovider but in rest of kubernetes repository, we do capture important bugs in release notes.

@jichenjc
Copy link
Contributor

/test cloud-provider-openstack-e2e-test-csi-cinder

@k8s-ci-robot
Copy link
Contributor

@jichenjc: 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-e2e-test-csi-cinder

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.

@jichenjc
Copy link
Contributor

I am +1 to list bug fixes in release notes as this seems best practices in other k8s ecosystem repo...

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 11, 2021

Build failed.

@jichenjc
Copy link
Contributor

/test cloud-provider-openstack-e2e-test-csi-cinder

@k8s-ci-robot
Copy link
Contributor

@jichenjc: 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-e2e-test-csi-cinder

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 Mar 11, 2021

Build succeeded.

@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 Mar 11, 2021
@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 11, 2021

@jichenjc @gnufied the release note has been added

@jichenjc
Copy link
Contributor

/approve

thanks

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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 Mar 11, 2021
@k8s-ci-robot k8s-ci-robot merged commit aa5e9fe into kubernetes:master Mar 11, 2021
@Fedosin Fedosin mentioned this pull request Mar 12, 2021
Merged
@lingxiankong
Copy link
Contributor

@ramineni Is this a candidate for backport?

@ramineni
Copy link
Contributor

@ramineni Is this a candidate for backport?

@lingxiankong right. I'll propose a PR for backport to 1.20

ramineni pushed a commit to ramineni/cloud-provider-openstack that referenced this pull request Mar 16, 2021
@Fedosin @ramineni
[cinder-csi-plugin] Fix filesystem resize (kubernetes#1434)
049fa57 
Now we use "findmnt" command to find the device path. Unfortunately,
this command may return multiple entries of the same mount, but in
fact we need just the first one.

To prevent this issue we add "--first-only" flag to the command.
k8s-ci-robot pushed a commit that referenced this pull request Mar 17, 2021
@ramineni @Fedosin
[cinder-csi-plugin] Fix filesystem resize (#1434) (#1443)
9a5688c 
Now we use "findmnt" command to find the device path. Unfortunately,
this command may return multiple entries of the same mount, but in
fact we need just the first one.

To prevent this issue we add "--first-only" flag to the command.

Co-authored-by: Mike Fedosin <mfedosin@redhat.com>
powellchristoph pushed a commit to powellchristoph/cloud-provider-openstack that referenced this pull request Jan 19, 2022
@Fedosin
[cinder-csi-plugin] Fix filesystem resize (kubernetes#1434)
b111a0f 
Now we use "findmnt" command to find the device path. Unfortunately,
this command may return multiple entries of the same mount, but in
fact we need just the first one.

To prevent this issue we add "--first-only" flag to the command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramineni ramineni ramineni left review comments

@lingxiankong lingxiankong Awaiting requested review from lingxiankong

@ricolin ricolin Awaiting requested review from ricolin

Assignees

@gnufied gnufied

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[cinder-csi-plugin] Filesystem size stays the same after PVC resize
6 participants
@Fedosin @ramineni @gnufied @jichenjc @k8s-ci-robot @lingxiankong