chore: [cache-dir-size-fix] Part 2: Add configuration flags and parsing for file cache size fix

[gargnitingoogle]

Description

• Dependent on chore: [cache-dir-size-fix] Part 1: Add disk-size utilities and shared-dir locker utilities #4414
• Adds new gcsfuse parameters for testing
• Dependency for chore: [cache-dir-size-fix] Part 3: Add disk size calculator for file cache #4416

Link to the issue in case of a bug fix.

b/477828938

Testing details

1. Manual - NA
2. Unit tests - NA
3. Integration tests - NA

Any backward incompatible change? If so, please explain.

[gargnitingoogle]

/gemini review

[gargnitingoogle]

/gemini summary

[gemini-code-assist]

Code Review

This pull request introduces new configuration flags for a file cache size scan feature. The changes include updating the configuration struct, adding and parsing command-line flags, and implementing associated validation and rationalization logic. The implementation is clear and includes new tests for argument parsing and config rationalization. My primary feedback is to enhance the test coverage for the new validation logic to ensure all scenarios are properly handled.

[gemini-code-assist]

The new test TestValidateFileCacheConfig_SizeScanDeleteEmptyDirs is a good start, but it only covers one of the new validation scenarios for the size scan feature. To ensure robustness, it would be beneficial to expand this test to cover all the new validation logic added in isValidFileCacheConfig.

I'd suggest renaming the test to something more general, like TestValidateFileCacheConfig_SizeScanFlags, and adding test cases for the following scenarios:

• SizeScanEnable=true with SizeScanFrequencySeconds=0 should return an error.
• SizeScanEnable=false with SizeScanFiles=true should return an error.
• SizeScanEnable=false with SizeScanFrequencySeconds explicitly set to a non-zero value should return an error. This requires setting the value in the viper instance passed to ValidateConfig.

Here's an example for the last case:

// In a new test case
v := viper.New()
v.Set("file-cache.size-scan-frequency-seconds", 5)
c := validConfig(t)
c.FileCache.SizeScanEnable = false
c.FileCache.SizeScanFrequencySeconds = 5

err := ValidateConfig(v, &c)

assert.Error(t, err)

This will make the tests for the new validation logic more comprehensive.

[gemini-code-assist]

Summary of Changes

This pull request, part two of a cache directory size fix, establishes the foundational configuration and parsing mechanisms for a new file cache size management feature. It introduces several new flags that allow users to precisely control how the file cache directory is scanned for size, including options for enabling/disabling the scan, specifying scan frequency, and managing empty directories. This work lays the groundwork for the actual implementation of the size scanning functionality.

Highlights

• New Configuration Parameters: Introduced new configuration parameters for controlling file cache size scanning, including enabling/disabling the scan, scanning files, deleting empty directories, and setting scan frequency.
• Parsing Logic for New Flags: Implemented parsing logic for the newly added file cache size scan flags in the command-line interface.
• Validation Rules: Added validation rules to ensure consistency and correctness of the file cache size scan configurations, such as requiring a positive scan frequency when enabled.
• Expanded Test Coverage: Expanded unit and integration tests to cover the new configuration parameters, their parsing, and validation logic.

Changelog

• cfg/config.go
  • Added SizeScanDeleteEmptyDirs, SizeScanEnable, SizeScanFiles, and SizeScanFrequencySeconds fields to the FileCacheConfig struct.
  • Registered new command-line flags for these file cache size scan parameters.
• cfg/config_util.go
  • Introduced DefaultFileCacheSizeScanFrequencySeconds constant.
• cfg/params.yaml
  • Defined new YAML parameters for file-cache.size-scan-delete-empty-dirs, file-cache.size-scan-enable, file-cache.size-scan-files, and file-cache.size-scan-frequency-seconds.
• cfg/rationalize.go
  • Added resolveSizeScanEnableValue function to conditionally disable size scanning if MaxSizeMb is -1 or file cache is not enabled.
  • Integrated resolveSizeScanEnableValue into the Rationalize function.
• cfg/rationalize_test.go
  • Added TestRationalize_SizeScanEnableValue to test the conditional disabling of size scanning.
• cfg/validate.go
  • Modified isValidFileCacheConfig to accept a viper.Viper instance.
  • Implemented new validation logic for file cache size scan flags, ensuring dependencies and valid ranges (e.g., frequency > 0 when enabled).
• cfg/validate_test.go
  • Added new test cases to validate the file cache size scan configuration logic.
• cmd/config_validation_test.go
  • Updated defaultFileCacheConfig and TestValidateConfigFile_FileCacheConfigSuccessful to include the new size scan parameters.
• cmd/root_test.go
  • Updated TestArgsParsing_FileCacheFlags to include new size scan parameters in expected configurations.
  • Added TestArgsParsing_FileCacheSizeScanFlags to specifically test argument parsing and validation for the new size scan flags.

Activity

• The author, gargnitingoogle, requested a Gemini review.
• The author, gargnitingoogle, requested a Gemini summary.
• gemini-code-assist[bot] provided a review comment suggesting to expand the test TestValidateFileCacheConfig_SizeScanDeleteEmptyDirs to cover all new validation scenarios for the size scan feature, including cases for SizeScanEnable=true with SizeScanFrequencySeconds=0, SizeScanEnable=false with SizeScanFiles=true, and SizeScanEnable=false with SizeScanFrequencySeconds explicitly set to a non-zero value.