Feature/sidecarset 2123 #2200

Colvin-Y · 2025-10-09T06:17:56Z

Ⅰ. Describe what this PR does

for issue #2123

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

Signed-off-by: Colvin-Y <[email protected]>

kruise-bot · 2025-10-09T06:18:01Z

[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 fei-guo for approval by writing /assign @fei-guo 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.

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2025-10-09T06:30:58Z

Codecov Report

❌ Patch coverage is 86.91358% with 106 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.15%. Comparing base (b57ebaf) to head (1d076b8).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/util/calculator/calculator.y	78.80%	32 Missing ⚠️
pkg/webhook/pod/mutating/sidecarset_resources.go	89.67%	20 Missing and 9 partials ⚠️
pkg/util/calculator/lexer.go	85.63%	20 Missing and 6 partials ⚠️
pkg/webhook/pod/mutating/sidecarset.go	12.50%	12 Missing and 2 partials ⚠️
pkg/util/calculator/calculator.go	94.04%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2200      +/-   ##
==========================================
+ Coverage   45.53%   46.15%   +0.62%     
==========================================
  Files         318      322       +4     
  Lines       32389    33472    +1083     
==========================================
+ Hits        14747    15448     +701     
- Misses      16207    16567     +360     
- Partials     1435     1457      +22

Flag	Coverage Δ
unittests	`46.15% <86.91%> (+0.62%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

Signed-off-by: Colvin-Y <[email protected]>

Copilot

Pull Request Overview

Introduce dynamic SidecarSet resourcesPolicy and expression engine to compute sidecar resources at pod creation time

Add ResourcesPolicy API to SidecarSet, validation/mutation logic, and CRD/schema updates
Implement a calculator package (goyacc-based) to parse/evaluate resource expressions; add comprehensive unit, fuzz, and benchmark tests
Add e2e tests and sample manifests demonstrating resourcesPolicy usage

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
test/fuzz/oss_fuzz_build.sh	Adds fuzz targets for calculator parse APIs
test/e2e/apps/v1alpha1/sidecarset_resources_policy.go	E2E tests for resourcesPolicy user stories
pkg/webhook/sidecarset/validating/sidecarset_validating_test.go	Extends validating webhook unit tests for resourcesPolicy
pkg/webhook/sidecarset/validating/sidecarset_resources_policy_test.go	Unit tests for resourcesPolicy validation helpers
pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go	Validates ResourcesPolicy and expressions in sidecarset webhook
pkg/webhook/pod/mutating/sidecarset_resources.go	Implements resources aggregation and expression evaluation
pkg/webhook/pod/mutating/sidecarset_resources_test.go	Unit tests for resources aggregation and evaluation
pkg/util/calculator/*	New expression engine (lexer, parser, tests, fuzz, benches, docs)
config/crd/bases/apps.kruise.io_sidecarsets.yaml	CRD schema extended for ResourcesPolicy
config/samples/apps_v1alpha1_sidecarset.yaml	Sample SidecarSet using resourcesPolicy
docs/proposals/20250913-sidecarset-dynamic-resources-when-creating.md	Proposal updated; regex/name clarifications and initContainers note
apis/apps/v1alpha1/*	Types and deepcopy for ResourcesPolicy
pkg/webhook/pod/mutating/sidecarset.go	Hooks resourcesPolicy application in sidecar build path
Makefile	Bumps controller-gen version
apis/apps/v1beta1/broadcastjob_conversion.go	Minor cleanup

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

test/e2e/apps/v1alpha1/sidecarset_resources_policy.go

pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go

Copilot · 2025-10-11T06:57:55Z

pkg/util/calculator/calculator.go

+	"sync"
+
+	"k8s.io/apimachinery/pkg/api/resource"
+)
+
+// Global variable for yacc parser - use mutex for thread safety
+var (
+	currentCalc   *Calculator
+	currentCalcMu sync.Mutex
+)


All Parse calls are serialized by currentCalcMu, which prevents concurrent expression evaluation across goroutines. Consider removing global state and making the parser reentrant (e.g., carry the Calculator via the lexer or use a pure/reentrant parser) so different Calculator instances can parse in parallel.

Suggested change

"sync"

"k8s.io/apimachinery/pkg/api/resource"

)

// Global variable for yacc parser - use mutex for thread safety

var (

currentCalc *Calculator

currentCalcMu sync.Mutex

)

"k8s.io/apimachinery/pkg/api/resource"

)

// Removed global parser state to allow concurrent parsing.

ditto, but hard to fix :(

Copilot · 2025-10-11T06:57:55Z

pkg/util/calculator/README.md

+- **Thread-safe**: No global state, each calculator instance is independent
+- **Resource isolation**: Expression + variables uniquely map to calculator instances  
+- **High performance**: Lock-free design for concurrent operations


The implementation uses a global mutex (currentCalcMu) during Parse, so it is not lock-free and serializes parsing. Please update the documentation or refactor the parser to remove global state and the global lock.

Copilot · 2025-10-11T06:57:56Z

pkg/webhook/pod/mutating/sidecarset_resources.go

+			// For memory, use DecimalSI
+			return resource.NewQuantity(int64(result.Number), resource.DecimalSI), nil


For memory numeric results, returning DecimalSI from a bare number is ambiguous and contradicts the comment (binary units). If a memory expression yields a plain number, convert it to bytes or Mi explicitly, e.g., treat the number as Mi: bytes := int64(result.Number * 1024 * 1024); return resource.NewQuantity(bytes, resource.BinarySI). Alternatively, reject bare numbers in memory expressions and require unit-suffixed quantities.

Suggested change

// For memory, use DecimalSI

return resource.NewQuantity(int64(result.Number), resource.DecimalSI), nil

// For memory, treat bare number as Mi and use BinarySI

bytes := int64(result.Number * 1024 * 1024)

return resource.NewQuantity(bytes, resource.BinarySI), nil

Copilot · 2025-10-11T06:57:56Z

pkg/webhook/pod/mutating/sidecarset_resources_test.go

+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name:  "large.engine.v4",


Container names with dots are invalid in Kubernetes (must be a DNS_LABEL). Replace with hyphenated names (e.g., large-engine-v4, large-engine-v8) to reflect realistic workloads and align with the proposal/docs.

Copilot · 2025-10-11T06:57:56Z

pkg/webhook/pod/mutating/sidecarset_resources_test.go

+							},
+						},
+						{
+							Name:  "large.engine.v8",


Container names with dots are invalid in Kubernetes (must be a DNS_LABEL). Replace with hyphenated names (e.g., large-engine-v4, large-engine-v8) to reflect realistic workloads and align with the proposal/docs.

Copilot · 2025-10-11T06:57:56Z

pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go

+	// empty regex is invalid
+	if len(policy.TargetContainersNameRegex) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath.Child("targetContainersNameRegex"), "invalid empty regex"))
+	} else if _, err := regexp.Compile(policy.TargetContainersNameRegex); err != nil {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("targetContainersNameRegex"), policy.TargetContainersNameRegex,


CRD sets a default of "." for targetContainersNameRegex, but the webhook rejects empty values. This mismatch can cause rejections if defaulting hasn’t been applied yet. Consider treating empty as default (".") or using field.Defaulting in the mutating path instead of erroring.

Suggested change

// empty regex is invalid

if len(policy.TargetContainersNameRegex) == 0 {

allErrs = append(allErrs, field.Required(fldPath.Child("targetContainersNameRegex"), "invalid empty regex"))

} else if _, err := regexp.Compile(policy.TargetContainersNameRegex); err != nil {

allErrs = append(allErrs, field.Invalid(fldPath.Child("targetContainersNameRegex"), policy.TargetContainersNameRegex,

// treat empty regex as default ".*"

regexPattern := policy.TargetContainersNameRegex

if len(regexPattern) == 0 {

regexPattern = ".*"

}

if _, err := regexp.Compile(regexPattern); err != nil {

allErrs = append(allErrs, field.Invalid(fldPath.Child("targetContainersNameRegex"), regexPattern,

won't fix. Validating webhook after mutating webhook, it's correct.

Copilot · 2025-10-11T06:57:57Z

pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go

+	allErrs := field.ErrorList{}
+
+	policy := container.ResourcesPolicy
+


There is no explicit validation for targetContainerMode values here, so an invalid string would only be caught by CRD validation. If you intend the webhook to enforce this too, add a NotSupported check, e.g.: if policy.TargetContainerMode != appsv1alpha1.TargetContainerModeSum && policy.TargetContainerMode != appsv1alpha1.TargetContainerModeMax { allErrs = append(allErrs, field.NotSupported(fldPath.Child("targetContainerMode"), policy.TargetContainerMode, []string{string(appsv1alpha1.TargetContainerModeSum), string(appsv1alpha1.TargetContainerModeMax)})) }

Suggested change

// Validate TargetContainerMode

if policy.TargetContainerMode != appsv1alpha1.TargetContainerModeSum &&

policy.TargetContainerMode != appsv1alpha1.TargetContainerModeMax {

allErrs = append(allErrs, field.NotSupported(

fldPath.Child("targetContainerMode"),

policy.TargetContainerMode,

[]string{string(appsv1alpha1.TargetContainerModeSum), string(appsv1alpha1.TargetContainerModeMax)},

))

}

won't fix. See crd kubebuilder marker

Copilot · 2025-10-11T06:57:57Z

pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go

+		return allErrs
+	}
+
+	// Validate TargetContainersNameRegex


There is no explicit validation for targetContainerMode values here, so an invalid string would only be caught by CRD validation. If you intend the webhook to enforce this too, add a NotSupported check, e.g.: if policy.TargetContainerMode != appsv1alpha1.TargetContainerModeSum && policy.TargetContainerMode != appsv1alpha1.TargetContainerModeMax { allErrs = append(allErrs, field.NotSupported(fldPath.Child("targetContainerMode"), policy.TargetContainerMode, []string{string(appsv1alpha1.TargetContainerModeSum), string(appsv1alpha1.TargetContainerModeMax)})) }

won't fix. See crd kubebuilder marker

furykerry · 2025-10-13T11:15:34Z

pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go

+		// Validate that initContainers do NOT have ResourcesPolicy
+		if container.ResourcesPolicy != nil {
+			allErrs = append(allErrs, field.Forbidden(idxPath.Child("resourcesPolicy"),
+				"resourcesPolicy is not supported for initContainers, only for containers"))


init container with restartpolicy always is native sidecar, can we support such initContainer ?

ditto, and will add more userStory about sidecar initContainer

Signed-off-by: Colvin-Y <[email protected]>

Colvin-Y · 2025-10-20T11:30:21Z

/retest

kruise-bot · 2025-10-20T11:30:54Z

@Colvin-Y: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

Signed-off-by: Colvin-Y <[email protected]>

furykerry · 2025-10-27T11:08:27Z

/ok-to-test

Colvin-Y and others added 7 commits October 7, 2025 14:51

calculator engine

160ee2f

Signed-off-by: Colvin-Y <[email protected]>

API Definition

9a7db82

Signed-off-by: Colvin-Y <[email protected]>

validating webhook

133f646

Signed-off-by: Colvin-Y <[email protected]>

pod mutating webhook

71e3fb8

Signed-off-by: Colvin-Y <[email protected]>

add e2e

b473e35

Signed-off-by: Colvin-Y <[email protected]>

add fuzz test

f096215

Signed-off-by: Colvin-Y <[email protected]>

add benchmark test for calculator

9ef25cb

Signed-off-by: Colvin-Y <[email protected]>

kruise-bot added the do-not-merge/work-in-progress label Oct 9, 2025

kruise-bot requested review from FillZpp and veophi October 9, 2025 06:17

kruise-bot added the size/XXL label Oct 9, 2025

Colvin-Y added 3 commits October 10, 2025 11:25

fix lexer

c293355

Signed-off-by: Colvin-Y <[email protected]>

fix lexer

01a854b

Signed-off-by: Colvin-Y <[email protected]>

fix lint

5462144

Signed-off-by: Colvin-Y <[email protected]>

kruise-bot added the needs-rebase label Oct 10, 2025

Colvin-Y marked this pull request as ready for review October 11, 2025 06:54

Copilot AI review requested due to automatic review settings October 11, 2025 06:54

kruise-bot removed the do-not-merge/work-in-progress label Oct 11, 2025

kruise-bot requested a review from zmberg October 11, 2025 06:54

Merge branch 'master' into feature/sidecarset_2123

042ebfc

kruise-bot removed the needs-rebase label Oct 11, 2025

Copilot AI reviewed Oct 11, 2025

View reviewed changes

furykerry reviewed Oct 13, 2025

View reviewed changes

Colvin-Y added 4 commits October 20, 2025 15:58

optimize: free-lock calculator parser, performance upgrade 100%

c2d9b7f

Signed-off-by: Colvin-Y <[email protected]>

add e2e and ut for init-container dynamic resources inject

218f595

Signed-off-by: Colvin-Y <[email protected]>

fix e2e

6b7307a

Signed-off-by: Colvin-Y <[email protected]>

fix ut

1d076b8

Signed-off-by: Colvin-Y <[email protected]>

add helper to draw calculator func

3c8d251

Signed-off-by: Colvin-Y <[email protected]>

kruise-bot added the ok-to-test label Oct 27, 2025

		// For memory, use DecimalSI
		return resource.NewQuantity(int64(result.Number), resource.DecimalSI), nil

-			// For memory, use DecimalSI
-			return resource.NewQuantity(int64(result.Number), resource.DecimalSI), nil
+			// For memory, treat bare number as Mi and use BinarySI
+			bytes := int64(result.Number * 1024 * 1024)
+			return resource.NewQuantity(bytes, resource.BinarySI), nil

-	// empty regex is invalid
-	if len(policy.TargetContainersNameRegex) == 0 {
-		allErrs = append(allErrs, field.Required(fldPath.Child("targetContainersNameRegex"), "invalid empty regex"))
-	} else if _, err := regexp.Compile(policy.TargetContainersNameRegex); err != nil {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("targetContainersNameRegex"), policy.TargetContainersNameRegex,
+	// treat empty regex as default ".*"
+	regexPattern := policy.TargetContainersNameRegex
+	if len(regexPattern) == 0 {
+		regexPattern = ".*"
+	}
+	if _, err := regexp.Compile(regexPattern); err != nil {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("targetContainersNameRegex"), regexPattern,

		allErrs := field.ErrorList{}

		policy := container.ResourcesPolicy

+	// Validate TargetContainerMode
+	if policy.TargetContainerMode != appsv1alpha1.TargetContainerModeSum &&
+		policy.TargetContainerMode != appsv1alpha1.TargetContainerModeMax {
+		allErrs = append(allErrs, field.NotSupported(
+			fldPath.Child("targetContainerMode"),
+			policy.TargetContainerMode,
+			[]string{string(appsv1alpha1.TargetContainerModeSum), string(appsv1alpha1.TargetContainerModeMax)},
+		))
+	}

Feature/sidecarset 2123 #2200

Are you sure you want to change the base?

Feature/sidecarset 2123 #2200

Uh oh!

Conversation

Colvin-Y commented Oct 9, 2025

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

Uh oh!

kruise-bot commented Oct 9, 2025

Uh oh!

codecov bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Colvin-Y Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Colvin-Y commented Oct 20, 2025

Uh oh!

kruise-bot commented Oct 20, 2025

Uh oh!

furykerry commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov bot commented Oct 9, 2025 •

edited

Loading

Colvin-Y Oct 11, 2025 •

edited

Loading