[CLI Config Parity] migrate mount options (#2255)

* migrate mount options

* add unit tests

* lint fix

* add more unit tests

* review comment

cmd/flags.go

@@ -410,7 +410,8 @@ type flagStorage struct {
 	ConfigFile string
 
 	// File system
-	MountOptions map[string]string
+	// Deprecated: Use the param from cfg/config.go
+	MountOptions []string
 
 	// Deprecated: Use the param from cfg/config.go
 	DirMode os.FileMode
@@ -608,7 +609,7 @@ func populateFlags(c *cli.Context) (flags *flagStorage, err error) {
 		ConfigFile: c.String("config-file"),
 
 		// File system
-		MountOptions:     make(map[string]string),
+		MountOptions:     c.StringSlice("o"),
 		DirMode:          os.FileMode(*c.Generic("dir-mode").(*OctalInt)),
 		FileMode:         os.FileMode(*c.Generic("file-mode").(*OctalInt)),
 		Uid:              int64(c.Int("uid")),
@@ -661,12 +662,6 @@ func populateFlags(c *cli.Context) (flags *flagStorage, err error) {
 		// Post-mount actions
 		ExperimentalMetadataPrefetchOnMount: c.String(ExperimentalMetadataPrefetchOnMountFlag),
 	}
-
-	// Handle the repeated "-o" flag.
-	for _, o := range c.StringSlice("o") {
-		mountpkg.ParseOptions(flags.MountOptions, o)
-	}
-
 	err = validateFlags(flags)
 
 	return

cmd/flags_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/googlecloudplatform/gcsfuse/v2/internal/mount"
 	mountpkg "github.com/googlecloudplatform/gcsfuse/v2/internal/mount"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
 	"github.com/urfave/cli"
 )
@@ -241,27 +242,18 @@ func (t *FlagsTest) TestDurations() {
 	assert.Equal(t.T(), 30*time.Second, f.MaxRetrySleep)
 }
 
-func (t *FlagsTest) TestMaps() {
+func (t *FlagsTest) TestSlice() {
 	args := []string{
 		"-o", "rw,nodev",
 		"-o", "user=jacobsa,noauto",
 	}
 
 	f := parseArgs(t, args)
 
-	var keys sort.StringSlice
-	for k := range f.MountOptions {
-		keys = append(keys, k)
-	}
-
-	sort.Sort(keys)
-	assert.ElementsMatch(t.T(),
-		keys, []string{"noauto", "nodev", "rw", "user"})
-
-	assert.Equal(t.T(), "", f.MountOptions["noauto"])
-	assert.Equal(t.T(), "", f.MountOptions["nodev"])
-	assert.Equal(t.T(), "", f.MountOptions["rw"])
-	assert.Equal(t.T(), "jacobsa", f.MountOptions["user"])
+	sort.Strings(f.MountOptions)
+	require.Equal(t.T(), 2, len(f.MountOptions))
+	assert.Equal(t.T(), "rw,nodev", f.MountOptions[0])
+	assert.Equal(t.T(), "user=jacobsa,noauto", f.MountOptions[1])
 }
 
 func (t *FlagsTest) TestResolvePathForTheFlagInContext() {

cmd/legacy_param_mapper.go

@@ -52,9 +52,9 @@ func PopulateNewConfigFromLegacyFlagsAndConfig(c cliContext, flags *flagStorage,
 			"fuse":                        flags.DebugFuse,
 		},
 		"file-system": map[string]interface{}{
-			"dir-mode":  flags.DirMode,
-			"file-mode": flags.FileMode,
-			// Todo: "fuse-options":      nil,
+			"dir-mode":                   flags.DirMode,
+			"file-mode":                  flags.FileMode,
+			"fuse-options":               flags.MountOptions,
 			"gid":                        flags.Gid,
 			"ignore-interrupts":          flags.IgnoreInterrupts,
 			"rename-dir-limit":           flags.RenameDirLimit,
@@ -154,9 +154,11 @@ func PopulateNewConfigFromLegacyFlagsAndConfig(c cliContext, flags *flagStorage,
 	overrideWithFlag(c, "prometheus-port", &resolvedConfig.Metrics.PrometheusPort, prometheusPort)
 
 	if err := cfg.ValidateConfig(resolvedConfig); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("cfg.ValidateConfig: %w", err)
+	}
+	if err := cfg.Rationalize(resolvedConfig); err != nil {
+		return nil, fmt.Errorf("cfg.Rationalize: %w", err)
 	}
-	cfg.Rationalize(resolvedConfig)
 	return resolvedConfig, nil
 }
 

cmd/legacy_param_mapper_test.go

@@ -554,3 +554,16 @@ func TestEnableEmptyManagedFoldersRationalization(t *testing.T) {
 		})
 	}
 }
+
+func TestPopulateConfigFromLegacyFlags_MountOption(t *testing.T) {
+	flags := &flagStorage{
+		MountOptions:   []string{"rw,nodev", "user=jacobsa,noauto"},
+		ClientProtocol: mountpkg.ClientProtocol(cfg.HTTP2),
+	}
+
+	v, err := PopulateNewConfigFromLegacyFlagsAndConfig(&mockCLIContext{}, flags, &config.MountConfig{})
+
+	if assert.Nil(t, err) {
+		assert.Equal(t, v.FileSystem.FuseOptions, []string{"rw,nodev", "user=jacobsa,noauto"})
+	}
+}

cmd/mount.go

@@ -142,11 +142,29 @@ be interacting with the file system.`)
 
 	// Mount the file system.
 	logger.Infof("Mounting file system %q...", fsName)
+
+	mountCfg := getFuseMountConfig(fsName, newConfig, mountConfig)
+	mfs, err = fuse.Mount(mountPoint, server, mountCfg)
+	if err != nil {
+		err = fmt.Errorf("Mount: %w", err)
+		return
+	}
+
+	return
+}
+
+func getFuseMountConfig(fsName string, newConfig *cfg.Config, mountConfig *config.MountConfig) *fuse.MountConfig {
+	// Handle the repeated "-o" flag.
+	parsedOptions := make(map[string]string)
+	for _, o := range newConfig.FileSystem.FuseOptions {
+		mount.ParseOptions(parsedOptions, o)
+	}
+
 	mountCfg := &fuse.MountConfig{
 		FSName:     fsName,
 		Subtype:    "gcsfuse",
 		VolumeName: "gcsfuse",
-		Options:    flags.MountOptions,
+		Options:    parsedOptions,
 		// Allows parallel LookUpInode & ReadDir calls from Kernel's FUSE driver.
 		// GCSFuse takes exclusive lock on directory inodes during ReadDir call,
 		// hence there is no effect of parallelization of incoming ReadDir calls
@@ -160,12 +178,5 @@ be interacting with the file system.`)
 
 	mountCfg.ErrorLogger = logger.NewLegacyLogger(logger.LevelError, "fuse: ")
 	mountCfg.DebugLogger = logger.NewLegacyLogger(logger.LevelTrace, "fuse_debug: ")
