feat(core): add graceful shutdown and concurrency safety - #18
Conversation
Remove the k8s.io/kubernetes internal dependency (anti-pattern) and the
cadvisor dependency entirely. Replace them with minimal local packages
that read directly from sysfs/procfs and parse kubelet state files as
simple JSON.
Key changes:
- Go 1.15 -> 1.25, k8s.io/* v0.19.6 -> v0.35.0
- klog v1 -> klog/v2 v2.130.1
- volcano.sh/apis updated to v1.14.2
- Remove 25-entry replace block (now only 1 for volcano.sh/apis)
- Add pkg/internal/{cpuset,checkpoint,capacity,eviction} as lightweight
replacements for k8s.io/kubernetes internals
- Fix unchecked error in checkpoint unmarshal
- Fix "scoket_id" typo
- Fix shadowed builtin `cap` variable
- Replace all deprecated ioutil usage with os package
Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
- Reject eviction threshold percentages outside 0-100 range - Reject reversed CPU ranges (end < start) in capacity parser - Validate single CPU values are numeric - Lower noisy klog.Infof to V(4) in reservation calculation - Add table-driven unit tests for cpuset, checkpoint, capacity, and eviction packages covering edge cases and invalid inputs Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Rewrite the build system for modern Go development:
- Makefile: add lint (golangci-lint), test (-race), and clean targets;
decouple image build from local binary via multi-stage Docker
- Dockerfile: multi-stage build (golang:1.25 builder + distroless/static
nonroot runtime) replacing the bare alpine image
- .golangci.yml: golangci-lint v2 config with errcheck, govet,
ineffassign, staticcheck, unused, and revive enabled
- GitHub Actions CI: lint, test, and build jobs on push/PR
- pkg/version: new package for build-time metadata via ldflags
Fix lint violations caught by the new tooling:
- main.go: for { select { case <-tick.C } } -> for range tick.C
- machineinfo: rename cap variable to avoid shadowing builtin
- numatopo: getNumaNodeCpuCap -> getNumaNodeCPUCap,
getCoreIDSocketIDForCpu -> getCoreIDSocketIDForCPU
- Add package comments to all packages
Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Implement signal handling with signal.NotifyContext so the exporter shuts down cleanly on SIGTERM/SIGINT. This is critical for Kubernetes pods which receive SIGTERM before being killed. Add sync.RWMutex protection for the package-level numaMap and config variables. While the current implementation is single-goroutine, this makes the code safe for future concurrency and prevents subtle bugs. Extract the MY_NODE_NAME environment variable into a named constant and improve error messages for missing configuration. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Code Review
This pull request introduces several structural improvements to the resource-exporter, including the addition of internal packages for capacity, checkpoint, cpuset, and eviction logic. It also enhances the build process via Makefile updates, adds graceful shutdown handling with signal notifications, and ensures thread-safety for shared state by introducing mutexes. The code review identified an opportunity to improve the robustness of the countCPUs function by providing more descriptive error handling for malformed input strings, which has been acknowledged as a valid improvement.
| if len(parts) == 1 { | ||
| if _, err := strconv.Atoi(strings.TrimSpace(parts[0])); err != nil { | ||
| return 0, err | ||
| } | ||
| count++ | ||
| } else if len(parts) == 2 { | ||
| start, err := strconv.Atoi(strings.TrimSpace(parts[0])) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| end, err := strconv.Atoi(strings.TrimSpace(parts[1])) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| if end < start { | ||
| return 0, fmt.Errorf("invalid CPU range: start %d > end %d", start, end) | ||
| } | ||
| count += end - start + 1 | ||
| } |
There was a problem hiding this comment.
The current implementation of countCPUs silently ignores invalid range parts, such as 0-1-2 in a string like 0-1-2,3. This could lead to an incorrect CPU count without any indication of an error. It would be more robust to return an error for such malformed range parts. Using a switch statement can make the logic clearer and handle invalid formats explicitly. This change also improves error messages for better diagnostics.
switch len(parts) {
case 1:
if _, err := strconv.Atoi(strings.TrimSpace(parts[0])); err != nil {
return 0, fmt.Errorf("invalid CPU ID in range part %q: %w", r, err)
}
count++
case 2:
start, err := strconv.Atoi(strings.TrimSpace(parts[0]))
if err != nil {
return 0, fmt.Errorf("invalid start CPU ID in range part %q: %w", r, err)
}
end, err := strconv.Atoi(strings.TrimSpace(parts[1]))
if err != nil {
return 0, fmt.Errorf("invalid end CPU ID in range part %q: %w", r, err)
}
if end < start {
return 0, fmt.Errorf("invalid CPU range: start %d > end %d in %q", start, end, r)
}
count += end - start + 1
default:
return 0, fmt.Errorf("invalid CPU range format: %q", r)
}
Summary
signal.NotifyContext(SIGTERM/SIGINT)sync.RWMutexprotection for package-level mutable state (numaMap,config)Stacked PR
This PR is stacked on top of PR #16. The relevant commit for review is
b77e504.Related Issues
Design
Graceful shutdown
The main loop now uses
signal.NotifyContextto catch SIGTERM and SIGINT. When a signal is received:defer tick.Stop())defer klog.Flush())This is critical for Kubernetes where pods receive SIGTERM before being forcefully killed. Without signal handling, the process would be mid-operation when killed, potentially leaving stale state.
Concurrency safety
Package-level mutable state is now protected with
sync.RWMutex:numaMapinframework.go— holds registered topology info providers and their latest stateconfiginkubeletconfig.go— holds kubelet policy and reservation dataWrite operations (
RegisterNumaType,TopoInfoUpdate,TryUpdatingResourceReservation) take a write lock. Read operations (GetPolicy,GetResReserved,GetAllResAllocatableInfo,GetCpusDetail) take a read lock.While the current implementation is single-goroutine, this makes the code safe for future concurrency (e.g., adding an HTTP health endpoint on a separate goroutine) and prevents subtle bugs.
Constants
The
MY_NODE_NAMEenvironment variable string is extracted into aconst envNodeNamein bothmain.goandupdate.go, eliminating magic strings and improving error messages.AI Assistance Disclosure
This PR was developed with AI guidance. The author reviewed, directed, and validated all changes carefully.
Closes