Rename log rotate config (#1520)

* change file-count to backup-file-count * remove 0 value constraint from backup-file-count * review comment to add unit test for backup file count 0 * review comments
GoogleCloudPlatform · Jan 4, 2024 · e804154 · e804154
1 parent db3a710
commit e804154
Show file tree

Hide file tree

Showing 12 changed files with 62 additions and 34 deletions.
diff --git a/internal/config/mount_config.go b/internal/config/mount_config.go
@@ -18,9 +18,9 @@ import "math"
 
 const (
 	// Default log rotation config values.
-	defaultMaxFileSizeMB = 512
-	defaultFileCount     = 10
-	defaultCompress      = false
+	defaultMaxFileSizeMB   = 512
+	defaultBackupFileCount = 10
+	defaultCompress        = false
 
 	// TtlInSecsUnsetSentinel is the value internally
 	// set for metada-cache:ttl-secs
@@ -69,21 +69,22 @@ type MountConfig struct {
 // configuration options:
 // 1. max-file-size-mb: specifies the maximum size in megabytes that a log file
 // can reach before it is rotated. The default value is 512 megabytes.
-// 2. file-count: determines the maximum number of log files to retain after they
-// have been rotated. The default value is 10.
+// 2. backup-file-count: determines the maximum number of backup log files to
+// retain after they have been rotated. The default value is 10. When value is
+// set to 0, all backup files are retained.
 // 3. compress: indicates whether the rotated log files should be compressed
 // using gzip. The default value is False.
 type LogRotateConfig struct {
-	MaxFileSizeMB int  `yaml:"max-file-size-mb"`
-	FileCount     int  `yaml:"file-count"`
-	Compress      bool `yaml:"compress"`
+	MaxFileSizeMB   int  `yaml:"max-file-size-mb"`
+	BackupFileCount int  `yaml:"backup-file-count"`
+	Compress        bool `yaml:"compress"`
 }
 
 func DefaultLogRotateConfig() LogRotateConfig {
 	return LogRotateConfig{
-		MaxFileSizeMB: defaultMaxFileSizeMB,
-		FileCount:     defaultFileCount,
-		Compress:      defaultCompress,
+		MaxFileSizeMB:   defaultMaxFileSizeMB,
+		BackupFileCount: defaultBackupFileCount,
+		Compress:        defaultCompress,
 	}
 }
 

diff --git a/internal/config/testdata/invalid_log_rotate_config_1.yaml b/internal/config/testdata/invalid_log_rotate_config_1.yaml
@@ -4,4 +4,4 @@ logging:
   severity: error
   log-rotate:
     max-file-size-mb: -1
-    file-count: -1
+    backup-file-count: -1
diff --git a/internal/config/testdata/invalid_log_rotate_config_2.yaml b/internal/config/testdata/invalid_log_rotate_config_2.yaml
@@ -3,4 +3,4 @@ write:
 logging:
   severity: error
   log-rotate:
-    file-count: -1
+    backup-file-count: -1
diff --git a/internal/config/testdata/valid_config.yaml b/internal/config/testdata/valid_config.yaml
@@ -6,7 +6,7 @@ logging:
   severity: error
   log-rotate:
     max-file-size-mb: 100
-    file-count: 5
+    backup-file-count: 5
     compress: false
 cache-location: "/tmp/read_cache/"
 file-cache:

diff --git a/internal/config/testdata/valid_config_with_0_backup-file-count.yaml b/internal/config/testdata/valid_config_with_0_backup-file-count.yaml
@@ -0,0 +1,12 @@
+write:
+  create-empty-file: true
+logging:
+  file-path: /tmp/logfile.json
+  format: text
+  severity: error
+  log-rotate:
+    max-file-size-mb: 100
+    backup-file-count: 0 # retain all backup files
+    compress: false
+
+
diff --git a/internal/config/yaml_parser.go b/internal/config/yaml_parser.go
@@ -56,8 +56,8 @@ func IsValidLogRotateConfig(config LogRotateConfig) error {
 	if config.MaxFileSizeMB <= 0 {
 		return fmt.Errorf("max-file-size-mb should be atleast 1")
 	}
-	if config.FileCount <= 0 {
-		return fmt.Errorf("file-count should be atleast 1")
+	if config.BackupFileCount < 0 {
+		return fmt.Errorf("backup-file-count should be 0 (to retain all backup files) or a positive value")
 	}
 	return nil
 }

diff --git a/internal/config/yaml_parser_test.go b/internal/config/yaml_parser_test.go
@@ -37,7 +37,7 @@ func validateDefaultConfig(mountConfig *MountConfig) {
 	ExpectEq("", mountConfig.LogConfig.Format)
 	ExpectEq("", mountConfig.LogConfig.FilePath)
 	ExpectEq(512, mountConfig.LogConfig.LogRotateConfig.MaxFileSizeMB)
-	ExpectEq(10, mountConfig.LogConfig.LogRotateConfig.FileCount)
+	ExpectEq(10, mountConfig.LogConfig.LogRotateConfig.BackupFileCount)
 	ExpectEq(false, mountConfig.LogConfig.LogRotateConfig.Compress)
 	AssertEq("", mountConfig.CacheLocation)
 	AssertEq(0, mountConfig.FileCacheConfig.MaxSizeInMB)
@@ -72,6 +72,20 @@ func (t *YamlParserTest) TestReadConfigFile_InvalidConfig() {
 	AssertTrue(strings.Contains(err.Error(), "error parsing config file: yaml: unmarshal errors:"))
 }
 
+func (t *YamlParserTest) TestReadConfigFile_ValidConfigWith0BackupFileCount() {
+	mountConfig, err := ParseConfigFile("testdata/valid_config_with_0_backup-file-count.yaml")
+
+	AssertEq(nil, err)
+	AssertNe(nil, mountConfig)
+	ExpectEq(true, mountConfig.WriteConfig.CreateEmptyFile)
+	ExpectEq(ERROR, mountConfig.LogConfig.Severity)
+	ExpectEq("/tmp/logfile.json", mountConfig.LogConfig.FilePath)
+	ExpectEq("text", mountConfig.LogConfig.Format)
+	ExpectEq(100, mountConfig.LogConfig.LogRotateConfig.MaxFileSizeMB)
+	ExpectEq(0, mountConfig.LogConfig.LogRotateConfig.BackupFileCount)
+	ExpectEq(false, mountConfig.LogConfig.LogRotateConfig.Compress)
+}
+
 func (t *YamlParserTest) TestReadConfigFile_Invalid_UnexpectedField_Config() {
 	_, err := ParseConfigFile("testdata/invalid_unexpectedfield_config.yaml")
 
@@ -90,7 +104,7 @@ func (t *YamlParserTest) TestReadConfigFile_ValidConfig() {
 	ExpectEq("/tmp/logfile.json", mountConfig.LogConfig.FilePath)
 	ExpectEq("text", mountConfig.LogConfig.Format)
 	ExpectEq(100, mountConfig.LogConfig.LogRotateConfig.MaxFileSizeMB)
-	ExpectEq(5, mountConfig.LogConfig.LogRotateConfig.FileCount)
+	ExpectEq(5, mountConfig.LogConfig.LogRotateConfig.BackupFileCount)
 	ExpectEq(false, mountConfig.LogConfig.LogRotateConfig.Compress)
 
 	// metadata-cache config
@@ -118,7 +132,7 @@ func (t *YamlParserTest) TestReadConfigFile_InvalidLogRotateConfig2() {
 
 	AssertNe(nil, err)
 	AssertTrue(strings.Contains(err.Error(),
-		fmt.Sprintf(parseConfigFileErrMsgFormat, "file-count should be atleast 1")))
+		fmt.Sprintf(parseConfigFileErrMsgFormat, "backup-file-count should be 0 (to retain all backup files) or a positive value")))
 }
 
 func (t *YamlParserTest) TestReadConfigFile_InvalidFileCacheMaxSizeConfig() {

diff --git a/internal/logger/logger.go b/internal/logger/logger.go
@@ -63,7 +63,7 @@ func InitLogFile(logConfig config.LogConfig) error {
 		fileWriter = &lumberjack.Logger{
 			Filename:   f.Name(),
 			MaxSize:    logConfig.LogRotateConfig.MaxFileSizeMB,
-			MaxBackups: logConfig.LogRotateConfig.FileCount - 1,
+			MaxBackups: logConfig.LogRotateConfig.BackupFileCount,
 			Compress:   logConfig.LogRotateConfig.Compress,
 		}
 	} else {

diff --git a/internal/logger/logger_test.go b/internal/logger/logger_test.go
@@ -268,15 +268,15 @@ func (t *LoggerTest) TestInitLogFile() {
 	filePath, _ := os.UserHomeDir()
 	filePath += "/log.txt"
 	fileSize := 100
-	fileCount := 2
+	backupFileCount := 2
 	logConfig := config.LogConfig{
 		Severity: config.DEBUG,
 		Format:   format,
 		FilePath: filePath,
 		LogRotateConfig: config.LogRotateConfig{
-			MaxFileSizeMB: fileSize,
-			FileCount:     fileCount,
-			Compress:      true,
+			MaxFileSizeMB:   fileSize,
+			BackupFileCount: backupFileCount,
+			Compress:        true,
 		},
 	}
 
@@ -288,6 +288,6 @@ func (t *LoggerTest) TestInitLogFile() {
 	ExpectEq(format, defaultLoggerFactory.format)
 	ExpectEq(config.DEBUG, defaultLoggerFactory.level)
 	ExpectEq(fileSize, defaultLoggerFactory.logRotateConfig.MaxFileSizeMB)
-	ExpectEq(fileCount, defaultLoggerFactory.logRotateConfig.FileCount)
+	ExpectEq(backupFileCount, defaultLoggerFactory.logRotateConfig.BackupFileCount)
 	ExpectEq(true, defaultLoggerFactory.logRotateConfig.Compress)
 }
diff --git a/tools/integration_tests/log_rotation/log_rotation_test.go b/tools/integration_tests/log_rotation/log_rotation_test.go
@@ -39,16 +39,16 @@ const (
 var logDirPath string
 var logFilePath string
 
-func getMountConfigForLogRotation(maxFileSizeMB, fileCount int, compress bool,
+func getMountConfigForLogRotation(maxFileSizeMB, backupFileCount int, compress bool,
 	logFilePath string) config.MountConfig {
 	mountConfig := config.MountConfig{
 		LogConfig: config.LogConfig{
 			Severity: config.TRACE,
 			FilePath: logFilePath,
 			LogRotateConfig: config.LogRotateConfig{
-				MaxFileSizeMB: maxFileSizeMB,
-				FileCount:     fileCount,
-				Compress:      compress,
+				MaxFileSizeMB:   maxFileSizeMB,
+				BackupFileCount: backupFileCount,
+				Compress:        compress,
 			},
 		},
 	}
@@ -79,11 +79,12 @@ func TestMain(m *testing.M) {
 	logFilePath = path.Join(logDirPath, logFileName)
 
 	// Set up config files.
+	// TODO: add tests for backupLogFileCount = 0.
 	configFile1 := setup.YAMLConfigFile(
-		getMountConfigForLogRotation(maxFileSizeMB, logFileCount, true, logFilePath),
+		getMountConfigForLogRotation(maxFileSizeMB, backupLogFileCount, true, logFilePath),
 		"config1.yaml")
 	configFile2 := setup.YAMLConfigFile(
-		getMountConfigForLogRotation(maxFileSizeMB, logFileCount, false, logFilePath),
+		getMountConfigForLogRotation(maxFileSizeMB, backupLogFileCount, false, logFilePath),
 		"config2.yaml")
 
 	// Set up flags to run tests on.

diff --git a/tools/integration_tests/operations/operations_test.go b/tools/integration_tests/operations/operations_test.go
@@ -96,8 +96,8 @@ func createMountConfigsAndEquivalentFlags() (flags [][]string) {
 		LogConfig: config.LogConfig{
 			Severity: config.TRACE,
 			LogRotateConfig: config.LogRotateConfig{
-				MaxFileSizeMB: 10,
-				FileCount:     1,
+				MaxFileSizeMB:   10,
+				BackupFileCount: 0, // to retain all backup log files.
 			},
 		},
 	}

diff --git a/tools/integration_tests/run_tests_mounted_directory.sh b/tools/integration_tests/run_tests_mounted_directory.sh
@@ -298,7 +298,7 @@ echo "logging:
         severity: trace
         log-rotate:
           max-file-size-mb: 2
-          file-count: 3
+          backup-file-count: 3
           compress: true
        " > /tmp/gcsfuse_config.yaml
 gcsfuse --config-file=/tmp/gcsfuse_config.yaml $TEST_BUCKET_NAME $MOUNT_DIR