From f75b45499577ae41e118b20afa4731b7d091a54c Mon Sep 17 00:00:00 2001 From: codechanges Date: Wed, 11 Dec 2024 10:29:36 +0530 Subject: [PATCH] =?UTF-8?q?Enforce=20file=20cache=20availability=20for=20p?= =?UTF-8?q?arallel=20downloads=20through=20config=E2=80=A6=20(#2772)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Enforce file cache availability for parallel downloads through config validation * update valid config yaml to disable parallel download * Added requisite import * Amend code to match with the latest commit changes * fix broken tests * Update cfg/validate_test.go Accepted Co-authored-by: Prince Kumar * resolve merge conflict * Used utility function to check whether file cache is enabled --------- Co-authored-by: Prince Kumar --- cfg/validate.go | 39 ++++++++++++++++++++----------- cfg/validate_test.go | 42 ++++++++++++++++++++++++++++++++++ cmd/config_validation_test.go | 2 +- cmd/root_test.go | 3 ++- cmd/testdata/valid_config.yaml | 2 +- 5 files changed, 72 insertions(+), 16 deletions(-) diff --git a/cfg/validate.go b/cfg/validate.go index a634b832e9..161a0c55de 100644 --- a/cfg/validate.go +++ b/cfg/validate.go @@ -17,6 +17,7 @@ package cfg import ( "errors" "fmt" + "math" ) @@ -43,6 +44,25 @@ func isValidURL(u string) error { return err } +func isValidParallelDownloadConfig(config *Config) error { + if config.FileCache.EnableParallelDownloads { + if !IsFileCacheEnabled(config) { + return errors.New("file cache should be enabled for parallel download support") + } + if config.FileCache.MaxParallelDownloads == 0 { + return errors.New("the value of max-parallel-downloads for file-cache must not be 0 when enable-parallel-downloads is true") + } + if config.FileCache.WriteBufferSize < CacheUtilMinimumAlignSizeForWriting { + return errors.New("the value of write-buffer-size for file-cache can't be less than 4096") + } + if (config.FileCache.WriteBufferSize % CacheUtilMinimumAlignSizeForWriting) != 0 { + return errors.New("the value of write-buffer-size for file-cache should be in multiple of 4096") + } + } + + return nil +} + func isValidFileCacheConfig(config *FileCacheConfig) error { if config.MaxSizeMb < -1 { return errors.New(FileCacheMaxSizeMBInvalidValueError) @@ -50,22 +70,11 @@ func isValidFileCacheConfig(config *FileCacheConfig) error { if config.MaxParallelDownloads < -1 { return errors.New(MaxParallelDownloadsInvalidValueError) } - if config.EnableParallelDownloads { - if config.MaxParallelDownloads == 0 { - return errors.New(MaxParallelDownloadsCantBeZeroError) - } - if config.WriteBufferSize < CacheUtilMinimumAlignSizeForWriting { - return errors.New("the value of write-buffer-size for file-cache can't be less than 4096") - } - if (config.WriteBufferSize % CacheUtilMinimumAlignSizeForWriting) != 0 { - return errors.New("the value of write-buffer-size for file-cache should be in multiple of 4096") - } - } if config.ParallelDownloadsPerFile < 1 { - return fmt.Errorf(ParallelDownloadsPerFileInvalidValueError) + return errors.New(ParallelDownloadsPerFileInvalidValueError) } if config.DownloadChunkSizeMb < 1 { - return fmt.Errorf(DownloadChunkSizeMBInvalidValueError) + return errors.New(DownloadChunkSizeMBInvalidValueError) } return nil @@ -243,5 +252,9 @@ func ValidateConfig(v isSet, config *Config) error { return fmt.Errorf("error parsing metrics config: %w", err) } + if err = isValidParallelDownloadConfig(config); err != nil { + return fmt.Errorf("error parsing parallel download config: %w", err) + } + return nil } diff --git a/cfg/validate_test.go b/cfg/validate_test.go index 550695f669..dafbb19127 100644 --- a/cfg/validate_test.go +++ b/cfg/validate_test.go @@ -156,6 +156,28 @@ func TestValidateConfigSuccessful(t *testing.T) { FileSystem: FileSystemConfig{KernelListCacheTtlSecs: 30}, }, }, + { + name: "valid_parallel_download_config_with_file_cache_enabled", + config: &Config{ + Logging: LoggingConfig{LogRotate: validLogRotateConfig()}, + CacheDir: "/some/valid/path", + FileCache: FileCacheConfig{ + DownloadChunkSizeMb: 50, + EnableParallelDownloads: true, + MaxParallelDownloads: 4, + ParallelDownloadsPerFile: 16, + MaxSizeMb: -1, + WriteBufferSize: 4 * 1024 * 1024, + }, + GcsConnection: GcsConnectionConfig{ + CustomEndpoint: "https://bing.com/search?q=dotnet", + SequentialReadSizeMb: 200, + }, + MetadataCache: MetadataCacheConfig{ + ExperimentalMetadataPrefetchOnMount: "disabled", + }, + }, + }, { name: "valid_chunk_transfer_timeout_secs", config: &Config{ @@ -341,6 +363,26 @@ func TestValidateConfig_ErrorScenarios(t *testing.T) { }, }, }, + { + name: "parallel_download_config_without_file_cache_enabled", + config: &Config{ + Logging: LoggingConfig{LogRotate: validLogRotateConfig()}, + FileCache: FileCacheConfig{ + DownloadChunkSizeMb: 50, + EnableParallelDownloads: true, + MaxParallelDownloads: 4, + ParallelDownloadsPerFile: 16, + WriteBufferSize: 4 * 1024 * 1024, + }, + GcsConnection: GcsConnectionConfig{ + CustomEndpoint: "https://bing.com/search?q=dotnet", + SequentialReadSizeMb: 200, + }, + MetadataCache: MetadataCacheConfig{ + ExperimentalMetadataPrefetchOnMount: "disabled", + }, + }, + }, { name: "chunk_transfer_timeout_in_negative", config: &Config{ diff --git a/cmd/config_validation_test.go b/cmd/config_validation_test.go index 0fef30bcb6..e5501e49ff 100644 --- a/cmd/config_validation_test.go +++ b/cmd/config_validation_test.go @@ -355,7 +355,7 @@ func TestValidateConfigFile_FileCacheConfigSuccessful(t *testing.T) { CacheFileForRangeRead: true, DownloadChunkSizeMb: 300, EnableCrc: true, - EnableParallelDownloads: true, + EnableParallelDownloads: false, MaxParallelDownloads: 200, MaxSizeMb: 40, ParallelDownloadsPerFile: 10, diff --git a/cmd/root_test.go b/cmd/root_test.go index fef6ba8360..40a28412ad 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -306,8 +306,9 @@ func TestArgsParsing_FileCacheFlags(t *testing.T) { }{ { name: "Test file cache flags.", - args: []string{"gcsfuse", "--file-cache-cache-file-for-range-read", "--file-cache-download-chunk-size-mb=20", "--file-cache-enable-crc", "--file-cache-enable-parallel-downloads", "--file-cache-max-parallel-downloads=40", "--file-cache-max-size-mb=100", "--file-cache-parallel-downloads-per-file=2", "--file-cache-enable-o-direct=false", "abc", "pqr"}, + args: []string{"gcsfuse", "--file-cache-cache-file-for-range-read", "--file-cache-download-chunk-size-mb=20", "--file-cache-enable-crc", "--cache-dir=/some/valid/dir", "--file-cache-enable-parallel-downloads", "--file-cache-max-parallel-downloads=40", "--file-cache-max-size-mb=100", "--file-cache-parallel-downloads-per-file=2", "--file-cache-enable-o-direct=false", "abc", "pqr"}, expectedConfig: &cfg.Config{ + CacheDir: "/some/valid/dir", FileCache: cfg.FileCacheConfig{ CacheFileForRangeRead: true, DownloadChunkSizeMb: 20, diff --git a/cmd/testdata/valid_config.yaml b/cmd/testdata/valid_config.yaml index 134685b527..e8414deee8 100644 --- a/cmd/testdata/valid_config.yaml +++ b/cmd/testdata/valid_config.yaml @@ -9,7 +9,7 @@ file-cache: cache-file-for-range-read: true download-chunk-size-mb: 300 enable-crc: true - enable-parallel-downloads: true + enable-parallel-downloads: false max-parallel-downloads: 200 max-size-mb: 40 parallel-downloads-per-file: 10