diff --git a/pkg/csi_driver/utils.go b/pkg/csi_driver/utils.go index 0d23e5bc5..7c84ab643 100644 --- a/pkg/csi_driver/utils.go +++ b/pkg/csi_driver/utils.go @@ -188,7 +188,7 @@ func parseVolumeAttributes(fuseMountOptions []string, volumeContext map[string]s fuseMountOptions = joinMountOptions(fuseMountOptions, strings.Split(mountOptions, ",")) } skipCSIBucketAccessCheck := false - disableMetricsCollection := false + disableMetricsCollection := true for volumeAttribute, mountOption := range volumeAttributesToMountOptionsMapping { value, ok := volumeContext[volumeAttribute] if !ok { diff --git a/pkg/csi_driver/utils_test.go b/pkg/csi_driver/utils_test.go index d2d27d6ea..529295670 100644 --- a/pkg/csi_driver/utils_test.go +++ b/pkg/csi_driver/utils_test.go @@ -70,12 +70,12 @@ func TestParseVolumeAttributes(t *testing.T) { t.Run("parsing volume attributes into mount options", func(t *testing.T) { t.Parallel() testCases := []struct { - name string - volumeContext map[string]string - expectedMountOptions []string - expectedSkipBucketAccessCheck bool - expectedDisableMetricsCollection bool - expectedErr bool + name string + volumeContext map[string]string + expectedMountOptions []string + expectedSkipBucketAccessCheck bool + expectedEnableMetricsCollection bool + expectedErr bool }{ { name: "should return correct fileCacheCapacity 1", @@ -379,40 +379,43 @@ func TestParseVolumeAttributes(t *testing.T) { expectedErr: true, }, { - name: "value set to true for VolumeContextKeyDisableMetrics", - volumeContext: map[string]string{VolumeContextKeyDisableMetrics: util.TrueStr}, - expectedMountOptions: []string{volumeAttributesToMountOptionsMapping[VolumeContextKeyDisableMetrics] + util.TrueStr}, - expectedDisableMetricsCollection: true, + name: "value set to true for VolumeContextKeyDisableMetrics", + volumeContext: map[string]string{VolumeContextKeyDisableMetrics: util.TrueStr}, + expectedMountOptions: []string{volumeAttributesToMountOptionsMapping[VolumeContextKeyDisableMetrics] + util.TrueStr}, + expectedEnableMetricsCollection: false, }, { - name: "value set to false for VolumeContextKeyDisableMetrics", - volumeContext: map[string]string{VolumeContextKeyDisableMetrics: util.FalseStr}, - expectedMountOptions: []string{volumeAttributesToMountOptionsMapping[VolumeContextKeyDisableMetrics] + util.FalseStr}, - expectedDisableMetricsCollection: false, + name: "value set to false for VolumeContextKeyDisableMetrics", + volumeContext: map[string]string{VolumeContextKeyDisableMetrics: util.FalseStr}, + expectedMountOptions: []string{volumeAttributesToMountOptionsMapping[VolumeContextKeyDisableMetrics] + util.FalseStr}, + expectedEnableMetricsCollection: true, }, } for _, tc := range testCases { - t.Logf("test case: %s", tc.name) - output, skipCSIBucketAccessCheck, disableMetricsCollection, err := parseVolumeAttributes([]string{}, tc.volumeContext) - if (err != nil) != tc.expectedErr { - t.Errorf("Got error %v, but expected error %v", err, tc.expectedErr) - } + t.Run(tc.name, func(t *testing.T) { + t.Logf("test case: %s", tc.name) + output, skipCSIBucketAccessCheck, disableMetricsCollection, err := parseVolumeAttributes([]string{}, tc.volumeContext) + if (err != nil) != tc.expectedErr { + t.Errorf("Got error %v, but expected error %v", err, tc.expectedErr) + } + enableMetricsCollection := !disableMetricsCollection - if tc.expectedErr { - continue - } - if tc.expectedSkipBucketAccessCheck != skipCSIBucketAccessCheck { - t.Errorf("Got skipBucketAccessCheck %v, but expected %v", skipCSIBucketAccessCheck, tc.expectedSkipBucketAccessCheck) - } - if tc.expectedDisableMetricsCollection != disableMetricsCollection { - t.Errorf("Got disableMetricsCollection %v, but expected %v", disableMetricsCollection, tc.expectedDisableMetricsCollection) - } + if tc.expectedErr { + return + } + if tc.expectedSkipBucketAccessCheck != skipCSIBucketAccessCheck { + t.Errorf("Got skipBucketAccessCheck %v, but expected %v", skipCSIBucketAccessCheck, tc.expectedSkipBucketAccessCheck) + } + if tc.expectedEnableMetricsCollection != enableMetricsCollection { + t.Errorf("Got disableMetricsCollection %v, but expected %v", enableMetricsCollection, tc.expectedEnableMetricsCollection) + } - less := func(a, b string) bool { return a > b } - if diff := cmp.Diff(output, tc.expectedMountOptions, cmpopts.SortSlices(less)); diff != "" { - t.Errorf("unexpected options args (-got, +want)\n%s", diff) - } + less := func(a, b string) bool { return a > b } + if diff := cmp.Diff(output, tc.expectedMountOptions, cmpopts.SortSlices(less)); diff != "" { + t.Errorf("unexpected options args (-got, +want)\n%s", diff) + } + }) } }) } diff --git a/pkg/sidecar_mounter/sidecar_mounter_config.go b/pkg/sidecar_mounter/sidecar_mounter_config.go index 28c5a8803..0cd3ecd69 100644 --- a/pkg/sidecar_mounter/sidecar_mounter_config.go +++ b/pkg/sidecar_mounter/sidecar_mounter_config.go @@ -150,16 +150,13 @@ func NewMountConfig(sp string) *MountConfig { func (mc *MountConfig) prepareMountArgs() { flagMap := map[string]string{ - "app-name": GCSFuseAppName, - "temp-dir": mc.BufferDir + TempDir, - "config-file": mc.ConfigFile, - "foreground": "", - "uid": "0", - "gid": "0", - "prometheus-port": strconv.Itoa(prometheusPort), + "app-name": GCSFuseAppName, + "temp-dir": mc.BufferDir + TempDir, + "config-file": mc.ConfigFile, + "foreground": "", + "uid": "0", + "gid": "0", } - // Use a new port each gcsfuse instance that we start. - prometheusPort++ configFileFlagMap := map[string]string{ "logging:file-path": "/dev/fd/1", // redirect the output to cmd stdout @@ -174,8 +171,10 @@ func (mc *MountConfig) prepareMountArgs() { i := strings.LastIndex(arg, ":") f, v := arg[:i], arg[i+1:] - if f == util.DisableMetricsForGKE && v == util.TrueStr { - flagMap["prometheus-port"] = "0" + if f == util.DisableMetricsForGKE && v == util.FalseStr { + flagMap["prometheus-port"] = strconv.Itoa(prometheusPort) + // Use a new port each gcsfuse instance that we start. + prometheusPort++ continue } diff --git a/pkg/sidecar_mounter/sidecar_mounter_config_test.go b/pkg/sidecar_mounter/sidecar_mounter_config_test.go index 841359a7e..27d59f594 100644 --- a/pkg/sidecar_mounter/sidecar_mounter_config_test.go +++ b/pkg/sidecar_mounter/sidecar_mounter_config_test.go @@ -220,7 +220,7 @@ func TestPrepareMountArgs(t *testing.T) { BufferDir: "test-buffer-dir", CacheDir: "test-cache-dir", ConfigFile: "test-config-file", - Options: []string{util.DisableMetricsForGKE + ":true"}, + Options: []string{util.DisableMetricsForGKE + ":false"}, }, expectedArgs: map[string]string{ "app-name": GCSFuseAppName, @@ -237,30 +237,31 @@ func TestPrepareMountArgs(t *testing.T) { prometheusPort := 8080 for _, tc := range testCases { - t.Logf("test case: %s", tc.name) - - found := false - for _, o := range tc.mc.Options { - if o == util.DisableMetricsForGKE+":true" { - found = true + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + found := false + for _, o := range tc.mc.Options { + if o == util.DisableMetricsForGKE+":false" { + found = true - break + break + } } - } - if !found { - tc.expectedArgs["prometheus-port"] = strconv.Itoa(prometheusPort) - prometheusPort++ - } + if found { + tc.expectedArgs["prometheus-port"] = strconv.Itoa(prometheusPort) + prometheusPort++ + } - tc.mc.prepareMountArgs() - if !reflect.DeepEqual(tc.mc.FlagMap, tc.expectedArgs) { - t.Errorf("Got args %v, but expected %v", tc.mc.FlagMap, tc.expectedArgs) - } + tc.mc.prepareMountArgs() + if !reflect.DeepEqual(tc.mc.FlagMap, tc.expectedArgs) { + t.Errorf("Got args %v, but expected %v", tc.mc.FlagMap, tc.expectedArgs) + } - if !reflect.DeepEqual(tc.mc.ConfigFileFlagMap, tc.expectedConfigMapArgs) { - t.Errorf("Got config file args %v, but expected %v", tc.mc.ConfigFileFlagMap, tc.expectedConfigMapArgs) - } + if !reflect.DeepEqual(tc.mc.ConfigFileFlagMap, tc.expectedConfigMapArgs) { + t.Errorf("Got config file args %v, but expected %v", tc.mc.ConfigFileFlagMap, tc.expectedConfigMapArgs) + } + }) } } diff --git a/test/e2e/specs/specs.go b/test/e2e/specs/specs.go index fa588a6a1..90f875862 100644 --- a/test/e2e/specs/specs.go +++ b/test/e2e/specs/specs.go @@ -63,7 +63,9 @@ const ( SubfolderInBucketPrefix = "gcsfuse-csi-subfolder-in-bucket" MultipleBucketsPrefix = "gcsfuse-csi-multiple-buckets" EnableFileCacheForceNewBucketPrefix = "gcsfuse-csi-enable-file-cache-force-new-bucket" + EnableFileCacheForceNewBucketAndMetricsPrefix = "gcsfuse-csi-enable-file-cache-force-new-bucket-and-metrics" EnableFileCachePrefix = "gcsfuse-csi-enable-file-cache" + EnableFileCacheAndMetricsPrefix = "gcsfuse-csi-enable-file-cache-and-metrics" EnableFileCacheWithLargeCapacityPrefix = "gcsfuse-csi-enable-file-cache-large-capacity" ImplicitDirsPath = "implicit-dir" InvalidVolume = "" diff --git a/test/e2e/specs/testdriver.go b/test/e2e/specs/testdriver.go index b8130d965..0dbeeb18a 100644 --- a/test/e2e/specs/testdriver.go +++ b/test/e2e/specs/testdriver.go @@ -57,6 +57,7 @@ type gcsVolume struct { shared bool readOnly bool skipBucketAccessCheck bool + enableMetrics bool } // InitGCSFuseCSITestDriver returns GCSFuseCSITestDriver that implements TestDriver interface. @@ -151,7 +152,7 @@ func (n *GCSFuseCSITestDriver) CreateVolume(ctx context.Context, config *storage bucketName = uuid.NewString() case InvalidVolumePrefix, SkipCSIBucketAccessCheckAndInvalidVolumePrefix: bucketName = InvalidVolume - case ForceNewBucketPrefix, EnableFileCacheForceNewBucketPrefix: + case ForceNewBucketPrefix, EnableFileCacheForceNewBucketPrefix, EnableFileCacheForceNewBucketAndMetricsPrefix: bucketName = n.createBucket(ctx, config.Framework.Namespace.Name) case MultipleBucketsPrefix: isMultipleBucketsPrefix = true @@ -206,6 +207,9 @@ func (n *GCSFuseCSITestDriver) CreateVolume(ctx context.Context, config *storage mountOptions += ",only-dir=" + dirPath case EnableFileCachePrefix, EnableFileCacheForceNewBucketPrefix: v.fileCacheCapacity = "100Mi" + case EnableFileCacheAndMetricsPrefix, EnableFileCacheForceNewBucketAndMetricsPrefix: + v.fileCacheCapacity = "100Mi" + v.enableMetrics = true case EnableFileCacheWithLargeCapacityPrefix: v.fileCacheCapacity = "2Gi" case SkipCSIBucketAccessCheckPrefix, SkipCSIBucketAccessCheckAndFakeVolumePrefix, SkipCSIBucketAccessCheckAndInvalidVolumePrefix: @@ -229,7 +233,7 @@ func (n *GCSFuseCSITestDriver) CreateVolume(ctx context.Context, config *storage } switch config.Prefix { - case "", EnableFileCachePrefix, EnableFileCacheWithLargeCapacityPrefix: + case "", EnableFileCachePrefix, EnableFileCacheWithLargeCapacityPrefix, EnableFileCacheAndMetricsPrefix: // Use config.Prefix to pass the bucket names back to the test suite. config.Prefix = bucketName } @@ -262,6 +266,10 @@ func (n *GCSFuseCSITestDriver) GetPersistentVolumeSource(readOnly bool, _ string va[driver.VolumeContextKeySkipCSIBucketAccessCheck] = util.TrueStr } + if gv.enableMetrics { + va[driver.VolumeContextKeyDisableMetrics] = util.FalseStr + } + return &corev1.PersistentVolumeSource{ CSI: &corev1.CSIPersistentVolumeSource{ Driver: n.driverInfo.Name, @@ -289,6 +297,10 @@ func (n *GCSFuseCSITestDriver) GetVolume(config *storageframework.PerTestConfig, va[driver.VolumeContextKeySkipCSIBucketAccessCheck] = util.TrueStr } + if gv.enableMetrics { + va[driver.VolumeContextKeyDisableMetrics] = util.FalseStr + } + return va, gv.shared, gv.readOnly } diff --git a/test/e2e/testsuites/metrics.go b/test/e2e/testsuites/metrics.go index 32ac3e8af..7d92c15b4 100644 --- a/test/e2e/testsuites/metrics.go +++ b/test/e2e/testsuites/metrics.go @@ -257,7 +257,7 @@ func (t *gcsFuseCSIMetricsTestSuite) DefineTests(driver storageframework.TestDri // | // [bucket1] ginkgo.It("should emit metrics", func() { - init(1, specs.EnableFileCachePrefix) + init(1, specs.EnableFileCacheAndMetricsPrefix) defer cleanup() ginkgo.By("Configuring the pod") @@ -281,7 +281,7 @@ func (t *gcsFuseCSIMetricsTestSuite) DefineTests(driver storageframework.TestDri // | | // [bucket1] [bucket2] ginkgo.It("should emit metrics from multiple volumes", func() { - init(2, specs.EnableFileCacheForceNewBucketPrefix) + init(2, specs.EnableFileCacheForceNewBucketAndMetricsPrefix) defer cleanup() ginkgo.By("Configuring the pod")