Update utils_test.go and add more comments

Author: yaozile123

test/e2e/e2e_test.go

@@ -90,33 +90,6 @@ var _ = func() bool {
 	return true
 }()
 
-// fetchGCSFuseVersion initiates a minimal, standalone framework to query the cluster sidecar
-// during the init() phase. This enables dynamic test tree construction depending on the
-// version found in the remote cluster.
-// func fetchGCSFuseVersion() string {
-// 	config, err := framework.LoadConfig()
-// 	if err != nil {
-// 		klog.Fatalf("failed to load kube config for standalone version fetch: %v", err)
-// 	}
-// 	k8sClient, err := k8sclientset.NewForConfig(config)
-// 	if err != nil {
-// 		klog.Fatalf("failed to create client for standalone version fetch: %v", err)
-// 	}
-
-// 	minimalFramework := &framework.Framework{
-// 		ClientSet: k8sClient,
-// 		Namespace: &corev1.Namespace{
-// 			ObjectMeta: metav1.ObjectMeta{
-// 				Name: "default",
-// 			},
-// 		},
-// 	}
-// 	ctx := context.Background()
-// 	versionStr := specs.GetGCSFuseVersion(ctx, minimalFramework)
-// 	klog.Infof("Fetched GCSFuse version globally: %s", versionStr)
-// 	return versionStr
-// }
-
 func TestE2E(t *testing.T) {
 	t.Parallel()
 

test/e2e/testsuites/gcsfuse_integration.go

@@ -180,7 +180,7 @@ func (t *gcsFuseCSIGCSFuseIntegrationTestSuite) DefineTests(driver storageframew
 		v, branch := utils.GCSFuseBranch(gcsfuseVersionStr)
 
 		// Rename_symlink tests are in separat test package only of v2.11.4 for now
-		if testName == testNameRenameSymlink && (v.AtLeast(version.MustParseSemantic("v3.0.0-gke.0")) || v.LessThan(version.MustParseSemantic("v2.11.4-gke.0")) || branch == utils.MasterBranchName) {
+		if testName == testNameRenameSymlink && (branch == utils.MasterBranchName || (v.AtLeast(version.MustParseSemantic("v3.0.0-gke.0")) || v.LessThan(version.MustParseSemantic("v2.11.4-gke.0")))) {
 			e2eskipper.Skipf("skip gcsfuse integration rename_symlink test on gcsfuse version %v", v.String())
 		}
 

test/e2e/testsuites/gcsfuse_integration_file_cache_parallel_downloads.go

@@ -112,6 +112,7 @@ func (t *gcsFuseCSIGCSFuseIntegrationFileCacheParallelDownloadsTestSuite) Define
 		return branch
 	}
 
+	// TODO: Remove this once all supported GCSFuse versions support test_config.yaml.
 	gcsfuseIntegrationFileCacheTest := func(testName string, readOnly bool, fileCacheCapacity, fileCacheForRangeRead, metadataCacheTTLSeconds string, mountOptions ...string) {
 		ginkgo.By("Checking GCSFuse version and skip test if needed")
 		ginkgo.By(fmt.Sprintf("Running integration test %v with GCSFuse version %v", testName, GCSFuseVersionStr))
@@ -328,6 +329,7 @@ func (t *gcsFuseCSIGCSFuseIntegrationFileCacheParallelDownloadsTestSuite) Define
 		}
 	}
 
+	// TODO: Remove this once all supported GCSFuse versions support test_config.yaml.
 	generateStaticTests := func() {
 		// The following test cases are derived from https://github.com/GoogleCloudPlatform/gcsfuse/blob/master/tools/integration_tests/run_tests_mounted_directory.sh
 
@@ -467,10 +469,9 @@ func (t *gcsFuseCSIGCSFuseIntegrationFileCacheParallelDownloadsTestSuite) Define
 
 	framework.Logf("Generating tests based on test config")
 
-	// The gcsfuse test_config.yaml is introduced from the gcsfuse v3.5+.
-	// If the gcsfuse version is less than v3.5+, we will use the static tests.
+	// The gcsfuse test_config.yaml is introduced from the gcsfuse v3.7+.
+	// If the gcsfuse version is less than v3.7+, we will use the static tests.
 	// We will remove the static tests in the future.
-
 	if utils.IsReadFromTestConfig(GCSFuseVersionStr) {
 		klog.Info("Generating tests based on test config")
 		generateDynamicTests(GCSFuseVersionStr)

test/e2e/utils/handler.go

@@ -298,7 +298,7 @@ func Handle(testParams *TestParameters) error {
 
 	gcsfuseVersion, err := FetchGCSFuseVersion(context.Background(), k8sclientset)
 	if err != nil {
-		klog.Warningf("Failed to fetch GCSFuse version from cluster: %v. Tests might skip or fail if depending on this version.", err)
+		return fmt.Errorf("failed to fetch GCSFuse version from cluster: %v. Tests might skip or fail if depending on this version.", err)
 	} else {
 		klog.Infof("Fetched GCSFuse version from cluster: %s", gcsfuseVersion)
 		// Exporting the version so Ginkgo specs can read it

test/e2e/utils/utils.go

@@ -43,7 +43,7 @@ import (
 const (
 	DefaultNamespace              = "default"
 	MinGCSFuseKernelParamsVersion = "v3.7.0-gke.0"
-	MinGCSFuseTestConfigVersion   = "v3.5.0-gke.0"
+	MinGCSFuseTestConfigVersion   = "v3.7.0-gke.0"
 
 	GcsfuseVersionVarName = "gcsfuse-version"
 
@@ -190,7 +190,7 @@ func ParseTestConfig(body []byte) (TestPackages, error) {
 }
 
 func IsParallelDownloadsEnabled(flag string) bool {
-	return strings.Contains(flag, "file-cache-enable-parallel-downloads") && !strings.Contains(flag, "file-cache-enable-parallel-downloads=false")
+	return strings.Contains(flag, flagFileCacheEnableParallelDownloads) && !strings.Contains(flag, flagFileCacheEnableParallelDownloads+"=false")
 }
 
 func IsFileCacheEnabled(testName string) bool {
@@ -234,7 +234,6 @@ func ParseConfigFlags(flagStr string) ParsedConfig {
 		// See: https://github.com/GoogleCloudPlatform/gcs-fuse-csi-driver/blob/main/pkg/sidecar_mounter/sidecar_mounter_config.go#L83
 
 		// Group 1: Flags that are parsed manually and should not be passed to mountOptions.
-
 		// file-cache:max-size-mb is used by the CSI driver to enable the file cache feature and configure the cache volume.
 		// If not provided or set to 0, the cache directory will not be created.
 		// See: https://github.com/GoogleCloudPlatform/gcs-fuse-csi-driver/blob/main/pkg/sidecar_mounter/sidecar_mounter_config.go#L237-L241
@@ -262,14 +261,11 @@ func ParseConfigFlags(flagStr string) ParsedConfig {
 			continue
 		}
 
-		// Group 2: The following flags are translated to config file representation and passed to mountOptions.
-
-		// "log-file" is disallowed. The gcsfuse e2e tests hardcoded the log file in their test config.s
-		// We parse this to configure the test pod to align with the gcsfuse test config.
-		if strings.HasPrefix(f, flagLogFile) {
-			parsed.LogFilePath = strings.TrimPrefix(f, flagLogFile)
-			f = "logging:file-path:" + parsed.LogFilePath
-		} else if f == flagFileCacheEnableParallelDownloads || strings.HasPrefix(f, flagFileCacheEnableParallelDownloads+"=") {
+		// TODO: Remove this block after boolean flags formatting bug is fixed.
+		// Group 2: The following flags are booleans but not in boolFlags map in sidecar_mounter_config.go.
+		// They will be parsed as "x y" which is not a valid format in gcsfuse
+		// They need to be translated into config file representation x:y
+		if f == flagFileCacheEnableParallelDownloads || strings.HasPrefix(f, flagFileCacheEnableParallelDownloads+"=") {
 			val := "true"
 			if strings.Contains(f, "=") {
 				val = strings.SplitN(f, "=", 2)[1]
@@ -287,9 +283,14 @@ func ParseConfigFlags(flagStr string) ParsedConfig {
 				val = strings.SplitN(f, "=", 2)[1]
 			}
 			f = "file-system:enable-kernel-reader:" + val
-		} else if strings.HasPrefix(f, flagLogFormat) {
-			// "log-format" is disallowed so we need to format it to config file format
+		}
+
+		// log-format and log-file are in the disallowedFlags map, so we need to parse them and set them manually.
+		if strings.HasPrefix(f, flagLogFormat) {
 			f = "logging:format:" + strings.TrimPrefix(f, flagLogFormat)
+		} else if strings.HasPrefix(f, flagLogFile) {
+			parsed.LogFilePath = strings.TrimPrefix(f, flagLogFile)
+			f = "logging:file-path:" + parsed.LogFilePath
 		}
 
 		parsed.MountOptions = append(parsed.MountOptions, f)
@@ -342,6 +343,7 @@ func FetchGCSFuseVersion(ctx context.Context, cl clientset.Interface) (string, e
 		return "", fmt.Errorf("expected data for key `sidecar-image` in the config map `gcsfusecsi-image-config`")
 	}
 
+	// Deploy a temporary Pod to run the gcsfuse binary and fetch its version.
 	pod := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: "gcsfuse-version-fetcher-",
@@ -395,6 +397,8 @@ func FetchGCSFuseVersion(ctx context.Context, cl clientset.Interface) (string, e
 	klog.Infof("Pod %s is running, waiting 60s before fetching GCSFuse version from logs", createdPod.Name)
 	time.Sleep(60 * time.Second)
 
+	// Fetch logs from the specific container running the gcsfuse binary.
+	// We retry reading logs because it might take time for the logs to propagate to the apiserver.
 	var logs []byte
 	req := cl.CoreV1().Pods(DefaultNamespace).GetLogs(createdPod.Name, &corev1.PodLogOptions{Container: webhook.GcsFuseSidecarName})
 	err = wait.PollUntilContextTimeout(ctx, 30*time.Second, time.Minute*10, true, func(ctx context.Context) (bool, error) {
@@ -411,7 +415,7 @@ func FetchGCSFuseVersion(ctx context.Context, cl clientset.Interface) (string, e
 
 	output := string(logs)
 
-	// Parse: "gcsfuse version 3.7.1-gke.0 ..."
+	// Parse the version string from the standard format: "gcsfuse version 3.7.1-gke.0 ..."
 	l := strings.Split(strings.TrimSpace(output), " ")
 	if len(l) <= 2 {
 		return "", fmt.Errorf("unexpected version output format: %s", output)

test/e2e/utils/utils_test.go

@@ -79,15 +79,14 @@ func TestParseConfigFlags(t *testing.T) {
 		}{
 			{
 				name:    "should parse standard file cache flags correctly",
-				flagStr: "--file-cache-max-size-mb=9,--file-cache-cache-file-for-range-read=true,--metadata-cache-ttl-secs=10,--o=ro",
+				flagStr: "--file-cache-max-size-mb=9,--file-cache-cache-file-for-range-read=true,--metadata-cache-ttl-secs=10,--o=ro,--log-severity=trace",
 				expectedParsed: ParsedConfig{
 					FileCacheCapacity: "9Mi",
 					ReadOnly:          true,
+					LogSeverity:       "trace",
 					MountOptions: []string{
-						"file-cache:max-size-mb:9",
 						"file-cache-cache-file-for-range-read=true",
 						"metadata-cache-ttl-secs=10",
-						"o=ro",
 					},
 				},
 			},
@@ -97,6 +96,7 @@ func TestParseConfigFlags(t *testing.T) {
 				expectedParsed: ParsedConfig{
 					FileCacheCapacity: "50Mi",
 					ReadOnly:          false,
+					LogSeverity:       "info",
 					MountOptions:      []string{},
 				},
 			},
@@ -106,12 +106,30 @@ func TestParseConfigFlags(t *testing.T) {
 				expectedParsed: ParsedConfig{
 					FileCacheCapacity: "100Mi",
 					ReadOnly:          false,
+					LogSeverity:       "info",
 					MountOptions: []string{
-						"file-cache:max-size-mb:100",
 						"metadata-cache-ttl-secs=0",
 					},
 				},
 			},
+			{
+				name:    "should translate configuration flags correctly",
+				flagStr: "--log-file=/tmp/log,--file-cache-enable-parallel-downloads,--file-cache-enable-o-direct=false,--enable-kernel-reader=true,--log-format=text,--cache-dir=/tmp/cache",
+				expectedParsed: ParsedConfig{
+					FileCacheCapacity: "50Mi",
+					ReadOnly:          false,
+					LogFilePath:       "/tmp/log",
+					LogSeverity:       "info",
+					CacheDir:          "/tmp/cache",
+					MountOptions: []string{
+						"logging:file-path:/tmp/log",
+						"file-cache:enable-parallel-downloads:true",
+						"file-cache:enable-o-direct:false",
+						"file-system:enable-kernel-reader:true",
+						"logging:format:text",
+					},
+				},
+			},
 		}
 
 		for _, tc := range testCases {