Fix/2769 update node status - #2770
Conversation
|
|
Hi @zyjhtangtang @luc99hen Sir , I just opened a PR for issue #2769 to fix the silent error suppression issue in updateNodeStatus. This update ensures that persistent errors during node condition updates are properly returned to callers instead of falling back to a stale node, and it includes a new unit test for coverage. All local tests, builds, and vet checks pass successfully. I would appreciate your feedback when you have a moment. Thank you for your time! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2770 +/- ##
==========================================
+ Coverage 46.20% 46.23% +0.03%
==========================================
Files 405 405
Lines 27540 27632 +92
==========================================
+ Hits 12724 12777 +53
- Misses 13649 13677 +28
- Partials 1167 1178 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
What type of PR is this?
/kind bug
What this PR does / why we need it:
Background
In
pkg/yurthub/proxy/autonomy/autonomy.go, theAutonomyProxyhandles node status updates via theupdateNodeStatusmethod, which utilizes a retry loop (nodeStatusUpdateRetry) to ensure node conditions are synchronized reliably between edge components and storage/cloud managers.The Bug
When intermittent or persistent errors occur during the retry loop, the code previously handled them by logging via
klog.ErrorSand continuing or breaking out of the loop based on specific error types (such asErrDirectClientMgr).However, if the loop completed with a non-nil
retNodereference (captured from a prior successful iteration or an intermediate partial fetch), the function fell through and returnedretNode, nil— completely discarding the accumulated error state.// Before (simplified logic)
for i := 0; i < nodeStatusUpdateRetry; i++ {
node, err = ap.tryUpdateNodeConditions(i, req)
if node != nil {
retNode = node
}
if errors.Is(err, ErrDirectClientMgr) {
break
} else if err != nil {
klog.ErrorS(err, "Error getting or updating node status, will retry")
} else {
return retNode, nil
}
}
if retNode == nil {
return nil, fmt.Errorf("failed to get node")
}
klog.ErrorS(err, "failed to update node autonomy status")
return retNode, nil // <--- Returns stale node with nil error even when persistent errors occurred!
Impact
Callers (such as
ServeHTTPand downstream proxy consumers) received anilerror alongside a potentially stale or un-updated node object. This silently swallowed critical storage and synchronization failures, leading the system to proceed under the false assumption that node autonomy state was successfully updated when it actually failed. This created subtle state divergence in edge node autonomy management.The Fix
hadError := falsetracking flag prior to entering the retry loop.hadError = truewhenever a non-retryable/persistent error occurs within the loop attempts.hadErroris true, the function now correctly rejects the operation by returning an appropriate error (fmt.Errorf("failed to update node autonomy status after retries")) instead of blindly returningretNode, nil.// After (fixed logic)
hadError := false
for i := 0; i < nodeStatusUpdateRetry; i++ {
node, err = ap.tryUpdateNodeConditions(i, req)
if node != nil {
retNode = node
}
if errors.Is(err, ErrDirectClientMgr) {
break
} else if err != nil {
hadError = true
klog.ErrorS(err, "Error getting or updating node status, will retry")
} else {
return retNode, nil
}
}
if retNode == nil {
return nil, fmt.Errorf("failed to get node")
}
if hadError {
return nil, fmt.Errorf("failed to update node autonomy status after retries")
}
klog.ErrorS(err, "failed to update node autonomy status")
return retNode, nil
Test Coverage
autonomy_test.gosimulating persistent errors across retry iterations where an intermediate node is present, verifying thatupdateNodeStatuscorrectly surfaces the error rather than silently returningretNode, nil.Which issue(s) this PR fixes:
Fixes #2769
Special notes for your reviewer:
go test -v ./pkg/yurthub/proxy/autonomy/...— all existing and new tests pass cleanly.go build ./pkg/yurthub/proxy/...— builds without errors.go vet ./pkg/yurthub/proxy/autonomy/...— zero warnings or issues.updateNodeStatusis called exclusively fromAutonomyProxy.ServeHTTP; downstream clients will now receive proper error propagation enabling appropriate fallback or retry behavior.Does this PR introduce a user-facing change?
NONE