-
-	mfs, err = fuse.Mount(mountPoint, server, mountCfg)
-	if err != nil {
-		err = fmt.Errorf("Mount: %w", err)
-		return
-	}
-
-	return
+	return mountCfg
 }

cmd/mount_test.go

@@ -0,0 +1,70 @@
+// Copyright 2024 Google Inc. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cmd
+
+import (
+	"testing"
+
+	"github.com/googlecloudplatform/gcsfuse/v2/cfg"
+	"github.com/googlecloudplatform/gcsfuse/v2/internal/config"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetFuseMountConfig_MountOptionsFormattedCorrectly(t *testing.T) {
+	testCases := []struct {
+		name                string
+		inputFuseOptions    []string
+		expectedFuseOptions map[string]string
+	}{
+		{
+			name:             "Fuse options input with comma [legacy flag format].",
+			inputFuseOptions: []string{"rw,nodev", "user=jacobsa,noauto"},
+			expectedFuseOptions: map[string]string{
+				"noauto": "",
+				"nodev":  "",
+				"rw":     "",
+				"user":   "jacobsa",
+			},
+		},
+		{
+			name:             "Fuse options input without comma [new config format].",
+			inputFuseOptions: []string{"rw", "nodev", "user=jacobsa", "noauto"},
+			expectedFuseOptions: map[string]string{
+				"noauto": "",
+				"nodev":  "",
+				"rw":     "",
+				"user":   "jacobsa",
+			},
+		},
+	}
+
+	fsName := "mybucket"
+	mountConfig := &config.MountConfig{}
+	for _, tc := range testCases {
+		newConfig := &cfg.Config{
+			FileSystem: cfg.FileSystemConfig{
+				FuseOptions: tc.inputFuseOptions,
+			},
+		}
+
+		fuseMountCfg := getFuseMountConfig(fsName, newConfig, mountConfig)
+
+		assert.Equal(t, fsName, fuseMountCfg.FSName)
+		assert.Equal(t, "gcsfuse", fuseMountCfg.Subtype)
+		assert.Equal(t, "gcsfuse", fuseMountCfg.VolumeName)
+		assert.Equal(t, tc.expectedFuseOptions, fuseMountCfg.Options)
+		assert.True(t, fuseMountCfg.EnableParallelDirOps) // Default true unless explicitly disabled
+	}
+}

cmd/root_test.go

@@ -83,7 +83,7 @@ func TestCobraArgsNumInRange(t *testing.T) {
 	}
 }
 
