Do not perform xfs_repair on xfs filesystem #150
Conversation
k8s-ci-robot
added
cncf-cla: yes
| err = mounter.checkAndRepairFilesystem(source) | ||
| } | ||
|
|
||
| err := mounter.checkAndRepairFilesystem(source) |
There was a problem hiding this comment.
code before change:
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))
}
}
}
and hence should be same as before.
|
/assign |
| err = mounter.checkAndRepairFilesystem(source) | ||
| } | ||
|
|
||
| err := mounter.checkAndRepairFilesystem(source) |
There was a problem hiding this comment.
The code that is part of checkAndRepairFilesystem(...) used to be called before // Check if the disk is already formatted. As part of #126 it was moved down here. Should we revert this change as well or is it necessary for some reason?
There was a problem hiding this comment.
We could revert that change too but I figured it is harmless and helps to keep diff of this PR small. We will have to fix all the breaking tests because order in which fsck gets called will change.
Also it is perfectly fine to not run fsck on a unformatted disk.
There was a problem hiding this comment.
Ack. Sounds good to me. @msau42 @xing-yang @jsafrane any objections?
There was a problem hiding this comment.
lgtm. The bug report had the error past the fs format check, so it's not the fs format check that's failing.
There was a problem hiding this comment.
The bug report had the error past the fs format check, so it's not the fs format check that's failing.
@gnufied can you confirm?
There was a problem hiding this comment.
which bug report? #141 or #125 ? I assume #141 - yes the problem isn't format checking but problem was that repairing the disk actually prevented volume from being mounted and recovery (although in some cases XFS volume can't recover itself and requires xfs_repair).
So, the only valid reason I did not do a full revert is because I liked not having to do a fsck if disk is unformatted.
|
/approve @xing-yang @saad-ali (others!) feel free to remove hold when this is ready |
k8s-ci-robot
added
do-not-merge/hold
|
/lgtm |
|
/hold cancel |
k8s-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, gnufied, saad-ali 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:
Approvers can indicate their approval by writing |
This partly reverts #126 but still we perform filesystem repair just before mounting.
fixes #141