fix(inplaceupdate): patch Pod condition instead of full status update#2507
fix(inplaceupdate): patch Pod condition instead of full status update#2507astrox131 wants to merge 1 commit into
Conversation
|
[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 |
|
Welcome @astrox131! It looks like this is your first PR to openkruise/kruise 🎉 |
There was a problem hiding this comment.
Pull request overview
This PR hardens Kruise in-place update readiness-gate writes by switching from full pods/status updates to a strategic-merge patch that targets only status.conditions, preventing older vendored Pod types from unintentionally clearing newer upstream status fields (notably DRA-related fields) during in-place updates.
Changes:
- Add
PatchPodStatussupport topodadapter.AdapterWithPatch(runtime + typed clients). - Update
inplaceupdate.updateConditionto patch thestatussubresource usingStrategicMergeFrom(..., MergeFromWithOptimisticLock{})instead ofStatus().Update(). - Add a wire-level unit test to assert the emitted request is a strategic-merge patch on
pods/statusaffecting onlystatus.conditions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/util/podadapter/adapter.go | Extends patch-capable adapter interface with PatchPodStatus and implements it for runtime/typed clients. |
| pkg/util/inplaceupdate/inplace_update.go | Switches readiness-gate condition writes to a status subresource strategic-merge patch with optimistic locking. |
| pkg/util/inplaceupdate/inplace_update_condition_test.go | Adds a unit test intercepting the status patch request to ensure only status.conditions is patched. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Adapter | ||
| PatchPod(pod *v1.Pod, patch client.Patch) (*v1.Pod, error) | ||
| PatchPodResource(pod *v1.Pod, patch client.Patch) (*v1.Pod, error) | ||
| PatchPodStatus(pod *v1.Pod, patch client.Patch) (*v1.Pod, error) | ||
| } |
| var body map[string]interface{} | ||
| if err := json.Unmarshal(gotData, &body); err != nil { | ||
| t.Fatalf("patch body is not valid JSON: %v (%s)", err, gotData) | ||
| } | ||
| status, ok := body["status"].(map[string]interface{}) | ||
| if !ok { | ||
| t.Fatalf("patch body has no status section: %s", gotData) | ||
| } |
In-place update writes the InPlaceUpdateReady condition via a full UpdateStatus. That round-trips the entire PodStatus through kruise's vendored (older) core/v1 types and silently drops any status fields kruise does not know about. On a cluster newer than the vendored API this clears the DRA status fields (ExtendedResourceClaimStatus / ResourceClaimStatuses / NodeAllocatableResourceClaimStatuses), which can break running pods -- e.g. a DRA-backed device disappears from the container on its next in-place restart (Found no NVIDIA driver). Switch updateCondition to a strategic-merge status patch that only carries the condition change, so unknown status fields are left untouched server-side. An optimistic lock preserves the previous "this Pod version only" behavior (see openkruise#2274). Adds PatchPodStatus to the podadapter (runtime + typed clients). Related: kubernetes/kubernetes#139772 Signed-off-by: astrox <chengyongxue@meshy.ai>
astrox131
force-pushed
the
fix/inplace-condition-status-patch
branch
from
8870da2 to
20ac073
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2507 +/- ##
==========================================
- Coverage 49.91% 49.88% -0.03%
==========================================
Files 325 325
Lines 28437 28448 +11
==========================================
- Hits 14194 14191 -3
- Misses 12588 12599 +11
- Partials 1655 1658 +3
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:
|
Ⅰ. Describe what this PR does
In-place update writes the
InPlaceUpdateReadyPod condition with a full status update (Status().Update). That round-trips the entirePodStatusthrough kruise's vendored (currentlyk8s.io/api v0.32.x)core/v1types, so any status field the vendored types don't know about is absent from the object kruise sends back and gets cleared server-side.On a cluster newer than the vendored API this clears the DRA status fields —
status.extendedResourceClaimStatus,status.resourceClaimStatuses,status.nodeAllocatableResourceClaimStatuses. For a running Pod that uses DRA (e.g. a GPU requested via theDRAExtendedResourcemapping), clearingextendedResourceClaimStatusmeans the kubelet can no longer resolve the claim's CDI devices on the next in-place container restart — the restarted container comes up with no device (Found no NVIDIA driver / No CUDA device) and CrashLoops. A normal crash-restart is fine; only kruise's in-place image update triggers the status write that drops the field.This PR switches
updateConditionto a strategic-merge patch of onlystatus.conditions, so status fields kruise doesn't manage are left untouched. It also future-proofs against any new Pod-status field added upstream ahead of kruise's vendored deps.PatchPodStatusis added toAdapterWithPatch(runtime + typed clients); the optimistic lock preserves the existing "apply only to this Pod version" behavior (#2274).Ⅱ. Does this pull request fix one issue?
NONE.
(Same root cause and field set as the upstream apiserver work in kubernetes/kubernetes#139772 and kubernetes/kubernetes#139876 — "old clients clearing DRA status via
pods/status". This fixes it on the kruise client side, which can land sooner and is defense-in-depth regardless of the apiserver change.)Ⅲ. Describe how to verify it
Reproduction: a
CloneSetwithupdateStrategy.type=InPlaceIfPossible, one container requesting a DRA-backed device (limits: deviceclass.resource.kubernetes.io/gpu.nvidia.com: "1"), on k8s ≥ 1.34 with theDRAExtendedResourcefeature. Trigger an in-place image bump — before this PR the restarted container loses the device; after it, the device persists.Verified end-to-end on RKE2 v1.35.4: the healthy container's OCI runtime spec had the 5 nvidia device nodes; after the in-place image swap the new container's spec had 0 devices and
status.extendedResourceClaimStatuswasnull. With the patch the status field survives and the device is re-injected.Unit test
TestUpdateConditionPatchesStatusOnlyasserts the emitted request is a strategic-merge patch on thestatussubresource whose body touches onlyconditions. This has to be checked at the wire level: the typed fake client cannot even hold a field the vendored types lack, so it cannot reproduce the clobber directly.Ⅳ. Special notes for reviews
k8s.io/apito ≥1.34 is a much larger dependency change and only moves the boundary.AdapterInformeris not anAdapterWithPatch, soupdateConditionfalls back to the existingUpdatePodStatusfor it (the informer/unit-test path), unchanged.Conflicton a staleresourceVersionexactly as the previous RV-pinnedUpdatedid; no retry/refetch is introduced.