Fix FibreChannel volume plugin corrupting filesystem on detach #97013

jsafrane · 2020-12-02T13:19:24Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
FibreChannel volume plugin misses one important step when removing a device: multipath -f, that flushes all multipath buffers to its individual paths. Without it, a filesystem on the device may get corrupted.

Calling multipath -f is easy, however, the patch grew a bit by passing all the necessary interfaces down to fc_util.go. The structure copies iscsi_util.go

See https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/storage_administration_guide/removing_devices for details.

Which issue(s) this PR fixes:

Fixes #97014

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixed FibreChannel volume plugin corrupting filesystems on detach of multipath volumes.

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

k8s-ci-robot · 2020-12-02T13:19:31Z

@jsafrane: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

jsafrane · 2020-12-02T13:35:38Z

/priority important-soon

FibreChannel volume plugin misses one important step when removing a device: "multipath -f". It flushes all multipath buffers to its individual paths. Without it, a filesystem on the device may get corrupted.

jingxu97 · 2020-12-02T17:14:33Z

pkg/volume/fc/fc_util.go

+func (util *fcUtil) deleteMultipathDevice(exec utilexec.Interface, dmDevice string) error {
+	out, err := exec.Command("multipath", "-f", dmDevice).CombinedOutput()
+	if err != nil {
+		return fmt.Errorf("failed to flush multipath device %s: %s\n%s", dmDevice, err, string(out))


just want to confirm whether in certain cases, it should continue instead of return error (e.g., the error means device no longer exist?)

Good catch! That's why iscsi volume plugin ignores the error code...

If the device is missing:

Filesystem volumes: operation_generator.go will stop with "The path isn't device path or doesn't exist. Skip checking device path: /dev/dm-0" and it won't call volume plugin at all. Therefore, if the multipath device is removed, but the individual paths are still there, nothing will remove them.

Block volumes: if the device is missing, the next DetachBlockFCDisk will stop in checkPathExists, if not earlier in operation_generator.go (did not test it).

kubernetes/pkg/volume/fc/fc_util.go

Line 301 in 6f8bdb7

if pathExists, pathErr := checkPathExists(devicePath); !pathExists || pathErr != nil {

iscsi volume plugin actually ignores the error code from "multipath -f", continuing deleting the device paths. Which may result in volume corruption, if the multipath device still exists.. Now is the question what is actually correct - try to remove the individual paths to fully detach the devices and risk corruption OR do not remove the paths at all? The paths should just sit in /dev, not used by anything.

My gut feeling is to copy iscsi approach and ignore the exit code.

according to manpage
-f Flush (remove) a multipath device map specified as parameter, if unused.

So if multipath -f fails, there is a good chance the device is still being used and removing it could cause data corruption. In this sense, the implementation in iscsi is indeed risky.

rootfs · 2020-12-02T18:46:55Z

/approve

rootfs · 2020-12-02T18:49:22Z

pkg/volume/fc/fc_util.go

+func (util *fcUtil) deleteMultipathDevice(exec utilexec.Interface, dmDevice string) error {
+	out, err := exec.Command("multipath", "-f", dmDevice).CombinedOutput()
+	if err != nil {
+		return fmt.Errorf("failed to flush multipath device %s: %s\n%s", dmDevice, err, string(out))


nit:

return fmt.Errorf("failed to flush multipath device %s: %v\n%s", dmDevice, err, string(out))

No, %s is fine for errror type.

k8s-ci-robot · 2020-12-02T18:49:48Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/fc/OWNERS~~ [jsafrane,rootfs]

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

gnufied · 2020-12-02T20:07:57Z

pkg/volume/fc/fc_util.go

@@ -354,6 +358,9 @@ func (util *fcUtil) DetachBlockFCDisk(c fcDiskUnmapper, mapPath, devicePath stri
 	if len(dm) != 0 {


Same link - https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/storage_administration_guide/removing_devices#:~:text=Run%20multipath%20%2Dl%20command%20to,to%20remove%20the%20multipath%20device. recommends that for raw block devices we should call blockdev --flushbufs device before removing the device. Should we do that as well?

Added as a separate commit.
BTW, we should refactor FC and iSCSI to use the same flush / device deletion. But that's for a separate PR.

yeah lets file that as a separate issue I think.

tsmetana · 2020-12-03T07:42:18Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

If a FibreChannel device is used as a block volume, we should flush its I/O before deleting its device. It is not strictly necessary when it's used as a filesystem (mount), but it won't hurt either.

gnufied · 2020-12-03T16:26:22Z

/retest
/lgtm

fejta-bot · 2020-12-03T20:49:30Z

/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 or /hold comment for consistent failures.

…13-upstream-release-1.19 Automated cherry pick of #97013: Fix FibreChannel volume plugin corrupting filesystem on

…13-upstream-release-1.18 Automated cherry pick of #97013: Fix FibreChannel volume plugin corrupting filesystem on

…13-upstream-release-1.20 Automated cherry pick of #97013: Fix FibreChannel volume plugin corrupting filesystem on

k8s-ci-robot requested review from jingxu97 and msau42 December 2, 2020 13:20

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 2, 2020

Fix FibreChannel volume plugin corrupting filesystem on detach

6f8bdb7

FibreChannel volume plugin misses one important step when removing a device: "multipath -f". It flushes all multipath buffers to its individual paths. Without it, a filesystem on the device may get corrupted.

jsafrane force-pushed the fc-fixcorrupted-fs branch from 58f366e to 6f8bdb7 Compare December 2, 2020 13:39

jingxu97 reviewed Dec 2, 2020

View reviewed changes

jsafrane mentioned this pull request Dec 2, 2020

Bug 1903524: [3.11] UPSTREAM: 97013: Fix FibreChannel volume plugin corrupting filesystem on detach openshift/origin#25733

Merged

rootfs reviewed Dec 2, 2020

View reviewed changes

gnufied reviewed Dec 2, 2020

View reviewed changes

Flush FibreChannel devices before deleting

8f186f9

If a FibreChannel device is used as a block volume, we should flush its I/O before deleting its device. It is not strictly necessary when it's used as a filesystem (mount), but it won't hurt either.

k8s-ci-robot assigned gnufied Dec 3, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 3, 2020

k8s-ci-robot merged commit f8db0d9 into kubernetes:master Dec 9, 2020

k8s-ci-robot added this to the v1.21 milestone Dec 9, 2020

k8s-ci-robot added a commit that referenced this pull request Dec 15, 2020

Merge pull request #97198 from jsafrane/automated-cherry-pick-of-#970…

7f39880

…13-upstream-release-1.19 Automated cherry pick of #97013: Fix FibreChannel volume plugin corrupting filesystem on

k8s-ci-robot added a commit that referenced this pull request Dec 15, 2020

Merge pull request #97204 from jsafrane/automated-cherry-pick-of-#970…

e33abf2

…13-upstream-release-1.18 Automated cherry pick of #97013: Fix FibreChannel volume plugin corrupting filesystem on

k8s-ci-robot added a commit that referenced this pull request Dec 15, 2020

Merge pull request #97197 from jsafrane/automated-cherry-pick-of-#970…

2e6adaa

…13-upstream-release-1.20 Automated cherry pick of #97013: Fix FibreChannel volume plugin corrupting filesystem on

jsafrane mentioned this pull request Dec 15, 2020

Bug 1903346: UPSTREAM: 97013: Fix FibreChannel volume plugin corrupting filesystem on detach openshift/kubernetes#489

Merged

github-actions bot mentioned this pull request Dec 16, 2020

Week Ending December 13, 2020 dev-obs/actus#308

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix FibreChannel volume plugin corrupting filesystem on detach #97013

Fix FibreChannel volume plugin corrupting filesystem on detach #97013

jsafrane commented Dec 2, 2020 •

edited

k8s-ci-robot commented Dec 2, 2020

jsafrane commented Dec 2, 2020

jingxu97 Dec 2, 2020

jsafrane Dec 2, 2020

jsafrane Dec 2, 2020

rootfs Dec 2, 2020

rootfs commented Dec 2, 2020

rootfs Dec 2, 2020

jsafrane Dec 2, 2020

k8s-ci-robot commented Dec 2, 2020

gnufied Dec 2, 2020

jsafrane Dec 3, 2020

gnufied Dec 3, 2020

tsmetana commented Dec 3, 2020

gnufied commented Dec 3, 2020

fejta-bot commented Dec 3, 2020

		@@ -354,6 +358,9 @@ func (util *fcUtil) DetachBlockFCDisk(c fcDiskUnmapper, mapPath, devicePath stri
		if len(dm) != 0 {

Fix FibreChannel volume plugin corrupting filesystem on detach #97013

Fix FibreChannel volume plugin corrupting filesystem on detach #97013

Conversation

jsafrane commented Dec 2, 2020 • edited

k8s-ci-robot commented Dec 2, 2020

jsafrane commented Dec 2, 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

rootfs commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmetana commented Dec 3, 2020

gnufied commented Dec 3, 2020

fejta-bot commented Dec 3, 2020

jsafrane commented Dec 2, 2020 •

edited