From eb4f8e5319d692565ce7848785cba56444e18dbc Mon Sep 17 00:00:00 2001 From: Ashmeen Kaur <57195160+ashmeenkaur@users.noreply.github.com> Date: Fri, 28 Jun 2024 09:30:09 +0530 Subject: [PATCH] [CLI Config Parity] migrate log-file legacy flag/config to new config (#2062) * removed log-file legacy usage * migrate log file flag * fix integration test * add unit test --- cmd/flags.go | 10 ---- cmd/flags_test.go | 17 ++----- cmd/legacy_main.go | 6 +-- cmd/legacy_param_mapper_test.go | 48 +++++++++++++++++++ internal/config/config_util.go | 6 +-- internal/config/config_util_test.go | 8 +--- internal/logger/logger.go | 21 ++++---- internal/logger/logger_test.go | 7 +-- .../log_rotation/log_rotation_test.go | 1 + 9 files changed, 73 insertions(+), 51 deletions(-) diff --git a/cmd/flags.go b/cmd/flags.go index aa5c18a56a..f8f7963de6 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -485,11 +485,6 @@ func resolvePathForTheFlagInContext(flagKey string, c *cli.Context) (err error) // GCSFUSE_PARENT_PROCESS_DIR. Child process is spawned when --foreground flag // is disabled. func resolvePathForTheFlagsInContext(c *cli.Context) (err error) { - err = resolvePathForTheFlagInContext("log-file", c) - if err != nil { - return fmt.Errorf("resolving for log-file: %w", err) - } - err = resolvePathForTheFlagInContext("config-file", c) if err != nil { return fmt.Errorf("resolving for config-file: %w", err) @@ -500,11 +495,6 @@ func resolvePathForTheFlagsInContext(c *cli.Context) (err error) { // resolveConfigFilePaths resolves the config file paths specified in the config file. func resolveConfigFilePaths(mountConfig *config.MountConfig) (err error) { - mountConfig.LogConfig.FilePath, err = resolveFilePath(mountConfig.LogConfig.FilePath, "logging: file") - if err != nil { - return - } - // Resolve cache-dir path resolvedPath, err := resolveFilePath(string(mountConfig.CacheDir), "cache-dir") mountConfig.CacheDir = resolvedPath diff --git a/cmd/flags_test.go b/cmd/flags_test.go index 75f6aae100..2d66710fb0 100644 --- a/cmd/flags_test.go +++ b/cmd/flags_test.go @@ -274,23 +274,18 @@ func (t *FlagsTest) TestResolvePathForTheFlagInContext() { currentWorkingDir, err := os.Getwd() assert.Equal(t.T(), nil, err) app.Action = func(appCtx *cli.Context) { - err = resolvePathForTheFlagInContext("log-file", appCtx) - assert.Equal(t.T(), nil, err) err = resolvePathForTheFlagInContext("key-file", appCtx) assert.Equal(t.T(), nil, err) err = resolvePathForTheFlagInContext("config-file", appCtx) assert.Equal(t.T(), nil, err) - assert.Equal(t.T(), filepath.Join(currentWorkingDir, "test.txt"), - appCtx.String("log-file")) assert.Equal(t.T(), filepath.Join(currentWorkingDir, "test.txt"), appCtx.String("key-file")) assert.Equal(t.T(), filepath.Join(currentWorkingDir, "config.yaml"), appCtx.String("config-file")) } // Simulate argv. - fullArgs := []string{"some_app", "--log-file=test.txt", - "--key-file=test.txt", "--config-file=config.yaml"} + fullArgs := []string{"some_app", "--key-file=test.txt", "--config-file=config.yaml"} err = app.Run(fullArgs) @@ -303,13 +298,12 @@ func (t *FlagsTest) TestResolvePathForTheFlagsInContext() { assert.Equal(t.T(), nil, err) app.Action = func(appCtx *cli.Context) { resolvePathForTheFlagsInContext(appCtx) - assert.Equal(t.T(), filepath.Join(currentWorkingDir, "test.txt"), - appCtx.String("log-file")) + assert.Equal(t.T(), filepath.Join(currentWorkingDir, "config.yaml"), appCtx.String("config-file")) } // Simulate argv. - fullArgs := []string{"some_app", "--log-file=test.txt", "--config-file=config.yaml"} + fullArgs := []string{"some_app", "--config-file=config.yaml"} err = app.Run(fullArgs) @@ -417,9 +411,6 @@ func (t *FlagsTest) TestValidateFlagsForUnsupportedExperimentalMetadataPrefetchO func (t *FlagsTest) Test_resolveConfigFilePaths() { mountConfig := &config.MountConfig{} - mountConfig.LogConfig = config.LogConfig{ - FilePath: "~/test.txt", - } mountConfig.CacheDir = "~/cache-dir" err := resolveConfigFilePaths(mountConfig) @@ -427,7 +418,6 @@ func (t *FlagsTest) Test_resolveConfigFilePaths() { assert.Equal(t.T(), nil, err) homeDir, err := os.UserHomeDir() assert.Equal(t.T(), nil, err) - assert.Equal(t.T(), filepath.Join(homeDir, "test.txt"), mountConfig.LogConfig.FilePath) assert.EqualValues(t.T(), filepath.Join(homeDir, "cache-dir"), mountConfig.CacheDir) } @@ -437,7 +427,6 @@ func (t *FlagsTest) Test_resolveConfigFilePaths_WithoutSettingPaths() { err := resolveConfigFilePaths(mountConfig) assert.Equal(t.T(), nil, err) - assert.Equal(t.T(), "", mountConfig.LogConfig.FilePath) assert.EqualValues(t.T(), "", mountConfig.CacheDir) } diff --git a/cmd/legacy_main.go b/cmd/legacy_main.go index 3af07ac776..7e432018a4 100644 --- a/cmd/legacy_main.go +++ b/cmd/legacy_main.go @@ -258,7 +258,7 @@ func runCLIApp(c *cli.Context) (err error) { return fmt.Errorf("error resolving flags and configs: %w", err) } - config.OverrideWithLoggingFlags(mountConfig, flags.LogFile, flags.LogFormat, + config.OverrideWithLoggingFlags(mountConfig, flags.LogFormat, flags.DebugFuse, flags.DebugGCS, flags.DebugMutex) config.OverrideWithIgnoreInterruptsFlag(c, mountConfig, flags.IgnoreInterrupts) config.OverrideWithAnonymousAccessFlag(c, mountConfig, flags.AnonymousAccess) @@ -276,7 +276,7 @@ func runCLIApp(c *cli.Context) (err error) { } if flags.Foreground { - err = logger.InitLogFile(mountConfig.LogConfig) + err = logger.InitLogFile(mountConfig.LogConfig, newConfig.Logging) if err != nil { return fmt.Errorf("init log file: %w", err) } @@ -296,7 +296,7 @@ func runCLIApp(c *cli.Context) (err error) { // Do not log these in stdout in case of daemonized run // if these are already being logged into a log-file, otherwise // there will be duplicate logs for these in both places (stdout and log-file). - if flags.Foreground || mountConfig.LogConfig.FilePath == "" { + if flags.Foreground || newConfig.Logging.FilePath == "" { flagsStringified, err := util.Stringify(*flags) if err != nil { logger.Warnf("failed to stringify cli flags: %v", err) diff --git a/cmd/legacy_param_mapper_test.go b/cmd/legacy_param_mapper_test.go index d1aa23deac..c67c23c8bd 100644 --- a/cmd/legacy_param_mapper_test.go +++ b/cmd/legacy_param_mapper_test.go @@ -394,3 +394,51 @@ func TestPopulateConfigFromLegacyFlags_KeyFileResolution(t *testing.T) { }) } } + +func TestPopulateConfigFromLegacyFlags_LogFileResolution(t *testing.T) { + currentWorkingDir, err := os.Getwd() + require.Nil(t, err) + var logFileTests = []struct { + testName string + givenLogFile string + expectedLogFile cfg.ResolvedPath + }{ + { + testName: "absolute path", + givenLogFile: "/tmp/log-file.json", + expectedLogFile: "/tmp/log-file.json", + }, + { + testName: "relative path", + givenLogFile: "~/Documents/log-file.json", + expectedLogFile: cfg.ResolvedPath(path.Join(os.Getenv("HOME"), "Documents/log-file.json")), + }, + { + testName: "current working directory", + givenLogFile: "log-file.json", + expectedLogFile: cfg.ResolvedPath(path.Join(currentWorkingDir, "log-file.json")), + }, + { + testName: "empty path", + givenLogFile: "", + expectedLogFile: "", + }, + } + + for _, tc := range logFileTests { + t.Run(tc.testName, func(t *testing.T) { + mockCLICtx := &mockCLIContext{} + legacyFlagStorage := &flagStorage{ + ClientProtocol: mountpkg.HTTP2, + LogFile: tc.givenLogFile, + } + legacyMountCfg := &config.MountConfig{} + + resolvedConfig, err := PopulateNewConfigFromLegacyFlagsAndConfig(mockCLICtx, legacyFlagStorage, legacyMountCfg) + + if assert.Nil(t, err) { + assert.Equal(t, tc.expectedLogFile, resolvedConfig.Logging.FilePath) + } + }) + } +} diff --git a/internal/config/config_util.go b/internal/config/config_util.go index 6ebde8c5d9..fe387748e5 100644 --- a/internal/config/config_util.go +++ b/internal/config/config_util.go @@ -35,12 +35,8 @@ const ( // OverrideWithLoggingFlags overwrites the configs with the flag values if the // config values are empty. -func OverrideWithLoggingFlags(mountConfig *MountConfig, logFile string, logFormat string, +func OverrideWithLoggingFlags(mountConfig *MountConfig, logFormat string, debugFuse bool, debugGCS bool, debugMutex bool) { - // If log file is not set in config file, override it with flag value. - if mountConfig.LogConfig.FilePath == "" { - mountConfig.LogConfig.FilePath = logFile - } // If log format is not set in config file, override it with flag value. if mountConfig.LogConfig.Format == "" { mountConfig.LogConfig.Format = logFormat diff --git a/internal/config/config_util_test.go b/internal/config/config_util_test.go index ffe1713876..81a3e2e7b5 100644 --- a/internal/config/config_util_test.go +++ b/internal/config/config_util_test.go @@ -59,17 +59,15 @@ func (t *ConfigTest) TestOverrideLoggingFlags_WithNonEmptyLogConfigs() { mountConfig := &MountConfig{} mountConfig.LogConfig = LogConfig{ Severity: ERROR, - FilePath: "/tmp/hello.txt", Format: "text", } mountConfig.WriteConfig = WriteConfig{ CreateEmptyFile: true, } - OverrideWithLoggingFlags(mountConfig, f.LogFile, f.LogFormat, f.DebugFuse, f.DebugGCS, f.DebugMutex) + OverrideWithLoggingFlags(mountConfig, f.LogFormat, f.DebugFuse, f.DebugGCS, f.DebugMutex) assert.Equal(t.T(), "text", mountConfig.LogConfig.Format) - assert.Equal(t.T(), "/tmp/hello.txt", mountConfig.LogConfig.FilePath) assert.Equal(t.T(), TRACE, mountConfig.LogConfig.Severity) } @@ -81,17 +79,15 @@ func (t *ConfigTest) TestOverrideLoggingFlags_WithEmptyLogConfigs() { mountConfig := &MountConfig{} mountConfig.LogConfig = LogConfig{ Severity: INFO, - FilePath: "", Format: "", } mountConfig.WriteConfig = WriteConfig{ CreateEmptyFile: true, } - OverrideWithLoggingFlags(mountConfig, f.LogFile, f.LogFormat, f.DebugFuse, f.DebugGCS, f.DebugMutex) + OverrideWithLoggingFlags(mountConfig, f.LogFormat, f.DebugFuse, f.DebugGCS, f.DebugMutex) assert.Equal(t.T(), "json", mountConfig.LogConfig.Format) - assert.Equal(t.T(), "a.txt", mountConfig.LogConfig.FilePath) assert.Equal(t.T(), INFO, mountConfig.LogConfig.Severity) } diff --git a/internal/logger/logger.go b/internal/logger/logger.go index ed4dd27455..8ee43cca6f 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -23,6 +23,7 @@ import ( "os" "runtime/debug" + "github.com/googlecloudplatform/gcsfuse/v2/cfg" "github.com/googlecloudplatform/gcsfuse/v2/internal/config" "gopkg.in/natefinch/lumberjack.v2" ) @@ -51,14 +52,14 @@ var ( // config. // Here, background true means, this InitLogFile has been called for the // background daemon. -func InitLogFile(logConfig config.LogConfig) error { +func InitLogFile(legacyLogConfig config.LogConfig, newLogConfig cfg.LoggingConfig) error { var f *os.File var sysWriter *syslog.Writer var fileWriter *lumberjack.Logger var err error - if logConfig.FilePath != "" { + if newLogConfig.FilePath != "" { f, err = os.OpenFile( - logConfig.FilePath, + string(newLogConfig.FilePath), os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0644, ) @@ -67,9 +68,9 @@ func InitLogFile(logConfig config.LogConfig) error { } fileWriter = &lumberjack.Logger{ Filename: f.Name(), - MaxSize: logConfig.LogRotateConfig.MaxFileSizeMB, - MaxBackups: logConfig.LogRotateConfig.BackupFileCount, - Compress: logConfig.LogRotateConfig.Compress, + MaxSize: legacyLogConfig.LogRotateConfig.MaxFileSizeMB, + MaxBackups: legacyLogConfig.LogRotateConfig.BackupFileCount, + Compress: legacyLogConfig.LogRotateConfig.Compress, } } else { if _, ok := os.LookupEnv(GCSFuseInBackgroundMode); ok { @@ -90,11 +91,11 @@ func InitLogFile(logConfig config.LogConfig) error { file: f, sysWriter: sysWriter, fileWriter: fileWriter, - format: logConfig.Format, - level: logConfig.Severity, - logRotateConfig: logConfig.LogRotateConfig, + format: legacyLogConfig.Format, + level: legacyLogConfig.Severity, + logRotateConfig: legacyLogConfig.LogRotateConfig, } - defaultLogger = defaultLoggerFactory.newLogger(logConfig.Severity) + defaultLogger = defaultLoggerFactory.newLogger(legacyLogConfig.Severity) return nil } diff --git a/internal/logger/logger_test.go b/internal/logger/logger_test.go index daf6741bd9..dcd1ace6e1 100644 --- a/internal/logger/logger_test.go +++ b/internal/logger/logger_test.go @@ -21,6 +21,7 @@ import ( "regexp" "testing" + "github.com/googlecloudplatform/gcsfuse/v2/cfg" "github.com/googlecloudplatform/gcsfuse/v2/internal/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" @@ -274,18 +275,18 @@ func (t *LoggerTest) TestInitLogFile() { filePath += "/log.txt" fileSize := 100 backupFileCount := 2 - logConfig := config.LogConfig{ + legacyLogConfig := config.LogConfig{ Severity: config.DEBUG, Format: format, - FilePath: filePath, LogRotateConfig: config.LogRotateConfig{ MaxFileSizeMB: fileSize, BackupFileCount: backupFileCount, Compress: true, }, } + newLogConfig := cfg.LoggingConfig{FilePath: cfg.ResolvedPath(filePath)} - err := InitLogFile(logConfig) + err := InitLogFile(legacyLogConfig, newLogConfig) assert.NoError(t.T(), err) assert.Equal(t.T(), filePath, defaultLoggerFactory.file.Name()) diff --git a/tools/integration_tests/log_rotation/log_rotation_test.go b/tools/integration_tests/log_rotation/log_rotation_test.go index 9bd70eed6e..0470cb32c8 100644 --- a/tools/integration_tests/log_rotation/log_rotation_test.go +++ b/tools/integration_tests/log_rotation/log_rotation_test.go @@ -94,6 +94,7 @@ func TestMain(m *testing.M) { // Set up directory for logs. logDirPath = setup.SetUpLogDirForTestDirTests(logDirName) logFilePath = path.Join(logDirPath, logFileName) + setup.SetLogFile(logFilePath) // Set up config files. // TODO: add tests for backupLogFileCount = 0.