disk: introduce data cache#1621
Conversation
k8s-ci-robot
added
kind/feature
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: huww98 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
huww98
force-pushed
the
local-disk-cache
branch
3 times, most recently
from
09b5bf8 to
0e9f46b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
k8s-ci-robot
added
the
needs-rebase
Use a faster disk as a cache layer over cloud disk.
huww98
force-pushed
the
local-disk-cache
branch
from
0e9f46b to
ecef144
Compare
k8s-ci-robot
removed
the
needs-rebase
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
There was a problem hiding this comment.
Automated Code Review
Summary: Introduces dm-cache (device-mapper cache) support for cloud disks, using local NVMe/SSD as a cache tier via loop devices. Includes setup, resize, and teardown of dm-cache backed by fallocated files.
Issues Found:
- Unsafe pointer usage: The
dmi_tstruct usesunsafe.Pointerwithunix.Syscallfor ioctl calls. While necessary for dm-ioctl, ensure the struct layout exactly matches the kernel's expectations — any padding mismatch could corrupt memory. - Error handling in
allocCacheFile: IfIoctlLoopConfigurefails, the loop device fd is leaked (opened but never closed). Add adefer loggedClose(logger, loop)before the configure call, or close on error path. - Race condition potential:
loop_get_free()gets a free loop device number, but between getting the number and opening it, another process could claim it. Consider retrying on EBUSY. structs.HostLayout: This is a Go 1.23+ feature. Confirm the project's minimum Go version supports this.- The
DataCachePathconstant uses/var/alibaba-cloud-csi/data-cache— this is a host path, so the Helm chart correctly adds the volume mount (seen in the template diff).
Suggestions:
- Add cleanup logic (or documentation) for what happens when a node reboots — loop devices configured with
LO_FLAGS_AUTOCLEARwill disappear, but the dm-cache device entries may not persist across reboots either. - Consider adding metrics for cache hit/miss ratios to help users understand cache effectiveness.
- Unit tests for
getDataCacheOptsparsing would be valuable.
Overall: Complex but well-implemented feature leveraging dm-cache for performance. The low-level kernel interface code is inherently delicate — careful testing on target kernels is essential. The fd leak in the error path should be fixed.
mowangdk
left a comment
mowangdk
left a comment
There was a problem hiding this comment.
Automated Code Review
Summary: Introduces dm-cache support as a local cache layer over cloud disks using device-mapper, loop devices, and fallocate.
Overall: Powerful feature with careful dm-cache management. Some concerns about resource leaks on error paths and unsafe pointer usage.
| Flags: flags, | ||
| } | ||
| copy(dm.Name[:], volumeID) | ||
| _, _, err := unix.Syscall(unix.SYS_IOCTL, uintptr(fd), action, uintptr(unsafe.Pointer(&dm))) |
There was a problem hiding this comment.
allocCacheFile opens the loop device fd (loop) but never closes it on error paths after Open succeeds. If IoctlLoopConfigure fails, the loop fd leaks. Add a deferred close that is cancelled on success, or restructure so the fd is always returned.
| } | ||
| copy(dmi.Name[:], volumeID) | ||
| copy(dmi.Args[:], args) | ||
| _, _, errno := unix.Syscall(unix.SYS_IOCTL, uintptr(dmCtrl), unix.DM_TABLE_LOAD, uintptr(unsafe.Pointer(&dmi))) |
There was a problem hiding this comment.
The correctness of passing unsafe.Pointer(&dmi) to SYS_IOCTL depends on the struct layout matching what the kernel expects exactly. Consider adding a compile-time size assertion (e.g., var _ [4096 - unsafe.Sizeof(dmi_t{})]byte) to catch layout drift if the Go struct definitions change.
| return "/dev/mapper/" + volumeID | ||
| } | ||
|
|
||
| func setupDataCache(logger klog.Logger, d *dataCache, device, volumeID string) (string, error) { |
There was a problem hiding this comment.
getBlockDeviceCapacity(device) is called but there's no error handling visible. If it returns 0 (e.g., device not ready), dm-cache is set up with size 0, which would be invalid. Consider validating the return value before calling setupDmCache.
| } | ||
|
|
||
| // Teardown DataCache | ||
| if cacheErr := teardownDataCache(logger, req.VolumeId); cacheErr != nil { |
There was a problem hiding this comment.
Data cache teardown happens after unmount. If teardownDataCache fails, the function returns an error but the volume is already unmounted. This leaves a lingering dm-cache device that blocks future unstage attempts. Consider whether teardown failures should be non-fatal (log and continue) to avoid permanently blocking the unstage flow.
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
kubernetes-prow
Bot
added
lifecycle/rotten
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Use a faster disk as a cache layer over cloud disk.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: