Enforce file cache availability for parallel downloads through config… (

#2772) * 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 <princer@google.com> * resolve merge conflict * Used utility function to check whether file cache is enabled --------- Co-authored-by: Prince Kumar <princer@google.com>
GoogleCloudPlatform · Dec 11, 2024 · f75b454 · f75b454
1 parent 94a9357
commit f75b454
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 16 deletions.
diff --git a/cfg/validate.go b/cfg/validate.go
@@ -17,6 +17,7 @@ package cfg
 import (
 	"errors"
 	"fmt"
+
 	"math"
 )
 
@@ -43,29 +44,37 @@ 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)
 	}
 	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
@@ -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
@@ -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
@@ -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
@@ -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