Clean up code

Author: sunnylovestiramisu

Makefile

@@ -48,7 +48,7 @@ else
 endif
 
 build-container: require-GCE_PD_CSI_STAGING_IMAGE require-GCE_PD_CSI_STAGING_VERSION init-buildx
-	$(DOCKER) buildx build --progress=plain --platform=linux --progress=plain \
+	$(DOCKER) buildx build --platform=linux --progress=plain \
 		-t $(STAGINGIMAGE):$(STAGINGVERSION) \
 		--build-arg BUILDPLATFORM=linux \
 		--build-arg STAGINGVERSION=$(STAGINGVERSION) \

pkg/common/constants.go

@@ -32,4 +32,8 @@ const (
 
 	// Label that is set on a disk when it is used by a 'multi-zone' VolumeHandle
 	MultiZoneLabel = "goog-gke-multi-zone"
+
+	// Data cache mode
+	DataCacheModeWriteBack    = "writeback"
+	DataCacheModeWriteThrough = "writethrough"
 )

pkg/common/parameters.go

@@ -18,6 +18,7 @@ package common
 
 import (
 	"fmt"
+	"strconv"
 	"strings"
 
 	"k8s.io/klog/v2"
@@ -75,9 +76,8 @@ const (
 )
 
 type DataCacheParameters struct {
-	// Values: {string}
+	// Values: {string} in int64 form
 	// Default: ""
-	// Example: "25Gi"
 	DataCacheSize string
 	// Values: writethrough, writeback
 	// Default: writethrough
@@ -151,10 +151,10 @@ func ExtractAndDefaultParameters(parameters map[string]string, driverName string
 		ResourceTags:         make(map[string]string), // Default
 	}
 
-	// Set data cache feature default
+	// Set data cache mode default
 	d := DataCacheParameters{}
-	if enableDataCache {
-		d.DataCacheMode = "writethrough"
+	if enableDataCache && parameters[ParameterKeyDataCacheSize] != "" {
+		d.DataCacheMode = DataCacheModeWriteThrough
 	}
 
 	for k, v := range extraVolumeLabels {
@@ -245,12 +245,20 @@ func ExtractAndDefaultParameters(parameters map[string]string, driverName string
 				return p, d, fmt.Errorf("parameters contains invalid option %q", ParameterKeyDataCacheSize)
 			}
 			// TODO: need to parse or validate the string
-			d.DataCacheSize = v
+
+			paramDataCacheSize, err := ConvertGiStringToInt64(v)
+			if err != nil {
+				return p, d, fmt.Errorf("parameters contain invalid dataCacheSize parameter: %w", err)
+			}
+			d.DataCacheSize = strconv.FormatInt(paramDataCacheSize, 10)
 			klog.V(2).Infof("====== Data cache size is %v ======", v)
 		case ParameterKeyDataCacheMode:
 			if !enableDataCache {
 				return p, d, fmt.Errorf("parameters contains invalid option %q", ParameterKeyDataCacheSize)
 			}
+			if err := ValidateDataCacheMode(v); err != nil {
+				return p, d, fmt.Errorf("parameters contains invalid option: %w", err)
+			}
 			d.DataCacheMode = v
 			klog.V(2).Infof("====== Data cache mode is %v ======", v)
 		case ParameterKeyResourceTags:

pkg/common/parameters_test.go

@@ -25,13 +25,15 @@ import (
 
 func TestExtractAndDefaultParameters(t *testing.T) {
 	tests := []struct {
-		name               string
-		parameters         map[string]string
-		labels             map[string]string
-		enableStoragePools bool
-		extraTags          map[string]string
-		expectParams       DiskParameters
-		expectErr          bool
+		name                  string
+		parameters            map[string]string
+		labels                map[string]string
+		enableStoragePools    bool
+		enableDataCache       bool
+		extraTags             map[string]string
+		expectParams          DiskParameters
+		expectDataCacheParams DataCacheParameters
+		expectErr             bool
 	}{
 		{
 			name:       "defaults",
@@ -350,11 +352,58 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 			labels:             map[string]string{},
 			expectErr:          true,
 		},
+		{
+			name:            "data cache parameters - set default cache mode",
+			enableDataCache: true,
+			parameters:      map[string]string{ParameterKeyType: "pd-balanced", ParameterKeyReplicationType: "none", ParameterKeyDiskEncryptionKmsKey: "foo/key", ParameterKeyLabels: "key1=value1,key2=value2", ParameterKeyDataCacheSize: "1234Gi"},
+			labels:          map[string]string{},
+			expectParams: DiskParameters{
+				DiskType:             "pd-balanced",
+				ReplicationType:      "none",
+				DiskEncryptionKMSKey: "foo/key",
+				Tags:                 map[string]string{},
+				Labels: map[string]string{
+					"key1": "value1",
+					"key2": "value2",
+				},
+			},
+			expectDataCacheParams: DataCacheParameters{
+				DataCacheMode: DataCacheModeWriteThrough,
+				DataCacheSize: "1234",
+			},
+		},
+		{
+			name:            "data cache parameters",
+			enableDataCache: true,
+			parameters:      map[string]string{ParameterKeyType: "pd-balanced", ParameterKeyReplicationType: "none", ParameterKeyDiskEncryptionKmsKey: "foo/key", ParameterKeyLabels: "key1=value1,key2=value2", ParameterKeyDataCacheSize: "1234Gi", ParameterKeyDataCacheMode: DataCacheModeWriteBack},
+			labels:          map[string]string{},
+			expectParams: DiskParameters{
+				DiskType:             "pd-balanced",
+				ReplicationType:      "none",
+				DiskEncryptionKMSKey: "foo/key",
+				Tags:                 map[string]string{},
+				Labels: map[string]string{
+					"key1": "value1",
+					"key2": "value2",
+				},
+			},
+			expectDataCacheParams: DataCacheParameters{
+				DataCacheMode: DataCacheModeWriteBack,
+				DataCacheSize: "1234",
+			},
+		},
+		{
+			name:            "data cache parameters - enableDataCache is false",
+			enableDataCache: false,
+			parameters:      map[string]string{ParameterKeyType: "pd-balanced", ParameterKeyReplicationType: "none", ParameterKeyDiskEncryptionKmsKey: "foo/key", ParameterKeyLabels: "key1=value1,key2=value2", ParameterKeyDataCacheSize: "1234Gi", ParameterKeyDataCacheMode: DataCacheModeWriteBack},
+			labels:          map[string]string{},
+			expectErr:       true,
+		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			p, _, err := ExtractAndDefaultParameters(tc.parameters, "testDriver", tc.labels, tc.enableStoragePools, false, tc.extraTags)
+			p, d, err := ExtractAndDefaultParameters(tc.parameters, "testDriver", tc.labels, tc.enableStoragePools, tc.enableDataCache, tc.extraTags)
 			if gotErr := err != nil; gotErr != tc.expectErr {
 				t.Fatalf("ExtractAndDefaultParameters(%+v) = %v; expectedErr: %v", tc.parameters, err, tc.expectErr)
 			}
@@ -365,6 +414,10 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 			if diff := cmp.Diff(p, tc.expectParams); diff != "" {
 				t.Errorf("ExtractAndDefaultParameters(%+v): -want, +got \n%s", tc.parameters, diff)
 			}
+
+			if diff := cmp.Diff(d, tc.expectDataCacheParams); diff != "" {
+				t.Errorf("ExtractAndDefaultParameters(%+v): -want, +got \n%s", tc.parameters, diff)
+			}
 		})
 	}
 }

pkg/common/runcmd.go

@@ -4,11 +4,6 @@ import (
 	"fmt"
 	"os/exec"
 	"strings"
-	/*
-		"bufio"
-		"io"
-		"io/ioutil"
-	*/
 
 	"k8s.io/klog/v2"
 )
@@ -36,25 +31,4 @@ func RunCommand(cmd string, args ...string) ([]byte, error) {
 		return output, fmt.Errorf("%s %s failed: %w; output: %s", cmd, strings.Join(args, " "), err, string(output))
 	}
 	return output, nil
-	/*
-		pipe, _ := execCmd.StdoutPipe()
-		if err := execCmd.Start(); err != nil {
-			klog.Errorf("====== Failed execCmd.Start() ======")
-		}
-		outStr := ""
-		go func(p io.ReadCloser) {
-			reader := bufio.NewReader(pipe)
-			klog.V(2).Infof("====== Start ioutil.ReadAll ======")
-			b, err := ioutil.ReadAll(reader)
-			if err != nil {
-				klog.Errorf("====== Failed ioutil.ReadAll(reader) %v ======", err)
-			}
-			klog.V(2).Infof("%v \n", string(b))
-			klog.V(2).Infof("====== End ioutil.ReadAll ======")
-		}(pipe)
-
-		if err := execCmd.Wait(); err != nil {
-			klog.Errorf("====== Failed execCmd.Wait(): %v ======", err)
-		}
-		return []byte(outStr), nil*/
 }

pkg/common/utils.go

@@ -92,6 +92,8 @@ var (
 		http.StatusNotFound:        codes.NotFound,
 	}
 
+	validDataCacheMode = []string{DataCacheModeWriteBack, DataCacheModeWriteThrough}
+
 	// Regular expressions for validating parent_id, key and value of a resource tag.
 	regexParent = regexp.MustCompile(`(^[1-9][0-9]{0,31}$)|(^[a-z][a-z0-9-]{4,28}[a-z0-9]$)`)
 	regexKey    = regexp.MustCompile(`^[a-zA-Z0-9]([0-9A-Za-z_.-]{0,61}[a-zA-Z0-9])?$`)
@@ -381,6 +383,15 @@ func ConvertMiStringToInt64(str string) (int64, error) {
 	return volumehelpers.RoundUpToMiB(quantity)
 }
 
+// ConvertGiStringToInt64 converts a GiB string to int64
+func ConvertGiStringToInt64(str string) (int64, error) {
+	quantity, err := resource.ParseQuantity(str)
+	if err != nil {
+		return -1, err
+	}
+	return volumehelpers.RoundUpToGiB(quantity)
+}
+
 // ConvertStringToBool converts a string to a boolean.
 func ConvertStringToBool(str string) (bool, error) {
 	switch strings.ToLower(str) {
@@ -578,6 +589,22 @@ func VolumeIdAsMultiZone(volumeId string) (string, error) {
 	return strings.Join(splitId, "/"), nil
 }
 
+func StringInSlice(s string, list []string) bool {
+	for _, v := range list {
+		if v == s {
+			return true
+		}
+	}
+	return false
+}
+
+func ValidateDataCacheMode(s string) error {
+	if StringInSlice(s, validDataCacheMode) {
+		return nil
+	}
+	return fmt.Errorf("invalid data-cache-mode %s. Only \"writeback\" and \"writethrough\" is a valid input", s)
+}
+
 // NewLimiter returns a token bucket based request rate limiter after initializing
 // the passed values for limit, burst (or token bucket) size. If opted for emptyBucket
 // all initial tokens are reserved for the first burst.