Fix UnmountDevice with deleted pod. #64882

jsafrane · 2018-06-07T15:07:23Z

When a pod is deleted, kubelet can't read VolumeAttachment objects. It should cache all information in a json file.

~~Work in progress: missing (unit?) tests~~

Release note:

NONE

@saad-ali @vladimirvivien @sbezverk
/sig storage

When a pod is deleted, kubelet can't read VolumeAttachment objects. It should cache all information in a json file.

jsafrane · 2018-06-08T11:12:21Z

Unit tests added. Previous unit tests were not exactly running, because of return in this block:

kubernetes/pkg/volume/csi/csi_attacher_test.go

Lines 686 to 692 in a7a081b

    
           // Verify 
        
           if err != nil { 
        
           	if !tc.shouldFail { 
        
           		t.Errorf("test should not fail, but error occurred: %v", err) 
        
           	} 
        
           	return 
        
           }

I.e. the tests stopped after the first tc.shouldFail test. I made the tests working.

jsafrane · 2018-06-15T17:16:10Z

/assign @saad-ali @vladimirvivien

vladimirvivien · 2018-06-18T16:12:58Z

/LGTM

k8s-ci-robot · 2018-06-18T16:13:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, vladimirvivien

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:

~~pkg/volume/csi/OWNERS~~ [jsafrane,vladimirvivien]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2018-06-18T19:13:07Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

jsafrane · 2018-06-19T12:07:16Z

I'll pick it into 1.10 and 1.11 when it gets merged (tomorrow?)

saad-ali

A few questions, otherwise lgtm

@vladimirvivien can you please review this from the block device perspective.

/assign @vladimirvivien

CC @davidz627 who authored the DeviceMount code for CSI

saad-ali · 2018-06-20T07:23:01Z

pkg/volume/csi/csi_attacher.go

@@ -269,6 +269,25 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo
 		return err
 	}

+	dataDir := filepath.Dir(deviceMountPath)


Won't this end up putting the file in the same dir that is mounted?
On the mount side, it saves the file in /blah/savefile.txt and puts the mount in /blah/mount/

No, that's why there is filepath.Dir. It ends up in /var/lib/kubelet/plugins/kubernetes.io/csi/pv/pvc-938861e4756911e8/vol_data.json and the volume is mounted into /var/lib/kubelet/plugins/kubernetes.io/csi/pv/pvc-938861e4756911e8/globalmount.

saad-ali · 2018-06-20T07:24:32Z

pkg/volume/csi/csi_attacher.go

+
+	data := map[string]string{
+		volDataKey.volHandle:  csiSource.VolumeHandle,
+		volDataKey.driverName: csiSource.Driver,


The mount side saves a lot more:

volDataKey.specVolID: spec.Name(), volDataKey.volHandle: pvSource.VolumeHandle, volDataKey.driverName: pvSource.Driver, volDataKey.nodeName: node, volDataKey.attachmentID: attachID,

Are we sure we don't need anything else from the spec?

No, UnstageVolume needs just volume id (+ driver).

@saad-ali the mount side saves more info because it's needed to reconstruct the VolumeAttachment ID in subsequent steps.

saad-ali · 2018-06-20T07:26:20Z

pkg/volume/csi/csi_attacher.go

+		volDataKey.volHandle:  csiSource.VolumeHandle,
+		volDataKey.driverName: csiSource.Driver,
+	}
+	if err := saveVolumeData(dataDir, volDataFileName, data); err != nil {


@vladimirvivien: you recently moved saveVolumeData(...) from SetupAt(...) to NewMounter(...). Does something similar need to happen here?

@saad-ali unfortunately, the way the Attacher API is written, no params is available during csi_plugin.go#NewAttacher/#NewDetacher that would allow the volume info to be persisted at that point as is done wit NewMounter/Unmounter. So, the saving of the volume info has to happen during MountDevice call.

saad-ali · 2018-06-20T07:27:55Z

pkg/volume/csi/csi_attacher.go

+		}
+		return err
+	}
+


If the MountDevice(...) operation fails below, how is the save file cleaned up?

If MounDevice fails, then the whole directory is cleaned by removeMountDir. But you raised valid point, there are other error paths (e.g. failed to load secrets) that lead to uncleaned json file. I'll fix it.

Filled #65323 + PR

k8s-github-robot · 2018-06-20T07:30:57Z

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@jsafrane @saad-ali @vladimirvivien

Action required: This pull request requires label changes. If the required changes are not made within 3 days, the pull request will be moved out of the v1.11 milestone.

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

jsafrane · 2018-06-20T13:25:15Z

/retest

k8s-github-robot · 2018-06-20T15:32:07Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-06-20T16:59:57Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

vladimirvivien

Saad's dir clean up concerns should be addressed.

vladimirvivien · 2018-06-20T21:40:03Z

pkg/volume/csi/csi_attacher.go

