From 24500803e53043f0bf11a7baf08839a825be2346 Mon Sep 17 00:00:00 2001 From: Ashmeen Kaur <57195160+ashmeenkaur@users.noreply.github.com> Date: Tue, 25 Jun 2024 18:28:57 +0530 Subject: [PATCH 1/4] Remove custom type strings from legacy mount config (#2060) * dont use custom type strings in legacy mount config * flaky test fixes --- cmd/flags.go | 2 +- internal/config/mount_config.go | 5 ++--- internal/config/yaml_parser.go | 18 ++++++++---------- internal/fs/parallel_dirops_test.go | 1 + internal/fs/read_cache_test.go | 8 ++++---- internal/logger/logger.go | 4 ++-- internal/logger/logger_test.go | 8 ++++---- internal/logger/slog_helper.go | 2 +- .../operations/operations_test.go | 2 +- .../integration_tests/read_cache/setup_test.go | 2 +- .../read_large_files/read_large_files_test.go | 4 ++-- .../readonly/readonly_test.go | 2 +- 12 files changed, 28 insertions(+), 30 deletions(-) diff --git a/cmd/flags.go b/cmd/flags.go index 3457628db8..09340e251d 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -512,7 +512,7 @@ func resolveConfigFilePaths(mountConfig *config.MountConfig) (err error) { // Resolve cache-dir path resolvedPath, err := resolveFilePath(string(mountConfig.CacheDir), "cache-dir") - mountConfig.CacheDir = config.CacheDir(resolvedPath) + mountConfig.CacheDir = resolvedPath if err != nil { return } diff --git a/internal/config/mount_config.go b/internal/config/mount_config.go index d5ddcf900b..cfbfcf9709 100644 --- a/internal/config/mount_config.go +++ b/internal/config/mount_config.go @@ -71,7 +71,7 @@ type WriteConfig struct { } type LogConfig struct { - Severity LogSeverity `yaml:"severity"` + Severity string `yaml:"severity"` Format string `yaml:"format"` FilePath string `yaml:"file-path"` LogRotateConfig LogRotateConfig `yaml:"log-rotate"` @@ -103,7 +103,6 @@ type GCSAuth struct { // Enable the storage control client flow on HNS buckets to utilize new APIs. type EnableHNS bool -type CacheDir string type FileSystemConfig struct { IgnoreInterrupts bool `yaml:"ignore-interrupts"` @@ -144,7 +143,7 @@ type MountConfig struct { WriteConfig `yaml:"write"` LogConfig `yaml:"logging"` FileCacheConfig `yaml:"file-cache"` - CacheDir `yaml:"cache-dir"` + CacheDir string `yaml:"cache-dir"` MetadataCacheConfig `yaml:"metadata-cache"` ListConfig `yaml:"list"` GCSConnection `yaml:"gcs-connection"` diff --git a/internal/config/yaml_parser.go b/internal/config/yaml_parser.go index d33e5ab56f..20b4068ce9 100644 --- a/internal/config/yaml_parser.go +++ b/internal/config/yaml_parser.go @@ -25,15 +25,13 @@ import ( "gopkg.in/yaml.v3" ) -type LogSeverity string - const ( - TRACE LogSeverity = "TRACE" - DEBUG LogSeverity = "DEBUG" - INFO LogSeverity = "INFO" - WARNING LogSeverity = "WARNING" - ERROR LogSeverity = "ERROR" - OFF LogSeverity = "OFF" + TRACE string = "TRACE" + DEBUG string = "DEBUG" + INFO string = "INFO" + WARNING string = "WARNING" + ERROR string = "ERROR" + OFF string = "OFF" parseConfigFileErrMsgFormat = "error parsing config file: %v" @@ -50,7 +48,7 @@ const ( DownloadChunkSizeMBInvalidValueError = "the value of download-chunk-size-mb for file-cache can't be less than 1" ) -func IsValidLogSeverity(severity LogSeverity) bool { +func IsValidLogSeverity(severity string) bool { switch severity { case TRACE, @@ -159,7 +157,7 @@ func ParseConfigFile(fileName string) (mountConfig *MountConfig, err error) { } // convert log severity to upper-case - mountConfig.LogConfig.Severity = LogSeverity(strings.ToUpper(string(mountConfig.LogConfig.Severity))) + mountConfig.LogConfig.Severity = strings.ToUpper(mountConfig.LogConfig.Severity) if !IsValidLogSeverity(mountConfig.LogConfig.Severity) { err = fmt.Errorf("error parsing config file: log severity should be one of [trace, debug, info, warning, error, off]") return diff --git a/internal/fs/parallel_dirops_test.go b/internal/fs/parallel_dirops_test.go index 3f8676e2ec..58f310f145 100644 --- a/internal/fs/parallel_dirops_test.go +++ b/internal/fs/parallel_dirops_test.go @@ -377,6 +377,7 @@ func (t *ParallelDiropsTest) TestParallelLookUpAndCreateSameFile() { } else { assert.True(t.T(), os.IsNotExist(lookUpErr)) } + assert.Nil(t.T(), file.Close()) } func (t *ParallelDiropsTest) TestParallelLookUpAndDeleteSameFile() { diff --git a/internal/fs/read_cache_test.go b/internal/fs/read_cache_test.go index 4fe553e8c8..b55335985d 100644 --- a/internal/fs/read_cache_test.go +++ b/internal/fs/read_cache_test.go @@ -72,7 +72,7 @@ func (t *FileCacheTest) SetUpTestSuite() { CacheFileForRangeRead: false, EnableCRC: true, }, - CacheDir: config.CacheDir(CacheDir), + CacheDir: CacheDir, } t.fsTest.SetUpTestSuite() } @@ -607,7 +607,7 @@ func (t *FileCacheWithCacheForRangeRead) SetUpTestSuite() { MaxSizeMB: -1, CacheFileForRangeRead: true, }, - CacheDir: config.CacheDir(CacheDir), + CacheDir: CacheDir, } t.fsTest.SetUpTestSuite() } @@ -740,7 +740,7 @@ func (t *FileCacheIsDisabledWithCacheDirAndZeroMaxSize) SetUpTestSuite() { MaxSizeMB: 0, CacheFileForRangeRead: true, }, - CacheDir: config.CacheDir(CacheDir), + CacheDir: CacheDir, } t.fsTest.SetUpTestSuite() } @@ -784,7 +784,7 @@ func (t *FileCacheDestroyTest) SetUpTestSuite() { MaxSizeMB: -1, CacheFileForRangeRead: true, }, - CacheDir: config.CacheDir(CacheDir), + CacheDir: CacheDir, } t.fsTest.SetUpTestSuite() } diff --git a/internal/logger/logger.go b/internal/logger/logger.go index cae9b88189..ed4dd27455 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -169,12 +169,12 @@ type loggerFactory struct { file *os.File sysWriter *syslog.Writer format string - level config.LogSeverity + level string logRotateConfig config.LogRotateConfig fileWriter *lumberjack.Logger } -func (f *loggerFactory) newLogger(level config.LogSeverity) *slog.Logger { +func (f *loggerFactory) newLogger(level string) *slog.Logger { // create a new logger var programLevel = new(slog.LevelVar) logger := slog.New(f.handler(programLevel, "")) diff --git a/internal/logger/logger_test.go b/internal/logger/logger_test.go index 3200b19251..daf6741bd9 100644 --- a/internal/logger/logger_test.go +++ b/internal/logger/logger_test.go @@ -52,7 +52,7 @@ func TestLoggerSuite(t *testing.T) { // Boilerplate // ////////////////////////////////////////////////////////////////////// -func redirectLogsToGivenBuffer(buf *bytes.Buffer, level config.LogSeverity) { +func redirectLogsToGivenBuffer(buf *bytes.Buffer, level string) { var programLevel = new(slog.LevelVar) defaultLogger = slog.New( defaultLoggerFactory.createJsonOrTextHandler(buf, programLevel, "TestLogs: "), @@ -63,7 +63,7 @@ func redirectLogsToGivenBuffer(buf *bytes.Buffer, level config.LogSeverity) { // fetchLogOutputForSpecifiedSeverityLevel takes configured severity and // functions that write logs as parameter and returns string array containing // output from each function call. -func fetchLogOutputForSpecifiedSeverityLevel(level config.LogSeverity, functions []func()) []string { +func fetchLogOutputForSpecifiedSeverityLevel(level string, functions []func()) []string { // create a logger that writes to buffer at configured level. var buf bytes.Buffer redirectLogsToGivenBuffer(&buf, level) @@ -109,7 +109,7 @@ func validateOutput(t *testing.T, expected []string, output []string) { } } -func validateLogOutputAtSpecifiedFormatAndSeverity(t *testing.T, format string, level config.LogSeverity, expectedOutput []string) { +func validateLogOutputAtSpecifiedFormatAndSeverity(t *testing.T, format string, level string, expectedOutput []string) { // set log format defaultLoggerFactory.format = format @@ -231,7 +231,7 @@ func (t *LoggerTest) TestJSONFormatLogs_LogLevelTRACE() { func (t *LoggerTest) TestSetLoggingLevel() { testData := []struct { - inputLevel config.LogSeverity + inputLevel string programLevel *slog.LevelVar expectedProgramLevel slog.Level }{ diff --git a/internal/logger/slog_helper.go b/internal/logger/slog_helper.go index 944eeafa8e..225b62bd16 100644 --- a/internal/logger/slog_helper.go +++ b/internal/logger/slog_helper.go @@ -37,7 +37,7 @@ const ( nanosKey = "nanos" ) -func setLoggingLevel(level config.LogSeverity, programLevel *slog.LevelVar) { +func setLoggingLevel(level string, programLevel *slog.LevelVar) { switch level { // logs having severity >= the configured value will be logged. case config.TRACE: diff --git a/tools/integration_tests/operations/operations_test.go b/tools/integration_tests/operations/operations_test.go index 480a0deee3..1e1f7968e0 100644 --- a/tools/integration_tests/operations/operations_test.go +++ b/tools/integration_tests/operations/operations_test.go @@ -115,7 +115,7 @@ func createMountConfigsAndEquivalentFlags() (flags [][]string) { // files MaxSizeMB: 2, }, - CacheDir: config.CacheDir(cacheDirPath), + CacheDir: cacheDirPath, LogConfig: config.LogConfig{ Severity: config.TRACE, LogRotateConfig: config.DefaultLogRotateConfig(), diff --git a/tools/integration_tests/read_cache/setup_test.go b/tools/integration_tests/read_cache/setup_test.go index a4328f6a1b..cb1c791c54 100644 --- a/tools/integration_tests/read_cache/setup_test.go +++ b/tools/integration_tests/read_cache/setup_test.go @@ -108,7 +108,7 @@ func createConfigFile(cacheSize int64, cacheFileForRangeRead bool, fileName stri MaxSizeMB: cacheSize, CacheFileForRangeRead: cacheFileForRangeRead, }, - CacheDir: config.CacheDir(cacheDirPath), + CacheDir: cacheDirPath, LogConfig: config.LogConfig{ Severity: config.TRACE, Format: "json", diff --git a/tools/integration_tests/read_large_files/read_large_files_test.go b/tools/integration_tests/read_large_files/read_large_files_test.go index f0dc398ad9..da76767ac9 100644 --- a/tools/integration_tests/read_large_files/read_large_files_test.go +++ b/tools/integration_tests/read_large_files/read_large_files_test.go @@ -48,7 +48,7 @@ func createMountConfigsAndEquivalentFlags() (flags [][]string) { MaxSizeMB: 700, CacheFileForRangeRead: true, }, - CacheDir: config.CacheDir(cacheDirPath), + CacheDir: cacheDirPath, LogConfig: config.LogConfig{ Severity: config.TRACE, LogRotateConfig: config.DefaultLogRotateConfig(), @@ -63,7 +63,7 @@ func createMountConfigsAndEquivalentFlags() (flags [][]string) { MaxSizeMB: -1, CacheFileForRangeRead: false, }, - CacheDir: config.CacheDir(cacheDirPath), + CacheDir: cacheDirPath, LogConfig: config.LogConfig{ Severity: config.TRACE, LogRotateConfig: config.DefaultLogRotateConfig(), diff --git a/tools/integration_tests/readonly/readonly_test.go b/tools/integration_tests/readonly/readonly_test.go index 18ff9d140d..33cc00cbf9 100644 --- a/tools/integration_tests/readonly/readonly_test.go +++ b/tools/integration_tests/readonly/readonly_test.go @@ -103,7 +103,7 @@ func createMountConfigsAndEquivalentFlags() (flags [][]string) { // files MaxSizeMB: 3, }, - CacheDir: config.CacheDir(cacheDirPath), + CacheDir: cacheDirPath, LogConfig: config.LogConfig{ Severity: config.TRACE, LogRotateConfig: config.DefaultLogRotateConfig(), From 492f2babbe9605b3aa2fa9cab148ec48fcf85c82 Mon Sep 17 00:00:00 2001 From: Ashmeen Kaur <57195160+ashmeenkaur@users.noreply.github.com> Date: Wed, 26 Jun 2024 17:09:52 +0530 Subject: [PATCH 2/4] Create new config object from legacy flags and config (#2061) * changes to populate config * remove unecessary change * add client protocol changes * review comments * review comment * rebase changes * review comments * review comment + unit tests * review comment --- cmd/legacy_param_mapper.go | 155 +++++++++++++ cmd/legacy_param_mapper_test.go | 388 ++++++++++++++++++++++++++++++++ tools/config-gen/params.yaml | 2 +- 3 files changed, 544 insertions(+), 1 deletion(-) create mode 100644 cmd/legacy_param_mapper.go create mode 100644 cmd/legacy_param_mapper_test.go diff --git a/cmd/legacy_param_mapper.go b/cmd/legacy_param_mapper.go new file mode 100644 index 0000000000..65cd9d7f8c --- /dev/null +++ b/cmd/legacy_param_mapper.go @@ -0,0 +1,155 @@ +// 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 ( + "fmt" + "reflect" + + "github.com/googlecloudplatform/gcsfuse/v2/cfg" + "github.com/googlecloudplatform/gcsfuse/v2/internal/config" + "github.com/mitchellh/mapstructure" +) + +// cliContext is abstraction over the IsSet() method of cli.Context, specially +// added to keep OverrideWithIgnoreInterruptsFlag method's unit test simple. +type cliContext interface { + IsSet(string) bool +} + +// PopulateNewConfigFromLegacyFlagsAndConfig takes cliContext, legacy flags and legacy MountConfig and resolves it into new cfg.Config Object. +func PopulateNewConfigFromLegacyFlagsAndConfig(c cliContext, flags *flagStorage, legacyConfig *config.MountConfig) (*cfg.Config, error) { + if flags == nil || legacyConfig == nil { + return nil, fmt.Errorf("PopulateNewConfigFromLegacyFlagsAndConfig: unexpected nil flags or mount config") + } + + resolvedConfig := &cfg.Config{} + + structuredFlags := &map[string]interface{}{ + "app-name": flags.AppName, + "debug": &map[string]interface{}{ + "exit-on-invariant-violation": flags.DebugInvariants, + "gcs": flags.DebugGCS, + "log-mutex": flags.DebugMutex, + }, + "file-system": map[string]interface{}{ + "dir-mode": flags.DirMode, + "file-mode": flags.FileMode, + // Todo: "fuse-options": nil, + "gid": flags.Gid, + "ignore-interrupts": flags.IgnoreInterrupts, + "rename-dir-limit": flags.RenameDirLimit, + "temp-dir": flags.TempDir, + "uid": flags.Uid, + }, + "foreground": flags.Foreground, + "gcs-auth": map[string]interface{}{ + "anonymous-access": flags.AnonymousAccess, + "key-file": flags.KeyFile, + "reuse-token-from-url": flags.ReuseTokenFromUrl, + "token-url": flags.TokenUrl, + }, + "gcs-connection": map[string]interface{}{ + "billing-project": flags.BillingProject, + "client-protocol": string(flags.ClientProtocol), + "custom-endpoint": flags.CustomEndpoint, + "experimental-enable-json-read": flags.ExperimentalEnableJsonRead, + "http-client-timeout": flags.HttpClientTimeout, + "limit-bytes-per-sec": flags.EgressBandwidthLimitBytesPerSecond, + "limit-ops-per-sec": flags.OpRateLimitHz, + "max-conns-per-host": flags.MaxConnsPerHost, + "max-idle-conns-per-host": flags.MaxIdleConnsPerHost, + "sequential-read-size-mb": flags.SequentialReadSizeMb, + }, + "gcs-retries": map[string]interface{}{ + "max-retry-sleep": flags.MaxRetrySleep, + "multiplier": flags.RetryMultiplier, + }, + "implicit-dirs": flags.ImplicitDirs, + "list": map[string]interface{}{ + "kernel-list-cache-ttl-secs": flags.KernelListCacheTtlSeconds, + }, + "logging": map[string]interface{}{ + "file-path": flags.LogFile, + "format": flags.LogFormat, + }, + "metadata-cache": map[string]interface{}{ + "deprecated-stat-cache-capacity": flags.StatCacheCapacity, + "deprecated-stat-cache-ttl": flags.StatCacheTTL, + "deprecated-type-cache-ttl": flags.TypeCacheTTL, + "enable-nonexistent-type-cache": flags.EnableNonexistentTypeCache, + "experimental-metadata-prefetch-on-mount": flags.ExperimentalMetadataPrefetchOnMount, + }, + "metrics": map[string]interface{}{ + "stackdriver-export-interval": flags.StackdriverExportInterval, + }, + "monitoring": map[string]interface{}{ + "experimental-opentelemetry-collector-address": flags.OtelCollectorAddress, + }, + "only-dir": flags.OnlyDir, + } + + // Use decoder to convert flagStorage to cfg.Config. + decoderConfig := &mapstructure.DecoderConfig{ + DecodeHook: cfg.DecodeHook(), + Result: resolvedConfig, + TagName: "yaml", + } + decoder, err := mapstructure.NewDecoder(decoderConfig) + if err != nil { + return nil, fmt.Errorf("mapstructure.NewDecoder: %w", err) + } + // Decoding flags. + if err = decoder.Decode(structuredFlags); err != nil { + return nil, fmt.Errorf("decoder.Decode(structuredFlags): %w", err) + } + + // If config file does not have any values, no need to decode it. + if reflect.ValueOf(*legacyConfig).IsZero() { + return resolvedConfig, nil + } + + // Save overlapping flags in a map to override the config value later. + var ( + logFile = resolvedConfig.Logging.FilePath + logFormat = resolvedConfig.Logging.Format + ignoreInterrupts = resolvedConfig.FileSystem.IgnoreInterrupts + anonymousAccess = resolvedConfig.GcsAuth.AnonymousAccess + kernelListCacheTTLSecs = resolvedConfig.List.KernelListCacheTtlSecs + ) + + // Decoding config to the same config structure (resolvedConfig). + if err = decoder.Decode(legacyConfig); err != nil { + return nil, fmt.Errorf("decoder.Decode(config): %w", err) + } + + // Override/Give priority to flags in case of overlap in flags and config. + overrideWithFlag(c, "log-file", &resolvedConfig.Logging.FilePath, logFile) + overrideWithFlag(c, "log-format", &resolvedConfig.Logging.Format, logFormat) + overrideWithFlag(c, "ignore-interrupts", &resolvedConfig.FileSystem.IgnoreInterrupts, ignoreInterrupts) + overrideWithFlag(c, "anonymous-access", &resolvedConfig.GcsAuth.AnonymousAccess, anonymousAccess) + overrideWithFlag(c, "kernel-list-cache-ttl-secs", &resolvedConfig.List.KernelListCacheTtlSecs, kernelListCacheTTLSecs) + + return resolvedConfig, nil +} + +// overrideWithFlag function overrides the toUpdate value with updateValue if +// the flag is set in cliCOntext. +func overrideWithFlag[T any](c cliContext, flag string, toUpdate *T, updateValue T) { + if !c.IsSet(flag) { + return + } + *toUpdate = updateValue +} diff --git a/cmd/legacy_param_mapper_test.go b/cmd/legacy_param_mapper_test.go new file mode 100644 index 0000000000..7f08770321 --- /dev/null +++ b/cmd/legacy_param_mapper_test.go @@ -0,0 +1,388 @@ +// 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 ( + "fmt" + "net/url" + "os" + "path" + "testing" + + "github.com/googlecloudplatform/gcsfuse/v2/cfg" + "github.com/googlecloudplatform/gcsfuse/v2/internal/config" + mountpkg "github.com/googlecloudplatform/gcsfuse/v2/internal/mount" + "github.com/stretchr/testify/assert" + "github.com/urfave/cli" +) + +type mockCLIContext struct { + cli.Context + isFlagSet map[string]bool +} + +func (m *mockCLIContext) IsSet(name string) bool { + return m.isFlagSet[name] +} + +func TestPopulateConfigFromLegacyFlags(t *testing.T) { + var populateConfigFromLegacyFlags = []struct { + testName string + legacyFlagStorage *flagStorage + mockCLICtx *mockCLIContext + legacyMountConfig *config.MountConfig + expectedConfig *cfg.Config + expectedErr error + }{ + { + testName: "nil flags", + legacyFlagStorage: nil, + mockCLICtx: &mockCLIContext{isFlagSet: map[string]bool{}}, + legacyMountConfig: &config.MountConfig{}, + expectedConfig: nil, + expectedErr: fmt.Errorf("PopulateNewConfigFromLegacyFlagsAndConfig: unexpected nil flags or mount config"), + }, + { + testName: "nil config", + legacyFlagStorage: &flagStorage{}, + mockCLICtx: &mockCLIContext{isFlagSet: map[string]bool{}}, + legacyMountConfig: nil, + expectedConfig: nil, + expectedErr: fmt.Errorf("PopulateNewConfigFromLegacyFlagsAndConfig: unexpected nil flags or mount config"), + }, + { + testName: "Test decode legacy flags.", + legacyFlagStorage: &flagStorage{ + AppName: "vertex", + Foreground: false, + ConfigFile: "~/Documents/config.yaml", + DirMode: 0755, + FileMode: 0711, + Uid: -1, + Gid: 17, + ImplicitDirs: true, + OnlyDir: "abc", + RenameDirLimit: 10, + IgnoreInterrupts: false, + CustomEndpoint: nil, + BillingProject: "billing-project", + KeyFile: "~/Documents/key-file", + TokenUrl: "tokenUrl", + ReuseTokenFromUrl: true, + EgressBandwidthLimitBytesPerSecond: 100, + OpRateLimitHz: 50, + SequentialReadSizeMb: 40, + AnonymousAccess: false, + MaxRetrySleep: 10, + RetryMultiplier: 2, + StatCacheCapacity: 200, + StatCacheTTL: 50, + TypeCacheTTL: 70, + KernelListCacheTtlSeconds: 30, + HttpClientTimeout: 100, + MaxRetryDuration: 10, + LocalFileCache: false, + TempDir: "~/temp", + MaxConnsPerHost: 200, + MaxIdleConnsPerHost: 150, + EnableNonexistentTypeCache: false, + StackdriverExportInterval: 40, + OtelCollectorAddress: "address", + LogFile: "/tmp/log-file.json", + LogFormat: "json", + ExperimentalEnableJsonRead: true, + DebugGCS: true, + DebugInvariants: true, + DebugMutex: true, + ExperimentalMetadataPrefetchOnMount: "sync", + ClientProtocol: mountpkg.HTTP1, + }, + mockCLICtx: &mockCLIContext{isFlagSet: map[string]bool{}}, + legacyMountConfig: &config.MountConfig{}, + expectedConfig: &cfg.Config{ + AppName: "vertex", + Foreground: false, + FileSystem: cfg.FileSystemConfig{ + DirMode: 493, // Octal(755) converted to decimal + FileMode: 457, // Octal(711) converted to decimal + Uid: -1, + Gid: 17, + RenameDirLimit: 10, + IgnoreInterrupts: false, + DisableParallelDirops: false, + FuseOptions: []string(nil), + TempDir: cfg.ResolvedPath(path.Join(os.Getenv("HOME"), "/temp")), + }, + ImplicitDirs: true, + OnlyDir: "abc", + CacheDir: "", + FileCache: cfg.FileCacheConfig{ + CacheFileForRangeRead: false, + ParallelDownloadsPerFile: 0, + EnableCrc: false, + EnableParallelDownloads: false, + MaxParallelDownloads: 0, + MaxSizeMb: 0, + DownloadChunkSizeMb: 0, + }, + GcsAuth: cfg.GcsAuthConfig{ + KeyFile: cfg.ResolvedPath(path.Join(os.Getenv("HOME"), "Documents/key-file")), + TokenUrl: "tokenUrl", + ReuseTokenFromUrl: true, + AnonymousAccess: false, + }, + GcsConnection: cfg.GcsConnectionConfig{ + CustomEndpoint: url.URL{}, + BillingProject: "billing-project", + ClientProtocol: cfg.Protocol("http1"), + LimitBytesPerSec: 100, + LimitOpsPerSec: 50, + SequentialReadSizeMb: 40, + MaxConnsPerHost: 200, + MaxIdleConnsPerHost: 150, + HttpClientTimeout: 100, + ExperimentalEnableJsonRead: true, + }, + GcsRetries: cfg.GcsRetriesConfig{ + MaxRetrySleep: 10, + Multiplier: 2, + }, + List: cfg.ListConfig{ + KernelListCacheTtlSecs: 30, + }, + Logging: cfg.LoggingConfig{ + FilePath: cfg.ResolvedPath("/tmp/log-file.json"), + Format: "json", + }, + MetadataCache: cfg.MetadataCacheConfig{ + DeprecatedStatCacheCapacity: 200, + DeprecatedStatCacheTtl: 50, + DeprecatedTypeCacheTtl: 70, + EnableNonexistentTypeCache: false, + ExperimentalMetadataPrefetchOnMount: "sync", + }, + Metrics: cfg.MetricsConfig{ + StackdriverExportInterval: 40, + }, + Monitoring: cfg.MonitoringConfig{ + ExperimentalOpentelemetryCollectorAddress: "address", + }, + Debug: cfg.DebugConfig{ + ExitOnInvariantViolation: true, + Gcs: true, + LogMutex: true, + }, + }, + expectedErr: nil, + }, + { + testName: "Test decode legacy config.", + legacyFlagStorage: &flagStorage{ + ClientProtocol: mountpkg.GRPC, + }, + mockCLICtx: &mockCLIContext{isFlagSet: map[string]bool{}}, + legacyMountConfig: &config.MountConfig{ + WriteConfig: config.WriteConfig{ + CreateEmptyFile: true, + }, + LogConfig: config.LogConfig{ + Severity: "info", + Format: "text", + FilePath: "~/Documents/log-file.txt", + LogRotateConfig: config.LogRotateConfig{ + MaxFileSizeMB: 20, + BackupFileCount: 2, + Compress: true, + }, + }, + FileCacheConfig: config.FileCacheConfig{ + MaxSizeMB: 20, + CacheFileForRangeRead: true, + EnableParallelDownloads: true, + ParallelDownloadsPerFile: 3, + MaxParallelDownloads: 6, + DownloadChunkSizeMB: 9, + EnableCRC: true, + }, + CacheDir: "~/cache-dir", + MetadataCacheConfig: config.MetadataCacheConfig{ + TtlInSeconds: 200, + TypeCacheMaxSizeMB: 7, + StatCacheMaxSizeMB: 4, + }, + ListConfig: config.ListConfig{ + EnableEmptyManagedFolders: true, + KernelListCacheTtlSeconds: 30, + }, + GCSConnection: config.GCSConnection{GRPCConnPoolSize: 29}, + GCSAuth: config.GCSAuth{AnonymousAccess: true}, + EnableHNS: true, + FileSystemConfig: config.FileSystemConfig{ + IgnoreInterrupts: true, + DisableParallelDirops: true, + }, + }, + expectedConfig: &cfg.Config{ + Write: cfg.WriteConfig{CreateEmptyFile: true}, + Logging: cfg.LoggingConfig{ + Severity: "INFO", + Format: "text", + FilePath: cfg.ResolvedPath(path.Join(os.Getenv("HOME"), "Documents/log-file.txt")), + LogRotate: cfg.LogRotateLoggingConfig{ + MaxFileSizeMb: 20, + BackupFileCount: 2, + Compress: true, + }, + }, + FileCache: cfg.FileCacheConfig{ + MaxSizeMb: 20, + CacheFileForRangeRead: true, + EnableParallelDownloads: true, + ParallelDownloadsPerFile: 3, + MaxParallelDownloads: 6, + DownloadChunkSizeMb: 9, + EnableCrc: true, + }, + CacheDir: cfg.ResolvedPath(path.Join(os.Getenv("HOME"), "cache-dir")), + MetadataCache: cfg.MetadataCacheConfig{ + TtlSecs: 200, + TypeCacheMaxSizeMb: 7, + StatCacheMaxSizeMb: 4, + }, + List: cfg.ListConfig{ + EnableEmptyManagedFolders: true, + KernelListCacheTtlSecs: 30, + }, + GcsConnection: cfg.GcsConnectionConfig{ + GrpcConnPoolSize: 29, + ClientProtocol: cfg.Protocol("grpc")}, + GcsAuth: cfg.GcsAuthConfig{AnonymousAccess: true}, + EnableHns: true, + FileSystem: cfg.FileSystemConfig{ + DisableParallelDirops: true, + IgnoreInterrupts: true, + }, + }, + expectedErr: nil, + }, + { + testName: "Test overlapping flags and configs set.", + legacyFlagStorage: &flagStorage{ + LogFile: "~/Documents/log-flag.txt", + LogFormat: "json", + IgnoreInterrupts: false, + AnonymousAccess: false, + KernelListCacheTtlSeconds: -1, + ClientProtocol: mountpkg.HTTP2, + }, + mockCLICtx: &mockCLIContext{ + isFlagSet: map[string]bool{ + "log-file": true, + "log-format": true, + "ignore-interrupts": true, + "anonymous-access": true, + "kernel-list-cache-ttl-secs": true, + }, + }, + legacyMountConfig: &config.MountConfig{ + LogConfig: config.LogConfig{ + FilePath: "~/Documents/log-config.txt", + Format: "text", + Severity: "INFO", + }, + FileSystemConfig: config.FileSystemConfig{ + IgnoreInterrupts: true, + }, + GCSAuth: config.GCSAuth{ + AnonymousAccess: true, + }, + ListConfig: config.ListConfig{ + KernelListCacheTtlSeconds: 100, + }, + }, + expectedConfig: &cfg.Config{ + Logging: cfg.LoggingConfig{ + FilePath: cfg.ResolvedPath(path.Join(os.Getenv("HOME"), "/Documents/log-flag.txt")), + Format: "json", + Severity: "INFO", + }, + FileSystem: cfg.FileSystemConfig{ + IgnoreInterrupts: false, + }, + GcsAuth: cfg.GcsAuthConfig{ + AnonymousAccess: false, + }, + List: cfg.ListConfig{ + KernelListCacheTtlSecs: -1, + }, + GcsConnection: cfg.GcsConnectionConfig{ + ClientProtocol: cfg.Protocol("http2"), + }, + }, + expectedErr: nil, + }, + } + + for _, tc := range populateConfigFromLegacyFlags { + t.Run(tc.testName, func(t *testing.T) { + resolvedConfig, err := PopulateNewConfigFromLegacyFlagsAndConfig(tc.mockCLICtx, tc.legacyFlagStorage, tc.legacyMountConfig) + + if assert.Equal(t, tc.expectedErr, err) { + assert.Equal(t, tc.expectedConfig, resolvedConfig) + } + }) + } +} + +func TestOverrideWithFlag(t *testing.T) { + tests := []struct { + name string + flag string + isFlagSet bool + initialValue any + updateValue any + expected any + }{ + { + name: "flagSet", + flag: "log-file", + isFlagSet: true, + initialValue: "/tmp/log.txt", + updateValue: "/tmp/newLog.txt", + expected: "/tmp/newLog.txt", + }, + { + name: "flagNotSet", + flag: "ignore-interrupts", + isFlagSet: false, + initialValue: false, + updateValue: true, + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + toUpdate := tc.initialValue + mockC := &mockCLIContext{ + isFlagSet: map[string]bool{tc.flag: tc.isFlagSet}, + } + + overrideWithFlag(mockC, tc.flag, &toUpdate, tc.updateValue) + + assert.Equal(t, tc.expected, toUpdate) + }) + } +} diff --git a/tools/config-gen/params.yaml b/tools/config-gen/params.yaml index 201cdddd84..3d30761c5c 100644 --- a/tools/config-gen/params.yaml +++ b/tools/config-gen/params.yaml @@ -282,7 +282,7 @@ client-protocol is set to 'http1'. The default value 0 indicates no limit on TCP connections (limited by the machine specifications). default: "0" - + - flag-name: "max-idle-conns-per-host" config-path: "gcs-connection.max-idle-conns-per-host" type: "int" From ebe610c480550ba4b1e9a5aaf3b20ba221e4b8a9 Mon Sep 17 00:00:00 2001 From: Ashmeen Kaur <57195160+ashmeenkaur@users.noreply.github.com> Date: Thu, 27 Jun 2024 10:54:11 +0530 Subject: [PATCH 3/4] [CLI Config Parity] migrate key-file flag usage to new config (#2069) * remove key file flag usage * typo fix * review comment Co-authored-by: Kislay Kishore * review comment Co-authored-by: Kislay Kishore * remove unexported method test --------- Co-authored-by: Kislay Kishore --- cmd/flags.go | 5 --- cmd/flags_test.go | 6 +--- cmd/legacy_main.go | 17 ++++++--- cmd/legacy_main_test.go | 9 ++--- cmd/legacy_param_mapper_test.go | 62 +++++++++++++++++++-------------- 5 files changed, 53 insertions(+), 46 deletions(-) diff --git a/cmd/flags.go b/cmd/flags.go index 09340e251d..aa5c18a56a 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -490,11 +490,6 @@ func resolvePathForTheFlagsInContext(c *cli.Context) (err error) { return fmt.Errorf("resolving for log-file: %w", err) } - err = resolvePathForTheFlagInContext("key-file", c) - if err != nil { - return fmt.Errorf("resolving for key-file: %w", err) - } - err = resolvePathForTheFlagInContext("config-file", c) if err != nil { return fmt.Errorf("resolving for config-file: %w", err) diff --git a/cmd/flags_test.go b/cmd/flags_test.go index 1cf12d026c..75f6aae100 100644 --- a/cmd/flags_test.go +++ b/cmd/flags_test.go @@ -303,17 +303,13 @@ 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, "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", "--log-file=test.txt", "--config-file=config.yaml"} err = app.Run(fullArgs) diff --git a/cmd/legacy_main.go b/cmd/legacy_main.go index f36b4bcebf..3af07ac776 100644 --- a/cmd/legacy_main.go +++ b/cmd/legacy_main.go @@ -28,6 +28,7 @@ import ( "path/filepath" "strings" + "github.com/googlecloudplatform/gcsfuse/v2/cfg" "github.com/googlecloudplatform/gcsfuse/v2/internal/canned" "github.com/googlecloudplatform/gcsfuse/v2/internal/config" "github.com/googlecloudplatform/gcsfuse/v2/internal/locker" @@ -100,7 +101,7 @@ func getConfigForUserAgent(mountConfig *config.MountConfig) string { } return fmt.Sprintf("%s:%s", isFileCacheEnabled, isFileCacheForRangeReadEnabled) } -func createStorageHandle(flags *flagStorage, mountConfig *config.MountConfig, userAgent string) (storageHandle storage.StorageHandle, err error) { +func createStorageHandle(newConfig *cfg.Config, flags *flagStorage, mountConfig *config.MountConfig, userAgent string) (storageHandle storage.StorageHandle, err error) { storageClientConfig := storageutil.StorageClientConfig{ ClientProtocol: flags.ClientProtocol, MaxConnsPerHost: flags.MaxConnsPerHost, @@ -110,7 +111,7 @@ func createStorageHandle(flags *flagStorage, mountConfig *config.MountConfig, us RetryMultiplier: flags.RetryMultiplier, UserAgent: userAgent, CustomEndpoint: flags.CustomEndpoint, - KeyFile: flags.KeyFile, + KeyFile: string(newConfig.GcsAuth.KeyFile), AnonymousAccess: mountConfig.GCSAuth.AnonymousAccess, TokenUrl: flags.TokenUrl, ReuseTokenFromUrl: flags.ReuseTokenFromUrl, @@ -132,7 +133,8 @@ func mountWithArgs( bucketName string, mountPoint string, flags *flagStorage, - mountConfig *config.MountConfig) (mfs *fuse.MountedFileSystem, err error) { + mountConfig *config.MountConfig, + newConfig *cfg.Config) (mfs *fuse.MountedFileSystem, err error) { // Enable invariant checking if requested. if flags.DebugInvariants { locker.EnableInvariantsCheck() @@ -149,7 +151,7 @@ func mountWithArgs( if bucketName != canned.FakeBucketName { userAgent := getUserAgent(flags.AppName, getConfigForUserAgent(mountConfig)) logger.Info("Creating Storage handle...") - storageHandle, err = createStorageHandle(flags, mountConfig, userAgent) + storageHandle, err = createStorageHandle(newConfig, flags, mountConfig, userAgent) if err != nil { err = fmt.Errorf("Failed to create storage handle using createStorageHandle: %w", err) return @@ -251,6 +253,11 @@ func runCLIApp(c *cli.Context) (err error) { return fmt.Errorf("parsing config file failed: %w", err) } + newConfig, err := PopulateNewConfigFromLegacyFlagsAndConfig(c, flags, mountConfig) + if err != nil { + return fmt.Errorf("error resolving flags and configs: %w", err) + } + config.OverrideWithLoggingFlags(mountConfig, flags.LogFile, flags.LogFormat, flags.DebugFuse, flags.DebugGCS, flags.DebugMutex) config.OverrideWithIgnoreInterruptsFlag(c, mountConfig, flags.IgnoreInterrupts) @@ -404,7 +411,7 @@ func runCLIApp(c *cli.Context) (err error) { // daemonize gives us and telling it about the outcome. var mfs *fuse.MountedFileSystem { - mfs, err = mountWithArgs(bucketName, mountPoint, flags, mountConfig) + mfs, err = mountWithArgs(bucketName, mountPoint, flags, mountConfig, newConfig) // This utility is to absorb the error // returned by daemonize.SignalOutcome calls by simply diff --git a/cmd/legacy_main_test.go b/cmd/legacy_main_test.go index f6842ebf05..b47fc153dd 100644 --- a/cmd/legacy_main_test.go +++ b/cmd/legacy_main_test.go @@ -20,6 +20,7 @@ import ( "strings" "testing" + "github.com/googlecloudplatform/gcsfuse/v2/cfg" "github.com/googlecloudplatform/gcsfuse/v2/internal/config" "github.com/googlecloudplatform/gcsfuse/v2/internal/util" @@ -49,12 +50,12 @@ func (t *MainTest) TestCreateStorageHandle() { MaxRetrySleep: 7, RetryMultiplier: 2, AppName: "app", - KeyFile: "testdata/test_creds.json", } mountConfig := &config.MountConfig{} + newConfig := &cfg.Config{GcsAuth: cfg.GcsAuthConfig{KeyFile: "testdata/test_creds.json"}} userAgent := "AppName" - storageHandle, err := createStorageHandle(flags, mountConfig, userAgent) + storageHandle, err := createStorageHandle(newConfig, flags, mountConfig, userAgent) assert.Equal(t.T(), nil, err) assert.NotEqual(t.T(), nil, storageHandle) @@ -69,14 +70,14 @@ func (t *MainTest) TestCreateStorageHandle_WithClientProtocolAsGRPC() { MaxRetrySleep: 7, RetryMultiplier: 2, AppName: "app", - KeyFile: "testdata/test_creds.json", } mountConfig := &config.MountConfig{ GCSConnection: config.GCSConnection{GRPCConnPoolSize: 1}, } + newConfig := &cfg.Config{GcsAuth: cfg.GcsAuthConfig{KeyFile: "testdata/test_creds.json"}} userAgent := "AppName" - storageHandle, err := createStorageHandle(flags, mountConfig, userAgent) + storageHandle, err := createStorageHandle(newConfig, flags, mountConfig, userAgent) assert.Equal(t.T(), nil, err) assert.NotEqual(t.T(), nil, storageHandle) diff --git a/cmd/legacy_param_mapper_test.go b/cmd/legacy_param_mapper_test.go index 7f08770321..d1aa23deac 100644 --- a/cmd/legacy_param_mapper_test.go +++ b/cmd/legacy_param_mapper_test.go @@ -25,6 +25,7 @@ import ( "github.com/googlecloudplatform/gcsfuse/v2/internal/config" mountpkg "github.com/googlecloudplatform/gcsfuse/v2/internal/mount" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/urfave/cli" ) @@ -346,43 +347,50 @@ func TestPopulateConfigFromLegacyFlags(t *testing.T) { } } -func TestOverrideWithFlag(t *testing.T) { - tests := []struct { - name string - flag string - isFlagSet bool - initialValue any - updateValue any - expected any +func TestPopulateConfigFromLegacyFlags_KeyFileResolution(t *testing.T) { + currentWorkingDir, err := os.Getwd() + require.Nil(t, err) + var keyFileTests = []struct { + testName string + givenKeyFile string + expectedKeyFile cfg.ResolvedPath }{ { - name: "flagSet", - flag: "log-file", - isFlagSet: true, - initialValue: "/tmp/log.txt", - updateValue: "/tmp/newLog.txt", - expected: "/tmp/newLog.txt", + testName: "absolute path", + givenKeyFile: "/tmp/key-file.json", + expectedKeyFile: "/tmp/key-file.json", }, { - name: "flagNotSet", - flag: "ignore-interrupts", - isFlagSet: false, - initialValue: false, - updateValue: true, - expected: false, + testName: "relative path", + givenKeyFile: "~/Documents/key-file.json", + expectedKeyFile: cfg.ResolvedPath(path.Join(os.Getenv("HOME"), "/Documents/key-file.json")), + }, + { + testName: "current working directory", + givenKeyFile: "key-file.json", + expectedKeyFile: cfg.ResolvedPath(path.Join(currentWorkingDir, "key-file.json")), + }, + { + testName: "empty path", + givenKeyFile: "", + expectedKeyFile: "", }, } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - toUpdate := tc.initialValue - mockC := &mockCLIContext{ - isFlagSet: map[string]bool{tc.flag: tc.isFlagSet}, + for _, tc := range keyFileTests { + t.Run(tc.testName, func(t *testing.T) { + mockCLICtx := &mockCLIContext{} + legacyFlagStorage := &flagStorage{ + ClientProtocol: mountpkg.HTTP2, + KeyFile: tc.givenKeyFile, } + legacyMountCfg := &config.MountConfig{} - overrideWithFlag(mockC, tc.flag, &toUpdate, tc.updateValue) + resolvedConfig, err := PopulateNewConfigFromLegacyFlagsAndConfig(mockCLICtx, legacyFlagStorage, legacyMountCfg) - assert.Equal(t, tc.expected, toUpdate) + if assert.Nil(t, err) { + assert.Equal(t, tc.expectedKeyFile, resolvedConfig.GcsAuth.KeyFile) + } }) } } 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 4/4] [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.