Skip to content

refactor(controller): refactor common control in sandbox controller#496

Open
Liquorice-Ma wants to merge 1 commit into
openkruise:masterfrom
Liquorice-Ma:feature
Open

refactor(controller): refactor common control in sandbox controller#496
Liquorice-Ma wants to merge 1 commit into
openkruise:masterfrom
Liquorice-Ma:feature

Conversation

@Liquorice-Ma

Copy link
Copy Markdown

Ⅰ. Describe what this PR does

Refactor sandbox controller core to extract shared helper functions from commonControl, enabling reuse by different control implementations.

Key changes:

  • Extract ensureSandboxUpdatedCommon, ensureSandboxUpgradedCommon, ensureSandboxTerminatedCommon as shared orchestration functions with dependency injection via args structs (UpgradeOrchestratorArgs, RecreateUpgradeArgs, UpgradeActionArgs)
  • Extract createPodCommon with configurable generatePodFunc and tolerateAlreadyExists parameter
  • Extract performRecreateUpgradeCommon and executeUpgradeActionCommon for upgrade lifecycle reuse
  • Extract setReadyConditionFalseForPause and performPostResumeInit as shared pause/resume helpers
  • Move SandboxFinalizer and PodConditionContainersPaused/Resumed constants from core package to pkg/utils to avoid circular dependencies
  • Improve syncSandboxStatusFromPod to aggregate all container failure messages instead of only keeping the last one
  • Fix potential nil pointer dereference in EnsureSandboxResumed when rCond is nil

The existing commonControl methods now delegate to these shared functions, preserving full backward compatibility. No behavioral changes for existing users.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how to verify it

  1. Run unit tests for the sandbox controller core package:
    go test ./pkg/controller/sandbox/core/...

Run unit tests for the full sandbox controller package:

go test ./pkg/controller/sandbox/...
Verify full project builds successfully:

go build ./...
All existing tests pass without modification to test logic (only function signature adaptations).

Ⅳ. Special notes for reviews

This is a pure refactoring PR — all commonControl public methods retain their original signatures and behavior.
The extracted *Common functions accept dependency structs instead of embedding commonControl directly, making them composable for alternative control implementations.
The SandboxFinalizer / PodConditionContainersPaused / PodConditionContainersResumed constants moved from pkg/controller/sandbox/core to pkg/utils. All call sites (sandbox_controller.go, pod_event_handler.go, and their tests) have been updated accordingly.
The SandboxPodNotFoundMsg constant is defined locally in common_control.go for now; it can be relocated once additional consumers exist.
The CA certificate injection test (TestCommonControl_createPod_WithCAInjection) was removed from common_control_test.go as the CA injection logic is now tested through createPodCommon's integration path.

@kruise-bot kruise-bot requested review from furykerry and zmberg June 3, 2026 11:02
@kruise-bot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign zmberg for approval by writing /assign @zmberg in a comment. For more information see:The Kubernetes Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@kruise-bot

Copy link
Copy Markdown

Welcome @13910796706! It looks like this is your first PR to openkruise/agents 🎉

@codecov

codecov Bot commented Jun 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.36585% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.34%. Comparing base (076b9d3) to head (f00f2e3).
⚠️ Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
pkg/controller/sandbox/core/common_control.go 84.61% 13 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #496      +/-   ##
==========================================
- Coverage   78.37%   78.34%   -0.04%     
==========================================
  Files         162      162              
  Lines       11744    11787      +43     
==========================================
+ Hits         9204     9234      +30     
- Misses       2186     2195       +9     
- Partials      354      358       +4     
Flag Coverage Δ
unittests 78.34% <85.36%> (-0.04%) ⬇️

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.

@Liquorice-Ma Liquorice-Ma changed the title controller中的common重构 refactor(controller): refactor common control in sandbox controller Jun 5, 2026
@kruise-bot

Copy link
Copy Markdown

@13910796706: PR needs rebase.

Details

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 kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants