Skip to content
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.

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

don't use ext4 mkfs options for other fs #16536

Merged
merged 1 commit into from
Nov 5, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkg/util/mount/mount.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,15 @@ func (mounter *SafeFormatAndMount) formatAndMount(source string, target string,
// It is possible that this disk is not formatted. Double check using diskLooksUnformatted
notFormatted, err := mounter.diskLooksUnformatted(source)
if err == nil && notFormatted {
args := []string{source}
// Disk is unformatted so format it.
// Use 'ext4' as the default
if len(fstype) == 0 {
fstype = "ext4"
}
args := []string{"-E", "lazy_itable_init=0,lazy_journal_init=0", "-F", source}
if fstype == "ext4" || fstype == "ext3" {
args = []string{"-E", "lazy_itable_init=0,lazy_journal_init=0", "-F", source}
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this all go in a *_linux .go file?

Copy link

Choose a reason for hiding this comment

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

shouldn't this all go in a *_linux .go file?

Ah yes they should. I filed #16729. Would you be okay with me moving them after this PR and #14200 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds a plan for me, @thockin ?

}
cmd := mounter.Runner.Command("mkfs."+fstype, args...)
_, err := cmd.CombinedOutput()
if err == nil {
Expand Down
20 changes: 15 additions & 5 deletions pkg/util/mount/safe_format_and_mount_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,15 @@ func TestSafeFormatAndMount(t *testing.T) {
fstype: "ext4",
},

{ // Test that 'file' is called and fails
{ // Test that 'lsblk' is called and fails
fstype: "ext4",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'")},
execScripts: []ExecArgs{
{"lsblk", []string{"-nd", "-o", "FSTYPE", "/dev/foo"}, "ext4", nil},
},
expectedError: fmt.Errorf("unknown filesystem type '(null)'"),
},
{ // Test that 'file' is called and confirms unformatted disk, format fails
{ // Test that 'lsblk' is called and confirms unformatted disk, format fails
fstype: "ext4",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'")},
execScripts: []ExecArgs{
Expand All @@ -78,7 +78,7 @@ func TestSafeFormatAndMount(t *testing.T) {
},
expectedError: fmt.Errorf("formatting failed"),
},
{ // Test that 'file' is called and confirms unformatted disk, format passes, second mount fails
{ // Test that 'lsblk' is called and confirms unformatted disk, format passes, second mount fails
fstype: "ext4",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'"), fmt.Errorf("Still cannot mount")},
execScripts: []ExecArgs{
Expand All @@ -87,7 +87,7 @@ func TestSafeFormatAndMount(t *testing.T) {
},
expectedError: fmt.Errorf("Still cannot mount"),
},
{ // Test that 'file' is called and confirms unformatted disk, format passes, second mount passes
{ // Test that 'lsblk' is called and confirms unformatted disk, format passes, second mount passes
fstype: "ext4",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'"), nil},
execScripts: []ExecArgs{
Expand All @@ -96,7 +96,7 @@ func TestSafeFormatAndMount(t *testing.T) {
},
expectedError: nil,
},
{ // Test that 'file' is called and confirms unformatted disk, format passes, second mount passes with ext3
{ // Test that 'lsblk' is called and confirms unformatted disk, format passes, second mount passes with ext3
fstype: "ext3",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'"), nil},
execScripts: []ExecArgs{
Expand All @@ -105,6 +105,16 @@ func TestSafeFormatAndMount(t *testing.T) {
},
expectedError: nil,
},
{ // Test that 'lsblk' is called and confirms unformatted disk, format passes, second mount passes
// test that none ext4 fs does not get called with ext4 options.
fstype: "xfs",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'"), nil},
execScripts: []ExecArgs{
{"lsblk", []string{"-nd", "-o", "FSTYPE", "/dev/foo"}, "", nil},
{"mkfs.xfs", []string{"/dev/foo"}, "", nil},
},
expectedError: nil,
},
Copy link

Choose a reason for hiding this comment

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

Oh we don't need to test a different fstype with all the test cases. Just one to test that ext4 gets run with the flags (probably already there) and one to test that non ext4 gets run without the flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all tests there just test ext4 with flags. new tests test all cases of non-ext fs without flags.

Copy link

Choose a reason for hiding this comment

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

In fact just keep the last test case above and delete the rest. Change to comment to something like "test that none ext4 fs does not get called with ext4 options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, updated

Copy link

Choose a reason for hiding this comment

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

cool, updated

Thanks... LGTM

}

for _, test := range tests {
Expand Down