+		volDataKey.volHandle:  csiSource.VolumeHandle,
+		volDataKey.driverName: csiSource.Driver,
+	}
+	if err := saveVolumeData(dataDir, volDataFileName, data); err != nil {


@saad-ali unfortunately, the way the Attacher API is written, no params is available during csi_plugin.go#NewAttacher/#NewDetacher that would allow the volume info to be persisted at that point as is done wit NewMounter/Unmounter. So, the saving of the volume info has to happen during MountDevice call.

vladimirvivien · 2018-06-20T21:41:02Z

pkg/volume/csi/csi_attacher.go

+
+	data := map[string]string{
+		volDataKey.volHandle:  csiSource.VolumeHandle,
+		volDataKey.driverName: csiSource.Driver,


@saad-ali the mount side saves more info because it's needed to reconstruct the VolumeAttachment ID in subsequent steps.

#65323-upstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #64882: Fix UnmountDevice with deleted pod. #65323: Fix cleanup of volume metadata json file. Cherry pick of #64882 #65323 on release-1.11. #64882: Fix UnmountDevice with deleted pod. #65323: Fix cleanup of volume metadata json file. ```release-note Fixed cleanup of CSI metadata files. ```

#65323-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #64882: Fix UnmountDevice with deleted pod. #65323: Fix cleanup of volume metadata json file. Cherry pick of #64882 #65323 on release-1.10. #64882: Fix UnmountDevice with deleted pod. #65323: Fix cleanup of volume metadata json file. ```release-note Fixed cleanup of CSI metadata files. ```

Fix UnmountDevice with deleted pod.

acad9d8

When a pod is deleted, kubelet can't read VolumeAttachment objects. It should cache all information in a json file.

k8s-ci-robot requested review from jingxu97 and verult June 7, 2018 15:07

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2018

jsafrane mentioned this pull request Jun 8, 2018

NodeAuthorizer prevents unmounting of CSI volumes when terminationGracePeriod is 0 #63827

Closed

jsafrane force-pushed the csi-mount-json branch from 154d058 to c902c68 Compare June 8, 2018 11:09

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 Jun 8, 2018

jsafrane force-pushed the csi-mount-json branch from c902c68 to acad9d8 Compare June 8, 2018 11:10

jsafrane changed the title ~~WIP: Fix UnmountDevice with deleted pod.~~ Fix UnmountDevice with deleted pod. Jun 8, 2018

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 Jun 8, 2018

k8s-ci-robot assigned saad-ali and vladimirvivien Jun 15, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2018

saad-ali reviewed Jun 20, 2018

View reviewed changes

saad-ali added this to the v1.12 milestone Jun 20, 2018

saad-ali added the status/approved-for-milestone label Jun 20, 2018

saad-ali modified the milestones: v1.12, v1.11 Jun 20, 2018

k8s-github-robot added the milestone/incomplete-labels label Jun 20, 2018

k8s-github-robot merged commit 92dfcfc into kubernetes:master Jun 20, 2018

vladimirvivien reviewed Jun 20, 2018

View reviewed changes

cvvz mentioned this pull request Mar 8, 2023

fix: the volume is not detached after the pod and PVC objects are deleted #116138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix UnmountDevice with deleted pod. #64882

Fix UnmountDevice with deleted pod. #64882

jsafrane commented Jun 7, 2018 •

edited

Loading

jsafrane commented Jun 8, 2018

jsafrane commented Jun 15, 2018

vladimirvivien commented Jun 18, 2018

k8s-ci-robot commented Jun 18, 2018

fejta-bot commented Jun 18, 2018

jsafrane commented Jun 19, 2018

saad-ali left a comment

saad-ali Jun 20, 2018

jsafrane Jun 21, 2018

saad-ali Jun 20, 2018

jsafrane Jun 20, 2018

vladimirvivien Jun 20, 2018

saad-ali Jun 20, 2018

vladimirvivien Jun 20, 2018

saad-ali Jun 20, 2018

jsafrane Jun 21, 2018

jsafrane Jun 21, 2018

k8s-github-robot commented Jun 20, 2018

jsafrane commented Jun 20, 2018

k8s-github-robot commented Jun 20, 2018

k8s-github-robot commented Jun 20, 2018

vladimirvivien left a comment

vladimirvivien Jun 20, 2018

vladimirvivien Jun 20, 2018

Fix UnmountDevice with deleted pod. #64882

Fix UnmountDevice with deleted pod. #64882

Conversation

jsafrane commented Jun 7, 2018 • edited Loading

jsafrane commented Jun 8, 2018

jsafrane commented Jun 15, 2018

vladimirvivien commented Jun 18, 2018

k8s-ci-robot commented Jun 18, 2018

fejta-bot commented Jun 18, 2018

jsafrane commented Jun 19, 2018

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Jun 20, 2018

jsafrane commented Jun 20, 2018

k8s-github-robot commented Jun 20, 2018

k8s-github-robot commented Jun 20, 2018

vladimirvivien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Jun 7, 2018 •

edited

Loading