do not remove volume dir when saveVolumeData fails #89464

fankangbest · 2020-03-25T06:27:28Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
If saveVolumeData() fails，all data in the volume dir is removed. This will happen when the disk is readonly and the volume is a remote disk.

    if err := saveVolumeData(dataDir, volDataFileName, volData); err != nil {
		if removeErr := os.RemoveAll(dataDir); removeErr != nil {
			klog.Error(log("failed to remove dir after error [%s]: %v", dataDir, removeErr))
		}
		return nil, errors.New(log("failed to save volume info data: %v", err))
	}

There is no need to remove all in volume dir. When saveVolumeData fails, just log an error is enough because kubelet will retry to write volDataFileName next time.

Which issue(s) this PR fixes:

Fixes #89281

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

k8s-ci-robot · 2020-03-25T06:27:37Z

Hi @fankangbest. 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.

fankangbest · 2020-03-25T06:30:11Z

@answer1991

gnufied · 2020-04-13T15:32:38Z

/ok-to-test

gnufied · 2020-04-13T15:47:47Z

@fankangbest thanks for the PR. I think this requires more careful fix because the fix proposed here will leave orphan pod directories when mount fails. See - #72346 for more details.

msau42 · 2020-04-13T17:29:06Z

/assign @gnufied
@kubernetes/sig-storage-pr-reviews

gnufied · 2020-06-01T16:50:58Z

pkg/volume/csi/csi_plugin.go

-		return nil, errors.New(log("failed to save volume info data: %v", err))
+		errMsg := log("failed to save volume info data: %v", err)
+		klog.Error(errMsg)
+		return nil, errors.New(errMsg)


sorry for taking time on this but rather than dropping this check altogether can we simple call the implementation of removeMountDir which should be relatively safe because it only removes data file and then removes with directory without ever calling os.RemoveAll ?

RemoveAll may delete data in the mountpoint, and it is unexpected. I think it is safe to call removeMountDir here because removeMountDir won't delete data.

Yes - can you update this PR to use removeMountDir ?

Sure, I will update the code and do a further test.

gnufied · 2020-06-01T16:51:05Z

pkg/volume/csi/csi_attacher.go

-		return err
+		errMsg := log("failed to save volume info data: %v", err)
+		klog.Error(errMsg)
+		return errors.New(errMsg)


similar comment as below

removeMountDir() will be called in defer func here.

correct me if I am wrong but the defer function is not installed yet and hence will not be called when the function returns from here.

@fankangbest Can you please address this comment? I am happy to have unit test merged later on but we need this fixed before we can merge this.

k8s-ci-robot · 2020-06-05T09:14:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fankangbest
To complete the pull request process, please assign gnufied
You can assign the PR to them by writing /assign @gnufied in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/volume/csi/OWNERS

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 · 2020-06-09T03:50:39Z

@fankangbest: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kind-ipv6	`f8c5159`	link	`/test pull-kubernetes-e2e-kind-ipv6`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

gnufied · 2020-06-10T20:59:22Z

@fankangbest can we also add some unit tests that exercise the scenario?

fejta-bot · 2020-09-20T17:07:12Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2020-10-20T17:49:57Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2020-11-27T20:43:16Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-11-27T20:43:30Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 25, 2020

k8s-ci-robot requested review from humblec and verult March 25, 2020 06:28

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 13, 2020

k8s-ci-robot assigned gnufied Apr 13, 2020

gnufied reviewed Jun 1, 2020

View reviewed changes

fankangbest force-pushed the myk8s/fix-csi-removeall branch from dec810f to ecd7f96 Compare June 5, 2020 09:13

fankangbest force-pushed the myk8s/fix-csi-removeall branch from ecd7f96 to ddda214 Compare June 9, 2020 03:14

do not remove volume dir when saveVolumeData fails

f8c5159

fankangbest force-pushed the myk8s/fix-csi-removeall branch from ddda214 to f8c5159 Compare June 9, 2020 03:18

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 20, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 20, 2020

huffmanca mentioned this pull request Oct 29, 2020

Dont remove volumes when saveVolumeData fails #96021

Merged

k8s-ci-robot closed this Nov 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

do not remove volume dir when saveVolumeData fails #89464

do not remove volume dir when saveVolumeData fails #89464

fankangbest commented Mar 25, 2020 •

edited

k8s-ci-robot commented Mar 25, 2020

fankangbest commented Mar 25, 2020

gnufied commented Apr 13, 2020

gnufied commented Apr 13, 2020

msau42 commented Apr 13, 2020

gnufied Jun 1, 2020

fankangbest Jun 4, 2020

gnufied Jun 4, 2020

fankangbest Jun 5, 2020

gnufied Jun 1, 2020

fankangbest Jun 9, 2020

gnufied Jun 10, 2020

gnufied Jun 22, 2020

k8s-ci-robot commented Jun 5, 2020

k8s-ci-robot commented Jun 9, 2020

gnufied commented Jun 10, 2020

fejta-bot commented Sep 20, 2020

fejta-bot commented Oct 20, 2020

fejta-bot commented Nov 27, 2020

k8s-ci-robot commented Nov 27, 2020

do not remove volume dir when saveVolumeData fails #89464

do not remove volume dir when saveVolumeData fails #89464

Conversation

fankangbest commented Mar 25, 2020 • edited

k8s-ci-robot commented Mar 25, 2020

fankangbest commented Mar 25, 2020

gnufied commented Apr 13, 2020

gnufied commented Apr 13, 2020

msau42 commented Apr 13, 2020

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-ci-robot commented Jun 5, 2020

k8s-ci-robot commented Jun 9, 2020

gnufied commented Jun 10, 2020

fejta-bot commented Sep 20, 2020

fejta-bot commented Oct 20, 2020

fejta-bot commented Nov 27, 2020

k8s-ci-robot commented Nov 27, 2020

fankangbest commented Mar 25, 2020 •

edited