Update chunkSize to be calculated on total cache #2035
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Sneha-at The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
|
@Sneha-at: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| @@ -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), | |||
There was a problem hiding this comment.
Why do we need these?
There was a problem hiding this comment.
I think some stale got committed, realized this just now.
| 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) |
There was a problem hiding this comment.
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 { | ||
| return "", fmt.Errorf("errored while fetching PV size %v: %s", err, info) | ||
| } | ||
| infoString := strings.TrimSpace(string(info)) |
There was a problem hiding this comment.
I would extract these string manipulation logic separately and add unit tests.
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
What this PR does / why we need it:
It updates the logic to use total cache size instead of PV cache size to calculate the chunk size.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: