Skip to content

Update chunkSize to be calculated on total cache #2035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 62 additions & 3 deletions pkg/gce-pd-csi-driver/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,18 @@ func setupCaching(devicePath string, req *csi.NodeStageVolumeRequest, nodeId str
klog.V(4).Infof("Assuming valid data cache size and mode, resizing cache is not supported")
} else {
cacheSize := req.GetPublishContext()[common.ContextDataCacheSize]
chunkSize, err := fetchChunkSizeKiB(cacheSize)
maxChunkSizeStr := strconv.FormatFloat(maxChunkSize, 'g', -1, 64)
var chunkSize string
cachePvSize, err := fetchPvSize(raidedLocalSsdPath)
if err != nil {
klog.Errorf("Errored to fetch cache size, verify the data-cache-size is valid: got %v, error: %q", cacheSize, err)
return mainDevicePath, err
klog.Errorf("Errored while fetching PV size, got %v, falling back to default chunkSize of %v", err, maxChunkSize)
chunkSize = maxChunkSizeStr
} else {
chunkSize, err = fetchChunkSizeKiB(cachePvSize)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fetchChunkSizeKiB function itself is assuming that the input passing in (previously cacheSize now cachePvSize) is in GiB unit. But your fetchPvSize is returning KiB size already? Maybe the logic in fetchChunkSizeKiB also needs to change?

if err != nil {
klog.Errorf("Errored to fetch cache size, verify the data-cache-size is valid: got %v, error: %q", chunkSize, err)
chunkSize = maxChunkSizeStr
}
}
// Check if LV exists
info, err = common.RunCommand("" /* pipedCmd */, nil /* pipedCmdArg */, "lvs", args...)
Expand Down Expand Up @@ -639,6 +647,21 @@ func watchDiskDetaches(watcher *fsnotify.Watcher, nodeName string, errorCh chan
errorCh <- fmt.Errorf("disk update event errored: %v", err)
// watch for events
case event := <-watcher.Events:
args := []string{
"--updatemetadata",
getVolumeGroupName(nodeName),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some stale got committed, realized this just now.

}
_, err := common.RunCommand("" /* pipedCmd */, nil /* pipedCmdArg */, "vgck", args...)
if err != nil {
klog.Errorf("Error updating volume group's metadata: %v", err)
}
args = []string{}
info, _ := common.RunCommand("" /* pipedCmd */, nil /* pipedCmdArg */, "pvs", args...)
args = []string{
getVolumeGroupName(nodeName),
}
infoVG, _ := common.RunCommand("" /* pipedCmd */, nil /* pipedCmdArg */, "vgdisplay", args...)
klog.Infof("Got VG %s and PV %s", infoVG, info)
// In case of an event i.e. creation or deletion of any new PV, we update the VG metadata.
// This might include some non-LVM changes, no harm in updating metadata multiple times.
reduceVolumeGroup(getVolumeGroupName(nodeName), true)
Expand Down Expand Up @@ -674,3 +697,39 @@ func addRaidedLSSDToVg(vgName, lssdPath string) error {
}
return nil
}

func fetchPvSize(pvName string) (string, error) {
var pvSize string
args := []string{
"--select",
"pv_name=" + pvName,
"-o",
"--noheadings",
"pv_size",
"--units=b",
}
info, err := common.RunCommand("" /* pipedCmd */, nil /* pipedCmdArg */, "pvs", args...)
if err != nil {
return "", fmt.Errorf("errored while fetching PV size %v: %s", err, info)
}
infoString := strings.TrimSpace(string(info))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would extract these string manipulation logic separately and add unit tests.

infoSlice := strings.Fields(infoString)
re, err := regexp.Compile("^[0-9]+B$")
if err != nil {
return "", fmt.Errorf("Errored while compiling regex for PV size")
}
for _, i := range infoSlice {
if re.MatchString(i) {
pvSize = strings.TrimSuffix(i, "B")
break
}
}
if pvSize != "" {
pvSizeInt, err := strconv.ParseFloat(pvSize, 64)
if err != nil {
return "", fmt.Errorf("Error while fetching PV size for cache")
}
return strconv.FormatInt(int64(pvSizeInt/KiB), 10) + "KiB", nil
}
return "", fmt.Errorf("Error fetching PV size for cache")
}