Skip to content

use xfs_repair to check and repair xfs filesystem in method formatAndMount #126

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

Merged
merged 2 commits into from
Jan 9, 2020
Merged

use xfs_repair to check and repair xfs filesystem in method formatAndMount #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa83e53
use xfs_repair to check and repair xfs filesystem
Dec 4, 2019
08f269a
update after review
Jan 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 69 additions & 24 deletions mount/mount_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,54 @@ func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
return SearchMountPoints(realpath, procMountInfoPath)
}

// checkAndRepairFileSystem checks and repairs filesystems using command fsck.
func (mounter *SafeFormatAndMount) checkAndRepairFilesystem(source string) error {
klog.V(4).Infof("Checking for issues with fsck on disk: %s", source)
args := []string{"-a", source}
out, err := mounter.Exec.Command("fsck", args...).CombinedOutput()
if err != nil {
ee, isExitError := err.(utilexec.ExitError)
switch {
case err == utilexec.ErrExecutableNotFound:
klog.Warningf("'fsck' not found on system; continuing mount without running 'fsck'.")
case isExitError && ee.ExitStatus() == fsckErrorsCorrected:
klog.Infof("Device %s has errors which were corrected by fsck.", source)
case isExitError && ee.ExitStatus() == fsckErrorsUncorrected:
return NewMountError(HasFilesystemErrors, "'fsck' found errors on device %s but could not correct them: %s", source, string(out))
case isExitError && ee.ExitStatus() > fsckErrorsUncorrected:
klog.Infof("`fsck` error %s", string(out))
}
}
return nil
}

// checkAndRepairXfsFilesystem checks and repairs xfs filesystem using command xfs_repair.
func (mounter *SafeFormatAndMount) checkAndRepairXfsFilesystem(source string) error {
klog.V(4).Infof("Checking for issues with xfs_repair on disk: %s", source)

args := []string{source}
checkArgs := []string{"-n", source}

// check-only using "xfs_repair -n", if the exit status is not 0, perform a "xfs_repair"
_, err := mounter.Exec.Command("xfs_repair", checkArgs...).CombinedOutput()
if err != nil {
if err == utilexec.ErrExecutableNotFound {
klog.Warningf("'xfs_repair' not found on system; continuing mount without running 'xfs_repair'.")
return nil
} else {
klog.Warningf("Filesystem corruption was detected for %s, running xfs_repair to repair", source)
out, err := mounter.Exec.Command("xfs_repair", args...).CombinedOutput()
if err != nil {
return NewMountError(HasFilesystemErrors, "'xfs_repair' found errors on device %s but could not correct them: %s\n", source, out)
} else {
klog.Infof("Device %s has errors which were corrected by xfs_repair.", source)
return nil
}
}
}
return nil
}

