From f7a719ec14b8a923b2656586efac6e33940357b7 Mon Sep 17 00:00:00 2001 From: Ashmeen Kaur <57195160+ashmeenkaur@users.noreply.github.com> Date: Mon, 27 Nov 2023 17:20:34 +0530 Subject: [PATCH] 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 --- internal/config/mount_config.go | 23 ++++++++++--------- .../testdata/invalid_log_rotate_config_1.yaml | 2 +- .../testdata/invalid_log_rotate_config_2.yaml | 2 +- internal/config/testdata/valid_config.yaml | 2 +- ...valid_config_with_0_backup-file-count.yaml | 12 ++++++++++ internal/config/yaml_parser.go | 4 ++-- internal/config/yaml_parser_test.go | 20 +++++++++++++--- internal/logger/logger.go | 2 +- internal/logger/logger_test.go | 10 ++++---- .../log_rotation/log_rotation_test.go | 13 ++++++----- .../operations/operations_test.go | 4 ++-- .../run_tests_mounted_directory.sh | 2 +- 12 files changed, 62 insertions(+), 34 deletions(-) create mode 100644 internal/config/testdata/valid_config_with_0_backup-file-count.yaml diff --git a/internal/config/mount_config.go b/internal/config/mount_config.go index 75478a981c..73f062ed4a 100644 --- a/internal/config/mount_config.go +++ b/internal/config/mount_config.go @@ -16,9 +16,9 @@ package config const ( // Default log rotation config values. - defaultMaxFileSizeMB = 512 - defaultFileCount = 10 - defaultCompress = false + defaultMaxFileSizeMB = 512 + defaultBackupFileCount = 10 + defaultCompress = false ) type WriteConfig struct { @@ -41,21 +41,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 index ec98e2fc9f..a0e44e580f 100644 --- 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 index 2d41d5269b..691aa578bc 100644 --- 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 index 83d71d0dcb..1c34706442 100644 --- 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 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 new file mode 100644 index 0000000000..f2121639c1 --- /dev/null +++ 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 index abe40cb962..42eb15c088 100644 --- a/internal/config/yaml_parser.go +++ b/internal/config/yaml_parser.go @@ -55,8 +55,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 index 89e160dd00..37aa0464f8 100644 --- a/internal/config/yaml_parser_test.go +++ b/internal/config/yaml_parser_test.go @@ -36,7 +36,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) } @@ -68,6 +68,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") @@ -86,7 +100,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) } @@ -111,5 +125,5 @@ 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"))) } diff --git a/internal/logger/logger.go b/internal/logger/logger.go index d57448c4bf..1d604f0051 100644 --- 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 index 740c5bdc7d..e78c64e093 100644 --- 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 index a49582a9d0..25381484f6 100644 --- 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 index 565823c63c..670c9f8f71 100644 --- a/tools/integration_tests/operations/operations_test.go +++ b/tools/integration_tests/operations/operations_test.go @@ -108,8 +108,8 @@ func TestMain(m *testing.M) { 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 index 8537e16408..e51727f891 100755 --- a/tools/integration_tests/run_tests_mounted_directory.sh +++ b/tools/integration_tests/run_tests_mounted_directory.sh @@ -260,7 +260,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