fix(sandbox): avoid triggering in-place update for metadata-only changes by furykerry · Pull Request #557 · openkruise/agents

furykerry · 2026-06-20T07:11:03Z

Ⅰ. Describe what this PR does

This PR optimizes the in-place update flow so that metadata-only changes (labels/annotations) bypass the full InplaceUpdate condition, preventing unnecessary Ready=False transitions for sandbox claims.

Key changes:

Metadata-only update detection (common_inplace_update_handler.go): New isMetadataOnlyChange function detects when only labels/annotations differ between the pod and sandbox template (no image or resource changes). When detected, the controller patches pod metadata directly without setting the InplaceUpdate condition, avoiding blocked sandbox readiness.
Export ResourcesEqual (inplace_update.go): Renamed resourcesEqual to exported ResourcesEqual and replaced all reflect.DeepEqual usages with it in DefaultGeneratePatchBodyFunc, DefaultGenerateResizeSubresourceBody, and isPodResourceResizeCompleted. The subset-based comparison ignores system-injected resources (e.g., ephemeral-storage from LimitRanger) so they don't trigger false in-place updates.
Checkpoint utils consistency (checkpoint/utils.go): Uses exported ResourcesEqual instead of reflect.DeepEqual for consistent resource comparison.
MergePodLabels utility (interface.go, create.go): New MergePodLabels(sandbox, labels) function centralizes pod label propagation. create.go refactored to use it.
Claim validation message (claim.go): Updated error message to "requires at least one of image or resources to be set".

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how to verify it

Run unit tests:

go test ./pkg/controller/sandbox/core/ -run TestIsMetadataOnlyChange -v
go test ./pkg/utils/inplaceupdate/ -run TestIsPodResourceResizeCompleted -v
go test ./pkg/sandbox-manager/infra/ -run TestMergePodLabels -v
go test ./pkg/sandbox-manager/infra/sandboxcr/ -run TestInfra_ClaimSandbox -v

Verify that a sandbox claim with only label/annotation changes does not set the InplaceUpdate condition
Verify that a sandbox claim with image or resource changes still goes through the full in-place update flow

Ⅳ. Special notes for reviews

isMetadataOnlyChange uses subset-based ResourcesEqual comparison — extra resources injected into the pod by admission webhooks are ignored so metadata-only changes are not mistakenly treated as in-place updates
isPodResourceResizeCompleted now uses ResourcesEqual (subset-based) instead of reflect.DeepEqual (exact match), consistent with the rest of the in-place update code
MergePodLabels tests are co-located in interface_test.go using a mockSandboxForLabels mock that implements the full Sandbox interface
TestIsPodResourceResizeCompleted is table-driven with 7 cases including edge cases for subset comparison

Appendix: Incidental Changes

Minor comment fix in pkg/cache/tasks.go (added trailing period)
Comment addition in pkg/sandbox-manager/config/infra.go clarifying metadata handling

…box ready When a claim request includes label/metadata changes that trigger an in-place update, the sandbox could be reported as ready before the controller processed the update, leading to premature readiness. - Add requireInplaceUpdateCompletion parameter to NewSandboxWaitReadyTask - Propagate request labels via InplaceUpdate.Metadata in the claim path so waitForSandboxReady waits for the InplaceUpdate condition to become True before reporting ready - Keep the clone path unchanged (creates a new pod, no in-place update) - Add InplaceUpdateMetadataOptions to config for label propagation - Update tests to cover the new wait behavior Signed-off-by: 守辰 <shouchen@U-TP9524M5-2100.local>

…Options.Metadata - Add isMetadataOnlyChange helper in handleInPlaceUpdateCommon to detect metadata-only changes (labels/annotations without image/resources diffs) - For metadata-only changes, directly patch pod metadata via control.Update without setting InplaceUpdate/Ready=False conditions - Update requireInplaceUpdateCompletion in claim.go to only be true when image or resources changed (not metadata-only) - Add fast-fail for InplaceUpdate Failed condition in NewSandboxWaitReadyTask to return error immediately instead of waiting for timeout - Remove Metadata field and InplaceUpdateMetadataOptions from config/infra.go since metadata-only updates are now handled separately by the controller - Move pod label propagation to basicSandboxCreateModifier for both claim and clone paths, replacing InplaceUpdate.Metadata usage - Add MergePodLabels/MergePodAnnotations helpers and SetPodAnnotations/ GetPodAnnotations on Sandbox interface (annotations helpers later removed as dead code after Metadata removal) - Add unit tests for requireInplaceUpdateCompletion and metadata-only change Signed-off-by: 守辰 <shouchen@U-TP9524M5-2100.local> Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

The TestSandboxManager_ClaimSandbox/Claim_with_inplace_update test was failing in CI because requireInplaceUpdateCompletion=true requires the InplaceUpdate condition to be set by the controller, but the test had no controller simulation. Add simulateInplaceUpdateControllerForApiTest goroutine and increase ClaimTimeout to 2s for InplaceUpdate test cases. Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

- Remove check for InplaceUpdating reason to avoid premature blocking - Limit in-place update readiness check to image or resources changes only - Return error immediately if InplaceUpdate condition is Failed - Skip in-place update condition check when requireInplaceUpdateCompletion is false Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

codecov · 2026-06-20T08:52:37Z

Codecov Report

❌ Patch coverage is 86.66667% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.77%. Comparing base (ce4b897) to head (3e073fb).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
...ller/sandbox/core/common_inplace_update_handler.go	79.31%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
+ Coverage   79.71%   79.77%   +0.05%     
==========================================
  Files         195      202       +7     
  Lines       14107    14721     +614     
==========================================
+ Hits        11246    11744     +498     
- Misses       2457     2549      +92     
- Partials      404      428      +24

Flag	Coverage Δ
unittests	`79.77% <86.66%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…iles - Add a comprehensive Python demo script demonstrating Qoder CLI usage via E2B SDK - Provide a Dockerfile for building a sandbox-ready image with Qoder CLI and dev tools - Include detailed README in English and Chinese documenting setup, deployment, and usage - Add SandboxSet YAML manifest for deploying Qoder CLI warm pool with persistent storage refactor(pkg/controller/checkpoint): use inplaceupdate.ResourcesEqual for resource check - Replace reflect.DeepEqual with ResourcesEqual for accurate resource comparisons test(pkg/controller/sandbox/core): add tests for metadata-only change detection - Cover various cases verifying metadata-only pod updates vs image/resource changes fix(pkg/controller/sandbox/core): treat metadata-only updates without inplace update flow - Patch pod metadata directly if only labels/annotations changed, ignoring injected resources test(pkg/sandbox-manager): enhance simulateInplaceUpdateController with resource version handling - Prevent stale update conflicts by fetching latest object before status update test(pkg/sandbox-manager): add test case for claim with metadata-only label modifications - Verify pod labels are merged via Modifier without triggering inplace update test(pkg/sandbox-manager/infra/sandboxcr): add tests for MergePodLabels utility - Check merging behavior with nil, empty, overlapping labels ensuring correct overrides refactor(pkg/utils/inplaceupdate): replace reflect.DeepEqual with ResourcesEqual consistently - Use ResourcesEqual function for semantic resource requirement equality ignoring injected fields

…e update flow - Enhance NewSandboxWaitReadyTask to optionally require inplace update completion - Add isMetadataOnlyChange helper to detect changes limited to labels/annotations - Patch pod metadata directly for metadata-only changes without setting InplaceUpdate condition - Update handleInPlaceUpdateCommon to apply direct metadata patch when only metadata changes - Add MergePodLabels utility to merge labels into sandbox pod template - Modify claim logic to distinguish metadata-only updates and skip waiting for inplace update completion - Simulate inplace update controller in tests to handle real inplace updates asynchronously - Add comprehensive tests for metadata-only changes and wait task behavior with inplace update flag - Fix minor doc improvements and error message clarification for inplace update validation - Replace reflect.DeepEqual with proper resource comparison helper for resource changes detection

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

…place-update # Conflicts: # pkg/sandbox-manager/infra/sandboxcr/sandbox_test.go

…yTask Remove the requireInplaceUpdateCompletion parameter and all related logic from NewSandboxWaitReadyTask. Restore the original InplaceUpdating reason check that blocks readiness only while an in-place update is in progress. - Revert NewSandboxWaitReadyTask signature in pkg/cache/interface.go and pkg/cache/tasks.go to remove the bool parameter - Restore the original InplaceUpdating condition check in the readiness predicate (blocks when reason == InplaceUpdating, passes otherwise) - Remove TestNewSandboxWaitReadyTask_RequireInplaceUpdateCompletion test - Remove requireInplaceUpdateCompletion logic from claim.go and clone.go waitForSandboxReady callers - Remove pre-set InplaceUpdate=True condition from core_test.go pool sandboxes that was added to satisfy the removed check Signed-off-by: shouchen <shouchen.zz@alibaba-inc.com> Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

Add missing edge cases to TestMergePodLabels table: - both empty maps - no change - empty string value - valid label - overwrite all existing labels - kubernetes-style dotted label keys - single label added to multiple existing Add TestMergePodLabels_NilTemplate to verify no panic when the sandbox's pod template is nil (SetPodLabels is a no-op, labels silently dropped). Add TestMergePodLabels_Idempotent to verify that calling MergePodLabels twice with the same labels produces the same result as calling once. Signed-off-by: shouchen <shouchen.zz@alibaba-inc.com> Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

zmberg · 2026-06-25T03:53:52Z

 			return false
 		}
-		if !resourcesEqual(c.Resources, *status.Resources) {
+		if !reflect.DeepEqual(c.Resources, *status.Resources) {


Why was this changed?

…ove tests - Replace reflect.DeepEqual with ResourcesEqual in isPodResourceResizeCompleted for consistent subset-based resource comparison, and remove unused reflect import - Convert TestIsPodResourceResizeCompleted to table-driven format with 7 cases including new edge cases: spec has more resource types than status, and status has more resource types than spec - Move MergePodLabels tests from sandboxcr/sandbox_test.go to infra/interface_test.go using a mockSandboxForLabels mock, co-locating tests with the MergePodLabels function definition Signed-off-by: shouchen <shouchen.zz@alibaba-inc.com> Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

zmberg · 2026-06-25T05:15:10Z

/lgtm
/approve

kruise-bot · 2026-06-25T05:15:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [zmberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kruise-bot requested review from AiRanthem and zmberg June 20, 2026 07:11

kruise-bot added the size/L label Jun 20, 2026

furykerry added 2 commits June 20, 2026 16:14

kruise-bot added size/XXL and removed size/L labels Jun 20, 2026

furykerry force-pushed the fix/wait-ready-inplace-update branch from dfe30c8 to 712dff2 Compare June 20, 2026 08:46

kruise-bot added size/L and removed size/XXL labels Jun 20, 2026

kruise-bot added size/XXL and removed size/L labels Jun 20, 2026

furykerry added 3 commits June 20, 2026 19:14

remove unexpected content

7082c49

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

remove unexpected content

16a7ae9

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

kruise-bot added size/XL approved and removed size/XXL labels Jun 20, 2026

zmberg removed the approved label Jun 23, 2026

kruise-bot added the needs-rebase label Jun 23, 2026

Merge remote-tracking branch 'upstream/master' into fix/wait-ready-in…

d9a4a04

…place-update # Conflicts: # pkg/sandbox-manager/infra/sandboxcr/sandbox_test.go

kruise-bot removed the needs-rebase label Jun 23, 2026

furykerry changed the title ~~fix(sandbox): wait for InplaceUpdate completion before reporting sandbox ready~~ fix(sandbox): avoid trigger complete inplaceupdate for metadata-only update Jun 25, 2026

zmberg reviewed Jun 25, 2026

View reviewed changes

furykerry changed the title ~~fix(sandbox): avoid trigger complete inplaceupdate for metadata-only update~~ fix(sandbox): avoid triggering in-place update for metadata-only changes Jun 25, 2026

kruise-bot assigned zmberg Jun 25, 2026

kruise-bot added the lgtm label Jun 25, 2026

kruise-bot added the approved label Jun 25, 2026

kruise-bot merged commit a4d5eff into openkruise:master Jun 25, 2026
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(sandbox): avoid triggering in-place update for metadata-only changes#557

fix(sandbox): avoid triggering in-place update for metadata-only changes#557
kruise-bot merged 12 commits into
openkruise:masterfrom
furykerry:fix/wait-ready-inplace-update

furykerry commented Jun 20, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jun 20, 2026 •

edited

Loading

Uh oh!

zmberg Jun 25, 2026

Uh oh!

furykerry Jun 25, 2026

Uh oh!

zmberg commented Jun 25, 2026

Uh oh!

kruise-bot commented Jun 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

furykerry commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

Appendix: Incidental Changes

Uh oh!

codecov Bot commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zmberg Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

furykerry Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

zmberg commented Jun 25, 2026

Uh oh!

kruise-bot commented Jun 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

furykerry commented Jun 20, 2026 •

edited

Loading

codecov Bot commented Jun 20, 2026 •

edited

Loading