OCPBUGS-77990: kubelet: close resize race in sync pod - #2743
haircommander wants to merge 2 commits into
Conversation
Formerly there was a race condition where SyncPod would check whether a resize was in progress using a stale snapshot of the pod. The pod worker snapshots the allocation at UpdatePod time, but the allocation manager goroutine may accept a new resize before SyncPod runs, so IsPodResizeInProgress could compare the stale allocation against actuated state and prematurely clear the PodResizeInProgress condition. Move the resize progress check into the allocation manager (CheckResizeProgress), where it is serialized with retryPendingResizes under allocationMutex. This reads the current allocation and manages the condition atomically, closing the race. Signed-off-by: Peter Hunt <pehunt@redhat.com>
The stale-allocation race in SyncPod resize completion detection that caused pod-resize-retry-deferred-test-2 to fail is fixed by UPSTREAM: 141325. Remove the OCPBUGS-64847 skip so the test runs again. Signed-off-by: Peter Hunt <pehunt@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci-robot
added
jira/valid-reference
|
@haircommander: This pull request references Jira Issue OCPBUGS-77990, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@haircommander: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haircommander 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 |
WalkthroughThe kubelet now delegates pod resize-progress evaluation to the allocation manager. Runtime resize state drives condition updates and completion events. Tests add asynchronous synchronization and cover stale allocation snapshots and completed resizes. ChangesPod resize progress
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SyncPod
participant ContainerRuntime
participant AllocationManager
SyncPod->>ContainerRuntime: Read pod resize state
SyncPod->>AllocationManager: CheckResizeProgress with runtime predicate
AllocationManager->>AllocationManager: Apply allocation and update condition
AllocationManager-->>SyncPod: Return cleared generation
SyncPod-->>SyncPod: Emit ResizeCompleted when resizing is complete
🚥 Pre-merge checks | ✅ 5 | ❌ 10❌ Failed checks (10 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
haircommander
changed the title
haircommander
changed the title
|
@haircommander: This pull request references Jira Issue OCPBUGS-77990, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/kubelet/container/testing/fake_runtime.go (1)
595-599: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the field reads with the existing mutex.
FakeRuntimeembedssync.Mutex, and methods such asSyncPodlock it before they touch fields.IsPodResizeInProgressreadsPodResizeInProgressFuncandPodResizeInProgresswithout the lock. Tests now assign these fields while the allocation manager can dispatchSyncPodon a separate goroutine (seepkg/kubelet/kubelet_test.golines 356-362). That combination can trigger a-racefailure.Note that the callback itself runs while the allocation manager holds
allocationMutex, so keep the locked section limited to the field reads.♻️ Proposed change
func (f *FakeRuntime) IsPodResizeInProgress(allocatedPod *v1.Pod, podStatus *kubecontainer.PodStatus) bool { - if f.PodResizeInProgressFunc != nil { - return f.PodResizeInProgressFunc(allocatedPod, podStatus) + f.Lock() + fn := f.PodResizeInProgressFunc + inProgress := f.PodResizeInProgress + f.Unlock() + if fn != nil { + return fn(allocatedPod, podStatus) } - return f.PodResizeInProgress + return inProgress }🤖 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 `@pkg/kubelet/container/testing/fake_runtime.go` around lines 595 - 599, Update FakeRuntime.IsPodResizeInProgress to lock the embedded mutex while reading PodResizeInProgressFunc and PodResizeInProgress, but release it before invoking the callback; preserve the existing callback-precedence and fallback behavior.pkg/kubelet/allocation/allocation_manager.go (2)
113-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the callback runs while
allocationMutexis held.
CheckResizeProgressinvokesisResizeInProgressinside the lock. A callback that calls any otherManagermethod deadlocks, becausesync.Mutexis not reentrant. State this constraint in the interface comment so future callers do not re-enter the manager.📝 Proposed doc addition
// The isResizeInProgress callback receives the pod with current allocation // applied and should return true if the resize has not yet been actuated. + // The callback is invoked while the allocation lock is held. It must not + // call back into the allocation Manager, and it should not block. // Returns the cleared generation and true if the resize completed.🤖 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 `@pkg/kubelet/allocation/allocation_manager.go` around lines 113 - 121, Update the interface comment for CheckResizeProgress to state that isResizeInProgress executes while allocationMutex is held and must not call other Manager methods or otherwise re-enter the manager. Keep the existing callback behavior and return-value documentation unchanged.
558-566: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument that
CheckResizeProgressinvokes the callback while holdingallocationMutex
kubeGenericRuntimeManager.IsPodResizeInProgressperforms only in-memory checks and state reads. Keep the callback contract explicit: it must not re-enter the allocation manager or perform blocking work.🤖 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 `@pkg/kubelet/allocation/allocation_manager.go` around lines 558 - 566, Document in CheckResizeProgress that isResizeInProgress executes while allocationMutex is held, and state that callbacks must perform only non-blocking in-memory checks without re-entering the allocation manager. Keep the existing locking and callback behavior unchanged.pkg/kubelet/kubelet_test.go (1)
4215-4245: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the generation carried by the completion path.
CheckResizeProgressreturns the cleared generation, andSyncPodputs that generation into theResizeCompletedmessage. The subtest checks only that the event string containsResizeCompleted. Assert the generation too, so a regression in the returned value is caught.
pod.Generationis 2 and the condition is set with that generation at line 4221, so the message should carry generation 2.♻️ Proposed assertion
var foundCompleted bool for len(fakeRecorder.Events) > 0 { event := <-fakeRecorder.Events if strings.Contains(event, "ResizeCompleted") { foundCompleted = true + require.Contains(t, event, `"generation":2`, "ResizeCompleted event should carry the cleared generation") } } require.True(t, foundCompleted, "expected ResizeCompleted event")🤖 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 `@pkg/kubelet/kubelet_test.go` around lines 4215 - 4245, Strengthen the “completed resize should clear condition and emit event” subtest by asserting that the captured ResizeCompleted event also contains generation 2, matching pod.Generation and the generation passed to SetPodResizeInProgressCondition. Keep the existing event-presence assertion and inspect the event text while draining fakeRecorder.Events.
🤖 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.
Inline comments:
In `@pkg/kubelet/kubelet_test.go`:
- Around line 348-362: Add defer testKubelet.Cleanup() to the test fixtures at
the specified locations in kubelet_node_status_test.go and kubelet_pods_test.go.
Ensure each test performs cleanup before returning so the WaitGroup is joined
and the kubelet root directory is removed.
---
Nitpick comments:
In `@pkg/kubelet/allocation/allocation_manager.go`:
- Around line 113-121: Update the interface comment for CheckResizeProgress to
state that isResizeInProgress executes while allocationMutex is held and must
not call other Manager methods or otherwise re-enter the manager. Keep the
existing callback behavior and return-value documentation unchanged.
- Around line 558-566: Document in CheckResizeProgress that isResizeInProgress
executes while allocationMutex is held, and state that callbacks must perform
only non-blocking in-memory checks without re-entering the allocation manager.
Keep the existing locking and callback behavior unchanged.
In `@pkg/kubelet/container/testing/fake_runtime.go`:
- Around line 595-599: Update FakeRuntime.IsPodResizeInProgress to lock the
embedded mutex while reading PodResizeInProgressFunc and PodResizeInProgress,
but release it before invoking the callback; preserve the existing
callback-precedence and fallback behavior.
In `@pkg/kubelet/kubelet_test.go`:
- Around line 4215-4245: Strengthen the “completed resize should clear condition
and emit event” subtest by asserting that the captured ResizeCompleted event
also contains generation 2, matching pod.Generation and the generation passed to
SetPodResizeInProgressCondition. Keep the existing event-presence assertion and
inspect the event text while draining fakeRecorder.Events.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c9a82ced-cda5-4236-8a8a-16c2047024d0
📒 Files selected for processing (5)
openshift-hack/cmd/k8s-tests-ext/disabled_tests.gopkg/kubelet/allocation/allocation_manager.gopkg/kubelet/container/testing/fake_runtime.gopkg/kubelet/kubelet.gopkg/kubelet/kubelet_test.go
💤 Files with no reviewable changes (1)
- openshift-hack/cmd/k8s-tests-ext/disabled_tests.go
| podSyncWG := &sync.WaitGroup{} | ||
| kubelet.allocationManager = allocation.NewInMemoryManager( | ||
| kubelet.statusManager, | ||
| func(pod *v1.Pod) { kubelet.HandlePodSyncs(tCtx, []*v1.Pod{pod}) }, | ||
| // The production pod worker dispatches syncs asynchronously, so the | ||
| // allocation manager can trigger a sync while holding allocationMutex | ||
| // (e.g. from retryPendingResizes) without the resulting SyncPod, which | ||
| // re-acquires allocationMutex via CheckResizeProgress, deadlocking. | ||
| // Mirror that here by running the sync on a separate goroutine. | ||
| func(pod *v1.Pod) { | ||
| podSyncWG.Add(1) | ||
| go func() { | ||
| defer podSyncWG.Done() | ||
| kubelet.HandlePodSyncs(tCtx, []*v1.Pod{pod}) | ||
| }() | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: List TestKubelet construction sites and check for Cleanup in the same function.
set -euo pipefail
fd -e go . pkg/kubelet --max-depth 1 | while IFS= read -r f; do
rg -n 'newTestKubelet[A-Za-z]*\(' "$f" || true
done
echo "--- Cleanup call sites ---"
rg -n --type=go '\.Cleanup\(\)' pkg/kubeletRepository: openshift/kubernetes
Length of output: 15697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- TestKubelet definition and Cleanup ---"
rg -n -A35 -B8 'type TestKubelet struct|func \(.*TestKubelet.*Cleanup' pkg/kubelet
echo "--- Construction sites with enclosing test/function context ---"
python3 - <<'PY'
from pathlib import Path
import re
for path in sorted(Path("pkg/kubelet").glob("*_test.go")):
text = path.read_text()
lines = text.splitlines()
starts = [i for i, line in enumerate(lines) if re.search(r'\bnewTestKubelet(?:WithImageList|ExcludeAdmitHandlers)?\s*\(', line)]
if not starts:
continue
print(f"\n{path}")
for i in starts:
# Show the nearest preceding function declaration and a bounded local region.
fn = next((j for j in range(i, -1, -1) if re.match(r'\s*func\b', lines[j])), None)
end = min(len(lines), i + 8)
print(f"construction line {i+1}; enclosing declaration line {(fn+1) if fn is not None else '?'}")
for j in range(max(0, i-1), end):
print(f"{j+1}: {lines[j]}")
PY
echo "--- All Cleanup references in kubelet test files ---"
rg -n --type=go '\b(?:testKubelet|testKubeletNoReset|tk)\.Cleanup\(\)|\.Cleanup\(func' pkg/kubeletRepository: openshift/kubernetes
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- TestKubelet definition and Cleanup ---"
rg -n -A35 -B8 'type TestKubelet struct|func \(.*TestKubelet.*Cleanup' pkg/kubelet
echo "--- Construction sites with enclosing test/function context ---"
python3 - <<'PY'
from pathlib import Path
import re
for path in sorted(Path("pkg/kubelet").glob("*_test.go")):
text = path.read_text()
lines = text.splitlines()
starts = [i for i, line in enumerate(lines) if re.search(r'\bnewTestKubelet(?:WithImageList|ExcludeAdmitHandlers)?\s*\(', line)]
if not starts:
continue
print(f"\n{path}")
for i in starts:
fn = next((j for j in range(i, -1, -1) if re.match(r'\s*func\b', lines[j])), None)
end = min(len(lines), i + 8)
print(f"construction line {i+1}; enclosing declaration line {(fn+1) if fn is not None else '?'}")
for j in range(max(0, i-1), end):
print(f"{j+1}: {lines[j]}")
PY
echo "--- All Cleanup references in kubelet test files ---"
rg -n --type=go '\b(?:testKubelet|testKubeletNoReset|tk)\.Cleanup\(\)|\.Cleanup\(func' pkg/kubeletRepository: openshift/kubernetes
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Target test and related synchronization ---"
sed -n '320,390p' pkg/kubelet/kubelet_node_status_test.go
rg -n -C4 'podSyncWG|triggerPodSync|HandlePodSyncs|retryPendingResizes|enableResizing|NewInMemoryManager' pkg/kubelet/kubelet_test.go pkg/kubelet/*_test.go
echo "--- Construction sites without a nearby Cleanup ---"
python3 - <<'PY'
from pathlib import Path
import re
construct = re.compile(r'\b(newTestKubelet(?:WithImageList|ExcludeAdmitHandlers)?)\s*\(')
cleanup = re.compile(r'\b(?:testKubelet|testKubeletNoReset|tk)\.Cleanup\s*\(\)|\.Cleanup\s*\(\s*func')
for path in sorted(Path("pkg/kubelet").glob("*_test.go")):
lines = path.read_text().splitlines()
for i, line in enumerate(lines):
if not construct.search(line):
continue
# Skip helper definitions and calls inside helper implementations.
if re.match(r'\s*func\b', line) or i < 230 and path.name == "kubelet_test.go":
continue
window = "\n".join(lines[i:min(len(lines), i+12)])
if not cleanup.search(window):
print(f"{path}:{i+1}: {line.strip()}")
PY
echo "--- Missing-cleanup test bodies ---"
sed -n '1645,1685p' pkg/kubelet/kubelet_node_status_test.go
sed -n '2160,2205p' pkg/kubelet/kubelet_node_status_test.go
sed -n '2455,2510p' pkg/kubelet/kubelet_node_status_test.go
sed -n '3575,3625p' pkg/kubelet/kubelet_pods_test.goRepository: openshift/kubernetes
Length of output: 37402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Target test and related synchronization ---"
sed -n '320,390p' pkg/kubelet/kubelet_node_status_test.go
rg -n -C4 'podSyncWG|triggerPodSync|HandlePodSyncs|retryPendingResizes|enableResizing|NewInMemoryManager' pkg/kubelet/kubelet_test.go pkg/kubelet/*_test.go
echo "--- Construction sites without a nearby Cleanup ---"
python3 - <<'PY'
from pathlib import Path
import re
construct = re.compile(r'\b(newTestKubelet(?:WithImageList|ExcludeAdmitHandlers)?)\s*\(')
cleanup = re.compile(r'\b(?:testKubelet|testKubeletNoReset|tk)\.Cleanup\s*\(\)|\.Cleanup\s*\(\s*func')
for path in sorted(Path("pkg/kubelet").glob("*_test.go")):
lines = path.read_text().splitlines()
for i, line in enumerate(lines):
if not construct.search(line):
continue
if re.match(r'\s*func\b', line) or i < 230 and path.name == "kubelet_test.go":
continue
window = "\n".join(lines[i:min(len(lines), i+12)])
if not cleanup.search(window):
print(f"{path}:{i+1}: {line.strip()}")
PY
echo "--- Missing-cleanup test bodies ---"
sed -n '1645,1685p' pkg/kubelet/kubelet_node_status_test.go
sed -n '2160,2205p' pkg/kubelet/kubelet_node_status_test.go
sed -n '2455,2510p' pkg/kubelet/kubelet_node_status_test.go
sed -n '3575,3625p' pkg/kubelet/kubelet_pods_test.goRepository: openshift/kubernetes
Length of output: 37402
Add cleanup for all uncleaned TestKubelet fixtures
Add defer testKubelet.Cleanup() at pkg/kubelet/kubelet_node_status_test.go:1659, pkg/kubelet/kubelet_node_status_test.go:2468, and pkg/kubelet/kubelet_pods_test.go:3590. These tests otherwise skip the WaitGroup join and kubelet root-directory cleanup.
🤖 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 `@pkg/kubelet/kubelet_test.go` around lines 348 - 362, Add defer
testKubelet.Cleanup() to the test fixtures at the specified locations in
kubelet_node_status_test.go and kubelet_pods_test.go. Ensure each test performs
cleanup before returning so the WaitGroup is joined and the kubelet root
directory is removed.
|
@haircommander: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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. |
|
@haircommander: This pull request references Jira Issue OCPBUGS-77990. The bug has been updated to no longer refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
trying #2765 instead |
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Formerly there was a race condition where SyncPod would check whether a resize was in progress using a stale snapshot of the pod. The pod worker snapshots the allocation at UpdatePod time, but the allocation manager goroutine may accept a new resize before SyncPod runs, so IsPodResizeInProgress could compare the stale allocation against actuated state and prematurely clear the PodResizeInProgress condition.
Move the resize progress check into the allocation manager (CheckResizeProgress), where it is serialized with retryPendingResizes under allocationMutex. This reads the current allocation and manages the condition atomically, closing the race.
fixes flakes in pod-resize-retry-deferred-test-2 seen in openshift CI
Which issue(s) this PR is related to:
Special notes for your reviewer:
co-authored by claude 4.8
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Summary by CodeRabbit