// formatAndMount uses unix utils to format and mount the given disk
func (mounter *SafeFormatAndMount) formatAndMount(source string, target string, fstype string, options []string) error {
readOnly := false
Expand All @@ -269,26 +317,6 @@ func (mounter *SafeFormatAndMount) formatAndMount(source string, target string,
options = append(options, "defaults")
var mountErrorValue MountErrorType

if !readOnly {
// Run fsck on the disk to fix repairable issues, only do this for volumes requested as rw.
klog.V(4).Infof("Checking for issues with fsck on disk: %s", source)
args := []string{"-a", source}
out, err := mounter.Exec.Command("fsck", args...).CombinedOutput()
if err != nil {
ee, isExitError := err.(utilexec.ExitError)
switch {
case err == utilexec.ErrExecutableNotFound:
klog.Warningf("'fsck' not found on system; continuing mount without running 'fsck'.")
case isExitError && ee.ExitStatus() == fsckErrorsCorrected:
klog.Infof("Device %s has errors which were corrected by fsck.", source)
case isExitError && ee.ExitStatus() == fsckErrorsUncorrected:
return NewMountError(HasFilesystemErrors, "'fsck' found errors on device %s but could not correct them: %s", source, string(out))
case isExitError && ee.ExitStatus() > fsckErrorsUncorrected:
klog.Infof("`fsck` error %s", string(out))
}
}
}

// Check if the disk is already formatted
existingFormat, err := mounter.GetDiskFormat(source)
if err != nil {
Expand Down Expand Up @@ -324,10 +352,27 @@ func (mounter *SafeFormatAndMount) formatAndMount(source string, target string,
}

klog.Infof("Disk successfully formatted (mkfs): %s - %s %s", fstype, source, target)
} else if fstype != existingFormat {
// Verify that the disk is formatted with filesystem type we are expecting
mountErrorValue = FilesystemMismatch
klog.Warningf("Configured to mount disk %s as %s but current format is %s, things might break", source, existingFormat, fstype)
} else {
if fstype != existingFormat {
// Verify that the disk is formatted with filesystem type we are expecting
mountErrorValue = FilesystemMismatch
klog.Warningf("Configured to mount disk %s as %s but current format is %s, things might break", source, existingFormat, fstype)
}

if !readOnly {
// Run check tools on the disk to fix repairable issues, only do this for formatted volumes requested as rw.
var err error
switch existingFormat {
case "xfs":
err = mounter.checkAndRepairXfsFilesystem(source)
default:
err = mounter.checkAndRepairFilesystem(source)
}

if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

Rebase is not that simple in this case: you should return NewMountError(mountErrorValue, err.Error()), to propagate potential FilesystemMismatch.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, you should return FilesystemMismatch, if the FS type is wrong, or HasFilesystemErrors if the FS type is OK, but the filesystem has errors.

Copy link
Member Author

@27149chen 27149chen Jan 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

  1. HasFilesystemErrors is retruned in checkAndRepairXfsFilesystem/checkAndRepairFilesystem, so it is ok to return err here directly.
  2. I don't think we should return a FilesystemMismatch here, because the only error which will happen here is a failed_to_repair_filesystem_error, and the fs passed in is the existing fs, not the defined one, so I think there is no filesystem mismatch which can cause this error.
  3. I forgot to return a HasFilesystemErrors in checkAndRepairXfsFilesystem, which is fixed now. @jsafrane PTAL.

Copy link
Member

Choose a reason for hiding this comment

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

ack, I missed checkAndRepairXfsFilesystem/checkAndRepairFilesystem

}
}
}

// Mount the disk
Expand Down
59 changes: 46 additions & 13 deletions mount/safe_format_and_mount_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ func TestSafeFormatAndMount(t *testing.T) {
description: "Test a normal mount of an already formatted device",
fstype: "ext4",
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=ext4\n", nil},
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
},
},
{
Expand All @@ -96,7 +96,6 @@ func TestSafeFormatAndMount(t *testing.T) {
description: "Test a normal mount of unformatted device",
fstype: "ext4",
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 2}},
{"mkfs.ext4", []string{"-F", "-m0", "/dev/foo"}, "", nil},
},
Expand All @@ -105,34 +104,34 @@ func TestSafeFormatAndMount(t *testing.T) {
description: "Test 'fsck' fails with exit status 4",
fstype: "ext4",
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 4}},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=ext4\n", nil},
{"fsck", []string{"-a", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 4}},
},
expectedError: fmt.Errorf("'fsck' found errors on device /dev/foo but could not correct them"),
},
{
description: "Test 'fsck' fails with exit status 1 (errors found and corrected)",
fstype: "ext4",
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 1}},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=ext4\n", nil},
{"fsck", []string{"-a", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 1}},
},
},
{
description: "Test 'fsck' fails with exit status other than 1 and 4 (likely unformatted device)",
fstype: "ext4",
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 8}},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=ext4\n", nil},
{"fsck", []string{"-a", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 8}},
},
},
{
description: "Test that 'blkid' is called and fails",
fstype: "ext4",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'")},
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nPTTYPE=dos\n", nil},
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
},
expectedError: fmt.Errorf("unknown filesystem type '(null)'"),
},
Expand All @@ -141,7 +140,6 @@ func TestSafeFormatAndMount(t *testing.T) {
fstype: "ext4",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'")},
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 2}},
{"mkfs.ext4", []string{"-F", "-m0", "/dev/foo"}, "", fmt.Errorf("formatting failed")},
},
Expand All @@ -152,7 +150,6 @@ func TestSafeFormatAndMount(t *testing.T) {
fstype: "ext4",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'")},
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 2}},
{"mkfs.ext4", []string{"-F", "-m0", "/dev/foo"}, "", nil},
},
Expand All @@ -162,7 +159,6 @@ func TestSafeFormatAndMount(t *testing.T) {
description: "Test that 'blkid' is called and confirms unformatted disk, format passes, mount passes",
fstype: "ext4",
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 2}},
{"mkfs.ext4", []string{"-F", "-m0", "/dev/foo"}, "", nil},
},
Expand All @@ -172,7 +168,6 @@ func TestSafeFormatAndMount(t *testing.T) {
description: "Test that 'blkid' is called and confirms unformatted disk, format passes, mount passes with ext3",
fstype: "ext3",
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 2}},
{"mkfs.ext3", []string{"-F", "-m0", "/dev/foo"}, "", nil},
},
Expand All @@ -182,7 +177,6 @@ func TestSafeFormatAndMount(t *testing.T) {
description: "test that none ext4 fs does not get called with ext4 options.",
fstype: "xfs",
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 2}},
{"mkfs.xfs", []string{"/dev/foo"}, "", nil},
},
Expand All @@ -192,21 +186,60 @@ func TestSafeFormatAndMount(t *testing.T) {
description: "Test that 'blkid' is called and reports ext4 partition",
fstype: "ext4",
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=ext4\n", nil},
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
},
},
{
description: "Test that 'blkid' is called but has some usage or other errors (an exit code of 4 is returned)",
fstype: "xfs",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'"), nil},
execScripts: []ExecArgs{
{"fsck", []string{"-a", "/dev/foo"}, "", nil},
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 4}},
{"mkfs.xfs", []string{"/dev/foo"}, "", nil},
},
expectedError: fmt.Errorf("exit 4"),
},
{
description: "Test that 'xfs_repair' is called only once, no need to repair the filesystem",
fstype: "xfs",
execScripts: []ExecArgs{
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=xfs\n", nil},
{"xfs_repair", []string{"-n", "/dev/foo"}, "", nil},
},
expectedError: nil,
},
{
description: "Test that 'xfs_repair' is called twice and repair the filesystem",
fstype: "xfs",
execScripts: []ExecArgs{
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=xfs\n", nil},
{"xfs_repair", []string{"-n", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 1}},
{"xfs_repair", []string{"/dev/foo"}, "\ndone\n", nil},
},
expectedError: nil,
},
{
description: "Test that 'xfs_repair' is called twice and repair the filesystem, but mount failed",
fstype: "xfs",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'")},
execScripts: []ExecArgs{
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=xfs\n", nil},
{"xfs_repair", []string{"-n", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 1}},
{"xfs_repair", []string{"/dev/foo"}, "\ndone\n", nil},
},
expectedError: fmt.Errorf("unknown filesystem type '(null)'"),
},
{
description: "Test that 'xfs_repair' is called twice but could not repair the filesystem",
fstype: "xfs",
execScripts: []ExecArgs{
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=xfs\n", nil},
{"xfs_repair", []string{"-n", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 1}},
{"xfs_repair", []string{"/dev/foo"}, "\nAn error occurred\n", &testingexec.FakeExitError{Status: 1}},
},
expectedError: fmt.Errorf("'xfs_repair' found errors on device %s but could not correct them: %v", "/dev/foo", "\nAn error occurred\n"),
},
}

for _, test := range tests {
Expand Down