-func TestArgsParsing(t *testing.T) {
+func TestArgsParsing_MountPoint(t *testing.T) {
 	wd, err := os.Getwd()
 	require.Nil(t, err)
 	hd, err := os.UserHomeDir()
@@ -140,3 +140,50 @@ func TestArgsParsing(t *testing.T) {
 		})
 	}
 }
+
+func TestArgsParsing_MountOptions(t *testing.T) {
+	tests := []struct {
+		name                 string
+		args                 []string
+		expectedMountOptions []string
+	}{
+		{
+			name:                 "Multiple mount options specified with multiple -o flags.",
+			args:                 []string{"gcsfuse", "--o", "rw,nodev", "--o", "user=jacobsa,noauto", "abc", "pqr"},
+			expectedMountOptions: []string{"rw", "nodev", "user=jacobsa", "noauto"},
+		},
+		{
+			name:                 "Only one mount option specified.",
+			args:                 []string{"gcsfuse", "--o", "rw", "abc", "pqr"},
+			expectedMountOptions: []string{"rw"},
+		},
+		{
+			name:                 "Multiple mount option specified with single flag.",
+			args:                 []string{"gcsfuse", "--o", "rw,nodev", "abc", "pqr"},
+			expectedMountOptions: []string{"rw", "nodev"},
+		},
+		{
+			name:                 "Multiple mount options specified with single -o flags.",
+			args:                 []string{"gcsfuse", "--o", "rw,nodev,user=jacobsa,noauto", "abc", "pqr"},
+			expectedMountOptions: []string{"rw", "nodev", "user=jacobsa", "noauto"},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			var mountOptions []string
+			cmd, err := NewRootCmd(func(cfg *cfg.Config, _ string, _ string) error {
+				mountOptions = cfg.FileSystem.FuseOptions
+				return nil
+			})
+			require.Nil(t, err)
+			cmd.SetArgs(tc.args)
+
+			err = cmd.Execute()
+
+			if assert.NoError(t, err) {
+				assert.Equal(t, tc.expectedMountOptions, mountOptions)
+			}
+		})
+	}
+}