Skip to content

Commit e94c263

Browse files
Sneha-atSneha-at
committed
Add unit tests
1 parent 81510ca commit e94c263

File tree

5 files changed

+127
-20
lines changed

5 files changed

+127
-20
lines changed

‎pkg/common/parameters.go

+4-2
Original file line numberDiff line numberDiff line change
@@ -280,19 +280,21 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string]
280280
if !enableDataCache {
281281
return p, d, fmt.Errorf("data caching enabled: %v; parameters contains invalid option %q", enableDataCache, ParameterKeyDataCacheSize)
282282
}
283-
// TODO: need to parse or validate the string
284283

285284
paramDataCacheSize, err := ConvertGiStringToInt64(v)
286285
if err != nil {
287286
return p, d, fmt.Errorf("parameters contain invalid dataCacheSize parameter: %w", err)
288287
}
288+
if err := ValidateNonNegativeInt(paramDataCacheSize); err != nil {
289+
return p, d, fmt.Errorf("parameters contains invalid option: %s: %w", ParameterKeyDataCacheSize, err)
290+
}
289291
d.DataCacheSize = strconv.FormatInt(paramDataCacheSize, 10)
290292
case ParameterKeyDataCacheMode:
291293
if !enableDataCache {
292294
return p, d, fmt.Errorf("data caching enabled %v; parameters contains invalid option %q", enableDataCache, ParameterKeyDataCacheSize)
293295
}
294296
if err := ValidateDataCacheMode(v); err != nil {
295-
return p, d, fmt.Errorf("parameters contains invalid option: %w", err)
297+
return p, d, fmt.Errorf("parameters contains invalid option: %s: %w", ParameterKeyDataCacheMode, err)
296298
}
297299
d.DataCacheMode = v
298300
case ParameterKeyResourceTags:

‎pkg/common/utils.go

+7
Original file line numberDiff line numberDiff line change
@@ -713,6 +713,13 @@ func ValidateDataCacheMode(s string) error {
713713
return fmt.Errorf("invalid data-cache-mode %s. Only \"writeback\" and \"writethrough\" is a valid input", s)
714714
}
715715

716+
func ValidateNonNegativeInt(n int64) error {
717+
if n <= 0 {
718+
return fmt.Errorf("Input should be set to > 0, got %d", n)
719+
}
720+
return nil
721+
}
722+
716723
// NewLimiter returns a token bucket based request rate limiter after initializing
717724
// the passed values for limit, burst (or token bucket) size. If opted for emptyBucket
718725
// all initial tokens are reserved for the first burst.

‎pkg/common/utils_test.go

+36
Original file line numberDiff line numberDiff line change
@@ -1823,6 +1823,42 @@ func TestValidateDataCacheMode(t *testing.T) {
18231823

18241824
}
18251825

1826+
func TestValidateNonNegativeInt(t *testing.T) {
1827+
testCases := []struct {
1828+
name string
1829+
cacheSize int64
1830+
expectError bool
1831+
}{
1832+
{
1833+
name: "valid input - positive cache size",
1834+
cacheSize: 100000,
1835+
},
1836+
{
1837+
name: "invalid input - cachesize 0",
1838+
cacheSize: 0,
1839+
expectError: true,
1840+
},
1841+
{
1842+
name: "invalid input - negative cache size",
1843+
cacheSize: -100,
1844+
expectError: true,
1845+
},
1846+
}
1847+
1848+
for _, tc := range testCases {
1849+
t.Logf("test case: %s", tc.name)
1850+
err := ValidateNonNegativeInt(tc.cacheSize)
1851+
if err != nil && !tc.expectError {
1852+
t.Errorf("Got error %v validate data cache mode %d; expect no error", err, tc.cacheSize)
1853+
}
1854+
1855+
if err == nil && tc.expectError {
1856+
t.Errorf("Got no error validate data cache mode %d; expect an error", tc.cacheSize)
1857+
}
1858+
}
1859+
1860+
}
1861+
18261862
func TestParseZoneFromURI(t *testing.T) {
18271863
testcases := []struct {
18281864
name string

‎pkg/gce-pd-csi-driver/cache.go

+23-18
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,18 @@ import (
1717
)
1818

1919
const (
20-
cacheSuffix = "csi-fast"
21-
mainLvSuffix = "csi-main"
22-
raidedLocalSsdName = "csi-driver-data-cache"
23-
raidMode = "0"
24-
maxAllowedChunks int64 = 1000000 // This is the max allowed chunks for LVM
25-
GiB int64 = 1024 * 1024 * 1024
26-
KiB int64 = 1024
20+
cacheSuffix = "csi-fast"
21+
mainLvSuffix = "csi-main"
22+
raidedLocalSsdName = "csi-driver-data-cache"
23+
raidMode = "0"
24+
maxAllowedChunks int64 = 1000000 // This is the max allowed chunks for LVM
25+
GiB float64 = 1024 * 1024 * 1024
26+
KiB float64 = 1024
27+
)
28+
29+
var (
30+
maxChunkSize float64 = 1 * GiB // Max allowed chunk size as per LVM documentation
31+
minChunkSize float64 = 160 * KiB // This is randomly selected, we need a multiple of 32KiB, the default size would be too small for caching https://man7.org/linux/man-pages/man8/lvcreate.8.html (--chunksize)
2732
)
2833

2934
func fetchRAIDedLocalSsdPath() (string, error) {
@@ -88,7 +93,7 @@ func setupCaching(devicePath string, req *csi.NodeStageVolumeRequest, nodeId str
8893
vgNameForPv := strings.TrimSpace(infoSlice[(len(infoSlice) - 1)])
8994
klog.V(4).Infof("Physical volume is part of Volume group: %v", vgNameForPv)
9095
if vgNameForPv == volumeGroupName {
91-
klog.V(4).Infof("Physical Volume(PV) already exists in the Volume Group")
96+
klog.V(4).Infof("Physical Volume(PV) already exists in the Volume Group %v", volumeGroupName)
9297
} else if vgNameForPv != "VG" && vgNameForPv != "" {
9398

9499
info, err = common.RunCommand("" /* pipedCmd */, nil /* pipedCmdArg */, "vgchange", []string{"-an", vgNameForPv}...)
@@ -164,7 +169,7 @@ func setupCaching(devicePath string, req *csi.NodeStageVolumeRequest, nodeId str
164169
klog.V(4).Infof("Assuming valid data cache size and mode, resizing cache is not supported")
165170
} else {
166171
cacheSize := req.GetPublishContext()[common.ContextDataCacheSize]
167-
chunkSize, err := fetchChunkSize(cacheSize)
172+
chunkSize, err := fetchChunkSizeKiB(cacheSize)
168173
if err != nil {
169174
klog.Errorf("Errored to fetch cache size, verify the data-cache-size is valid: got %v, error: %q", cacheSize, err)
170175
return mainDevicePath, err
@@ -201,7 +206,7 @@ func setupCaching(devicePath string, req *csi.NodeStageVolumeRequest, nodeId str
201206
req.GetPublishContext()[common.ContextDataCacheMode],
202207
volumeGroupName + "/" + mainLvName,
203208
"--chunksize",
204-
chunkSize,
209+
chunkSize, // default unit is KiB
205210
"--force",
206211
"-y",
207212
}
@@ -403,7 +408,7 @@ func cleanupCache(volumeId string, nodeId string) error {
403408

404409
func checkLvExists(lvName string) bool {
405410
args := []string{}
406-
info, err := common.RunCommand("" /* pipedCmd */, "" /* pipedCmdArg */, "lvscan", args...)
411+
info, err := common.RunCommand("" /* pipedCmd */, nil /* pipedCmdArg */, "lvscan", args...)
407412
if err != nil {
408413
klog.Errorf("Errored while checking if logical volume exists for %s %v: %s", lvName, err, info)
409414
return false
@@ -528,17 +533,17 @@ func isCachingSetup(mainLvName string) (error, bool) {
528533
return nil, false
529534
}
530535

531-
func fetchChunkSize(cacheSize string) (string, error) {
536+
func fetchChunkSizeKiB(cacheSize string) (string, error) {
532537
var chunkSize float64
533-
var maxChunkSize int64 = 1 * GiB // Max allowed chunk size as per LVM documentation
534-
var minChunkSize int64 = 320 * KiB // This is randomly selected, we need a multiple of 32KiB, the default size would be too small for caching https://man7.org/linux/man-pages/man8/lvcreate.8.html (--chunksize)
538+
535539
cacheSizeInt, err := common.ConvertGiStringToInt64(cacheSize)
536540
if err != nil {
537541
return "0", err
538542
}
539543
// Chunksize should be divisible by 32Kib so we need (chunksize/32*1024)*32*1024
540-
chunkSize = float64(cacheSizeInt) / float64(maxAllowedChunks)
541-
chunkSize = math.Ceil(chunkSize/float64(32*KiB)) * float64(32*KiB)
542-
chunkSize = math.Min(math.Max(chunkSize, float64(minChunkSize)), float64(maxChunkSize))
543-
return strconv.FormatInt(int64(chunkSize), 10), nil
544+
chunkSize = (float64(cacheSizeInt) * GiB) / float64(maxAllowedChunks)
545+
chunkSize = math.Round(chunkSize/(32*KiB)) * (32 * KiB)
546+
chunkSize = math.Min(math.Max(chunkSize, minChunkSize), maxChunkSize) / KiB
547+
// default chunk size unit KiB
548+
return strconv.FormatInt(int64(chunkSize), 10) + "KiB", nil
544549
}

‎pkg/gce-pd-csi-driver/cache_test.go

+57
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
package gceGCEDriver
2+
3+
import (
4+
"testing"
5+
)
6+
7+
func TestFetchChunkSizeKiB(t *testing.T) {
8+
testCases := []struct {
9+
name string
10+
cacheSize string
11+
expChunkSize string
12+
expErr bool
13+
}{
14+
{
15+
name: "chunk size is in the allowed range",
16+
cacheSize: "500Gi",
17+
expChunkSize: "512KiB", //range defined in fetchChunkSizeKiB
18+
},
19+
{
20+
name: "chunk size is set to the range ceil",
21+
cacheSize: "30000000Gi",
22+
expChunkSize: "1048576KiB", //range defined in fetchChunkSizeKiB - max 1GiB
23+
},
24+
{
25+
name: "chunk size is set to the allowed range floor",
26+
cacheSize: "10Gi",
27+
expChunkSize: "160KiB", //range defined in fetchChunkSizeKiB - min 160 KiB
28+
},
29+
{
30+
name: "cacheSize set to KiB also sets the chunk size to range floor",
31+
cacheSize: "100Ki",
32+
expChunkSize: "160KiB", //range defined in fetchChunkSizeKiB - min 160 KiB
33+
},
34+
{
35+
name: "invalid cacheSize",
36+
cacheSize: "fdfsdKi",
37+
expChunkSize: "160KiB", //range defined in fetchChunkSizeKiB - min 160 KiB
38+
expErr: true,
39+
},
40+
// cacheSize is validated in storage class parameter so assuming invalid cacheSize (like negative, 0) would not be passed to the function
41+
}
42+
43+
for _, tc := range testCases {
44+
chunkSize, err := fetchChunkSizeKiB(tc.cacheSize)
45+
if err != nil {
46+
if !tc.expErr {
47+
t.Errorf("Errored %s", err)
48+
}
49+
continue
50+
}
51+
if chunkSize != tc.expChunkSize {
52+
t.Errorf("Got %s want %s", chunkSize, tc.expChunkSize)
53+
}
54+
55+
}
56+
57+
}

0 commit comments

Comments
 (0)