fix(inventory): add diagnostics for negative resource accounting#414
Open
chalabi2 wants to merge 1 commit into
Open
fix(inventory): add diagnostics for negative resource accounting#414chalabi2 wants to merge 1 commit into
chalabi2 wants to merge 1 commit into
Conversation
Keep raw operator inventory intact so negative resource accounting remains visible for debugging, while status, metrics, and bidding consume a sanitized view. Add diagnostics around pod and storage subtraction to log the previous inventory state and the applied change when accounting goes negative. Replaces #379. Closes akash-network/support#429 Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
WalkthroughThis PR introduces structured diagnostic logging for negative resource quantities in inventory management, replaces silent NLZ-clamping with log-and-keep subtraction helpers across node, Ceph, and Rancher storage operators, adds a sanitized ChangesNegative inventory quantity sanitization and diagnostic logging
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chalabi2
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/inventory/node-discovery.go (1)
782-805: 🎯 Functional Correctness | 🟠 MajorMove EmptyDir accounting out of the container loop in both functions.
pod.Spec.Volumesis pod-scoped, but the volumes loop is nested inside the container loop. This causes memory accounting to be processed once per container instead of once per pod. Multi-container pods will incorrectly add or subtract memory multiple times, creating false accounting.This bug exists in both
addPodAllocatedResources(line 735-741) andsubPodAllocatedResources(line 799-805). Move both volumes loops outside their respective container loops so they execute once per pod.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/inventory/node-discovery.go` around lines 782 - 805, The volumes loop that handles EmptyDir memory accounting is incorrectly nested inside the container loop in both the subPodAllocatedResources and addPodAllocatedResources functions. Since pod.Spec.Volumes is pod-scoped and not container-scoped, this causes the memory accounting to be processed once per container instead of once per pod, leading to incorrect totals for multi-container pods. Move the entire volumes loop (the for loop iterating over pod.Spec.Volumes) outside and after the container loop by unindenting it one level, so it executes once per pod rather than once per container, in both functions.
🧹 Nitpick comments (2)
operator/inventory/node-discovery_test.go (1)
76-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression case for
memory.emptydirwith multi-container pods.This table currently validates cpu/gpu/memory requests but does not exercise the new EmptyDir subtraction path. A dedicated multi-container case will lock in correct pod-scoped EmptyDir accounting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/inventory/node-discovery_test.go` around lines 76 - 204, Add a new test case to the tests table in TestSubPodAllocatedResourcesLogsAndKeepsNegative function to validate the EmptyDir subtraction path with multi-container pods. Create a test case entry with appropriate fields (name, resourceName, requestQuantity, changeResource, before, subtracted, after, setAllocated, allocated callbacks) that exercises memory.emptydir resource handling specifically with a pod containing multiple containers in the Spec.Containers field to ensure correct pod-scoped EmptyDir accounting.operator/inventory/state.go (1)
84-84: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid re-logging identical negative snapshots on every signal.
This branch can emit the same large
inventory/negative_resourcespayload repeatedly while negatives persist. Consider deduping by last logged fingerprint or throttling to reduce log churn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/inventory/state.go` at line 84, The logNegativeClusterInventory function call is logging identical negative resource snapshots repeatedly whenever negative inventory persists, causing excessive log output. To fix this, implement deduplication logic in the logNegativeClusterInventory function by either tracking the last logged fingerprint of the negative resources and only logging when the fingerprint changes, or by implementing a throttling mechanism to limit how frequently the same negative snapshot is logged. This will reduce log churn while still capturing important state changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@operator/inventory/node-discovery.go`:
- Around line 782-805: The volumes loop that handles EmptyDir memory accounting
is incorrectly nested inside the container loop in both the
subPodAllocatedResources and addPodAllocatedResources functions. Since
pod.Spec.Volumes is pod-scoped and not container-scoped, this causes the memory
accounting to be processed once per container instead of once per pod, leading
to incorrect totals for multi-container pods. Move the entire volumes loop (the
for loop iterating over pod.Spec.Volumes) outside and after the container loop
by unindenting it one level, so it executes once per pod rather than once per
container, in both functions.
---
Nitpick comments:
In `@operator/inventory/node-discovery_test.go`:
- Around line 76-204: Add a new test case to the tests table in
TestSubPodAllocatedResourcesLogsAndKeepsNegative function to validate the
EmptyDir subtraction path with multi-container pods. Create a test case entry
with appropriate fields (name, resourceName, requestQuantity, changeResource,
before, subtracted, after, setAllocated, allocated callbacks) that exercises
memory.emptydir resource handling specifically with a pod containing multiple
containers in the Spec.Containers field to ensure correct pod-scoped EmptyDir
accounting.
In `@operator/inventory/state.go`:
- Line 84: The logNegativeClusterInventory function call is logging identical
negative resource snapshots repeatedly whenever negative inventory persists,
causing excessive log output. To fix this, implement deduplication logic in the
logNegativeClusterInventory function by either tracking the last logged
fingerprint of the negative resources and only logging when the fingerprint
changes, or by implementing a throttling mechanism to limit how frequently the
same negative snapshot is logged. This will reduce log churn while still
capturing important state changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1adbed34-6fd1-4f4d-8933-4e24331e3bef
📒 Files selected for processing (12)
cluster/inventory.gocluster/inventory_test.gocluster/kube/operators/clients/inventory/client.gocluster/kube/operators/clients/inventory/client_test.gocluster/kube/operators/clients/inventory/inventory.gooperator/inventory/ceph.gooperator/inventory/diagnostics.gooperator/inventory/diagnostics_test.gooperator/inventory/node-discovery.gooperator/inventory/node-discovery_test.gooperator/inventory/rancher.gooperator/inventory/state.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces #379 with a diagnostic fix for akash-network/support#429.
This keeps raw operator inventory intact so negative resource accounting remains visible, while bid, metrics, and provider status consume a sanitized non-negative view.
It also logs forensic data when accounting goes negative: pod/PV identity,
inventory_before, and the applied change. Rancher storage accounting now uses signed resource quantities instead ofuint64arithmetic so underflows stay visible instead of becoming huge positive values.Fixes akash-network/support#429.
Validation
GOWORK=off go test ./operator/inventory -count=1GOWORK=off go test ./cluster/... ./operator/inventory -count=1git diff --check