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

change node staging path for csi driver to PV agnostic #107065

Merged
merged 2 commits into from
Jan 21, 2022

Conversation

saikat-royc
Copy link
Member

@saikat-royc saikat-royc commented Dec 16, 2021

What type of PR is this?

/kind bug

What this PR does / why we need it:

Without this fix, PVs pointing to the same underlying volume handle, fail to mount on the same node. This is because kubelet is confused into thinking that the volume is already node staged.
Fixes:

  1. By making the staging path PV agnostic, a unique volume will be node staged only once. And this path will be bind-mounted on the pod specific path during pod bringup on nodes.
  2. The global path uses a sha of the volumeId. This means the node staged path can no longer be used to backtrace the PV information (which is used to figure out Driver and VolumeID). This change therefore creates a hard dependency on the vol_data.json to exist with correct driver and volumeID information.
  3. A fallback option is provided such that if the vol_data.json is missing, kubelet will try to parse the PV information from the old path format.

Which issue(s) this PR fixes:

Fixes #105899

Special notes for your reviewer:

CSI migration flows should not be impacted, because it is expected that the node is drained before migration is enabled.

An example staging path looks like this for PDCSI driver:

saikatroyc@e2e-test-saikatroyc-minion-group-5h8f ~ $ sudo ls /var/lib/kubelet/plugins/kubernetes.io/csi/pd.csi.storage.gke.io/d7b18a91056b638c3459de6043a10982e468f3b400c42c8e467e023b24aeab08
globalmount  vol_data.json

Does this PR introduce a user-facing change?

Change node staging path for csi driver to use a PV agnostic path. Nodes must be drained before updating the kubelet with this change.

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


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. 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. labels Dec 16, 2021
@saikat-royc
Copy link
Member Author

/hold not yet ready for review.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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 Dec 16, 2021
@saikat-royc
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 20, 2021
@saikat-royc
Copy link
Member Author

/assign @jingxu97

@saikat-royc
Copy link
Member Author

/retest

1 similar comment
@saikat-royc
Copy link
Member Author

/retest

@saikat-royc
Copy link
Member Author

@jingxu97 @jsafrane the PR is ready for review. Based on the initial review comments, I will add a e2e test as needed.

Copy link
Member

@jsafrane jsafrane left a comment

Choose a reason for hiding this comment

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

This looks good to me if we explicitly add a release note that nodes must be drained during update. It is implied by upgrade, but IMO an explicit note would help to avoid confusion.

And we can't ever cherry pick this into an older release.

It would be trivial to test if the old staging path exists and has a valid json file in makeDeviceMountPath and use that (although, unit tests would be not that simple).

pkg/volume/csi/csi_attacher.go Outdated Show resolved Hide resolved
pkg/volume/csi/csi_attacher_test.go Outdated Show resolved Hide resolved
@saikat-royc
Copy link
Member Author

This looks good to me if we explicitly add a release note that nodes must be drained during update. It is implied by upgrade, but IMO an explicit note would help to avoid confusion.

And we can't ever cherry pick this into an older release.

It would be trivial to test if the old staging path exists and has a valid json file in makeDeviceMountPath and use that (although, unit tests would be not that simple).

Added a release note. Also the UTs (TestAttacherUnmountDevice) cover the old and new path test cases.

@saikat-royc
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 Dec 21, 2021
@saikat-royc
Copy link
Member Author

/kind bug

nixpanic added a commit to nixpanic/kubernetes-csi-addons that referenced this pull request Jun 14, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the CSI-Addons NodeReclaimSpace procedure, must receive the
correct path, otherwise the driver will not be able to free space and
possibly return an error.

See-also: kubernetes/kubernetes#107065
See-also: https://bugzilla.redhat.com/2096209
Signed-off-by: Niels de Vos <ndevos@redhat.com>
nixpanic added a commit to nixpanic/kubernetes-csi-addons that referenced this pull request Jun 15, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the CSI-Addons NodeReclaimSpace procedure, must receive the
correct path, otherwise the driver will not be able to free space and
possibly return an error.

See-also: kubernetes/kubernetes#107065
See-also: https://bugzilla.redhat.com/2096209
Signed-off-by: Niels de Vos <ndevos@redhat.com>
mergify bot pushed a commit to csi-addons/kubernetes-csi-addons that referenced this pull request Jun 15, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the CSI-Addons NodeReclaimSpace procedure, must receive the
correct path, otherwise the driver will not be able to free space and
possibly return an error.

See-also: kubernetes/kubernetes#107065
See-also: https://bugzilla.redhat.com/2096209
Signed-off-by: Niels de Vos <ndevos@redhat.com>
nixpanic added a commit to nixpanic/kubernetes-csi-addons that referenced this pull request Jun 15, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the CSI-Addons NodeReclaimSpace procedure, must receive the
correct path, otherwise the driver will not be able to free space and
possibly return an error.

See-also: kubernetes/kubernetes#107065
See-also: https://bugzilla.redhat.com/2096209
Signed-off-by: Niels de Vos <ndevos@redhat.com>
(cherry picked from commit eb0718f)
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/kubernetes-csi-addons that referenced this pull request Jun 15, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the CSI-Addons NodeReclaimSpace procedure, must receive the
correct path, otherwise the driver will not be able to free space and
possibly return an error.

See-also: kubernetes/kubernetes#107065
See-also: https://bugzilla.redhat.com/2096209
Signed-off-by: Niels de Vos <ndevos@redhat.com>
(cherry picked from commit eb0718f)
pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request Jun 23, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the CSI-Addons volumeHealer, must receive the correct path,
otherwise the after a nodeplugin restart the NBD mounts will bailout
attempting to NodeStageVolume() call and return an error.

See-also: kubernetes/kubernetes#107065

Fixes: ceph#3176
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
pkalever pushed a commit to pkalever/rook that referenced this pull request Jun 23, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes.

The backward compatibility should be taken care by the CSI driver.

See-also: kubernetes/kubernetes#107065
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request Jun 23, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the volumeHealer, must receive the correct path, otherwise
the after a nodeplugin restart the NBD mounts will bailout attempting
to NodeStageVolume() call and return an error.

See-also: kubernetes/kubernetes#107065

Fixes: ceph#3176
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request Jun 23, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the volumeHealer, must receive the correct path, otherwise
the after a nodeplugin restart the NBD mounts will bailout attempting
to NodeStageVolume() call and return an error.

See-also: kubernetes/kubernetes#107065

Fixes: ceph#3176
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request Jun 24, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the volumeHealer, must receive the correct path, otherwise
the after a nodeplugin restart the NBD mounts will bailout attempting
to NodeStageVolume() call and return an error.

See-also: kubernetes/kubernetes#107065

Fixes: ceph#3176
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request Jun 24, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the volumeHealer, must receive the correct path, otherwise
the after a nodeplugin restart the NBD mounts will bailout attempting
to NodeStageVolume() call and return an error.

See-also: kubernetes/kubernetes#107065

Fixes: ceph#3176
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
mergify bot pushed a commit to ceph/ceph-csi that referenced this pull request Jun 24, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the volumeHealer, must receive the correct path, otherwise
the after a nodeplugin restart the NBD mounts will bailout attempting
to NodeStageVolume() call and return an error.

See-also: kubernetes/kubernetes#107065

Fixes: #3176
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
mergify bot pushed a commit to ceph/ceph-csi that referenced this pull request Jun 24, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the volumeHealer, must receive the correct path, otherwise
the after a nodeplugin restart the NBD mounts will bailout attempting
to NodeStageVolume() call and return an error.

See-also: kubernetes/kubernetes#107065

Fixes: #3176
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
(cherry picked from commit 1da446d)
mergify bot pushed a commit to ceph/ceph-csi that referenced this pull request Jun 24, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the volumeHealer, must receive the correct path, otherwise
the after a nodeplugin restart the NBD mounts will bailout attempting
to NodeStageVolume() call and return an error.

See-also: kubernetes/kubernetes#107065

Fixes: #3176
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
(cherry picked from commit 1da446d)
mergify bot pushed a commit to rook/rook that referenced this pull request Jun 27, 2022
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes.

The backward compatibility should be taken care by the CSI driver.

See-also: kubernetes/kubernetes#107065
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
(cherry picked from commit 2627f32)
cvvz added a commit to cvvz/blob-csi-driver that referenced this pull request Mar 30, 2023
cvvz added a commit to cvvz/blob-csi-driver that referenced this pull request Apr 4, 2023
commit d9fcdae
Merge: 6569b29 07f79d4
Author: weizhichen <weizhichen@microsoft.com>
Date:   Tue Apr 4 08:36:31 2023 +0000

    Merge branch 'master' of https://github.com/kubernetes-sigs/blob-csi-driver into e2e-test

commit 6569b29
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 3 11:37:40 2023 +0000

    parallel again

commit 9ed8a55
Merge: 551c409 a47bc07
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 3 10:27:26 2023 +0000

    Merge branch 'master' of https://github.com/kubernetes-sigs/blob-csi-driver into e2e-test

commit 551c409
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 3 10:21:04 2023 +0000

    another flaky test

commit 38e0b6a
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 3 08:46:01 2023 +0000

    fix panic

commit cce9102
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 3 07:40:40 2023 +0000

    flaky: e2e: fix pre-provisioned test

commit 3b5a6ee
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 3 03:19:55 2023 +0000

    framework init

commit 6fa9cd4
Author: weizhichen <weizhichen@microsoft.com>
Date:   Sun Apr 2 08:31:37 2023 +0000

    flake attempts

commit 7c752ae
Author: weizhichen <weizhichen@microsoft.com>
Date:   Sun Apr 2 03:29:32 2023 +0000

    cancel parallel

commit cffd14a
Author: weizhichen <weizhichen@microsoft.com>
Date:   Sun Apr 2 00:31:23 2023 +0000

    flakeattempts

commit c6a0266
Author: weizhichen <weizhichen@microsoft.com>
Date:   Sat Apr 1 20:10:30 2023 +0000

    make nfs test serial

commit 9bba102
Author: weizhichen <weizhichen@microsoft.com>
Date:   Sat Apr 1 01:46:32 2023 +0000

    make private endpoint test serial

commit fe1e3b8
Author: weizhichen <weizhichen@microsoft.com>
Date:   Sat Apr 1 00:52:47 2023 +0000

    output-interceptor-mode=none

commit c4a3a91
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Mar 31 10:05:40 2023 +0000

    no flake-attempts

commit a94726a
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Mar 31 09:59:57 2023 +0000

    gomega success

commit f9638e2
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Mar 31 09:27:02 2023 +0000

    pass project root to e2e test

commit 6b795c6
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Mar 31 07:48:30 2023 +0000

    fix

commit 2a14b30
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Mar 31 07:07:39 2023 +0000

    fix restart driver

commit 3553af8
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Mar 31 06:37:30 2023 +0000

    fix pre_provisioned_provided_credentials_tester

commit 0aa27dd
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Mar 31 06:10:13 2023 +0000

    move verify examples to ginkgo Node container

commit 7ebd065
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Mar 31 04:58:18 2023 +0000

    add flake-attempts

commit 02a48ba
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Mar 31 04:57:40 2023 +0000

    Revert "use seed to repro"

    This reverts commit 1c5fea8.

commit 1c5fea8
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 16:27:03 2023 +0000

    use seed to repro

commit c52b495
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 13:57:37 2023 +0000

    fix: container name

commit f1f55e4
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 13:23:25 2023 +0000

    fix dynamic inline volume and byok volume

commit 0e4b11e
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 12:24:47 2023 +0000

    revert --delete-namespace

commit 7df5df4
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 10:53:45 2023 +0000

    fix: set framework flags

commit 103840c
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 09:52:05 2023 +0000

    set delete-namespace=false to avoid deleting ns which is used by other
    specs during parallel testing

commit cda91f8
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 08:36:47 2023 +0000

    fix: should notify all goroutine channel by close

commit 0aa42e6
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 07:08:10 2023 +0000

    fix NodeTimeout, need context

commit 9c1fb1d
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 06:49:52 2023 +0000

    fix defer cleanup order

commit b4d44b2
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 06:43:29 2023 +0000

    add 10min GracePeriod for AfterSuite to avoid exit too quick

commit 49fdec3
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 05:22:55 2023 +0000

    workaround the issue: kubernetes/kubernetes#107065

commit 30369fb
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 05:19:35 2023 +0000

    fix restartDriverTest panic

commit 67ff546
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 05:00:44 2023 +0000

    fix: dump namespace info

commit 54a2d2f
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 04:25:58 2023 +0000

    add log after driver pod is restarted

commit fd32cab
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 03:04:35 2023 +0000

    fix

commit 7074288
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 02:37:11 2023 +0000

    adjust AccountCreationLeak check threshold

commit b86e385
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Mar 30 02:08:51 2023 +0000

    fix: reduce csi driver daemon restart times

commit 63e820f
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 29 18:35:54 2023 +0000

    fix pwd

commit 65c8c04
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 29 17:44:04 2023 +0000

    fix: no log print out after blob daemonset is recreated

commit b9754e4
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 29 15:03:27 2023 +0000

    fix

commit a9b913d
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 29 14:55:11 2023 +0000

    fix: createvolume and initialize volumeID in beforeeach

commit e6aa3a8
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 29 11:04:30 2023 +0000

    fix: set azidentity.EnvironmentCredential for each process

commit 2dd2015
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 29 09:19:49 2023 +0000

    fix init k8s client error

commit b319406
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 29 08:33:25 2023 +0000

    fix BeforeSuite and AfterSuite

commit 1496638
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 29 07:49:07 2023 +0000

    1. use e2e.test v1.26.0
    2. upgrade ginkgo to v2.9.2 to use GinkgoHelper
    3. add --fast-fail

commit d6d05ea
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 29 03:49:51 2023 +0000

    fix

commit 6e6d71f
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 29 03:22:14 2023 +0000

    test: speed up e2e test by running in parallel
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. area/test 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. 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
Development

Successfully merging this pull request may close these issues.

CSI Volume staging path contains PV name
10 participants