Skip to content

feat: Native DaemonSet Progressive Delivery - Part 1: Core Infrastruc…#314

Merged
kruise-bot merged 6 commits intoopenkruise:masterfrom
marcoma2018:ds/one
Oct 30, 2025
Merged

feat: Native DaemonSet Progressive Delivery - Part 1: Core Infrastruc…#314
kruise-bot merged 6 commits intoopenkruise:masterfrom
marcoma2018:ds/one

Conversation

@marcoma2018
Copy link
Copy Markdown
Contributor

@marcoma2018 marcoma2018 commented Oct 15, 2025

Summary

This PR implements the first part of Native DaemonSet Progressive Delivery support. This foundational implementation enables progressive delivery for standard Kubernetes DaemonSets (apps/v1) within the Kruise Rollout framework, which focuses on the core controller logic, ControllerRevision discovery, and BatchRelease integration as defined in the proposal.

Implementation Details

1. Core Controller Infrastructure

  • New partition-style controller in pkg/controller/batchrelease/control/partitionstyle/nativedaemonset/
    • Implements Initialize, UpgradeBatch, Finalize, CalculateBatchContext interface methods
    • Manages OnDelete update strategy for controlled rollouts
    • Handles batch upgrade logic with proper partition tracking

2. Workload Discovery and Parsing

  • Enhanced ControllerFinder in pkg/util/controller_finder.go
    • Added native DaemonSet discovery and revision management
    • Automatic identification of stable and canary revisions from ControllerRevision objects
    • Annotation-based persistence of revision information
  • Workload parsing utilities in pkg/util/parse_utils.go
    • Added ParseDaemonSet function for native DaemonSet workload parsing
    • Support for extracting revision information and status from DaemonSet objects
  • Workload utilities in pkg/util/workloads_utils.go
    • Added DaemonSet-specific helper functions for pod counting and revision matching
    • Enhanced workload type detection and handling

3. BatchRelease Integration

  • Added native DaemonSet adaptation to existing BatchRelease workflow
    • Updated batchrelease_controller.go, batchrelease_event_handler.go, andbatchrelease_executor.go to recognize and process native DaemonSet workloads
    • Integrated DaemonSet support into the main reconciliation loop and execution logic

4. Rollout Controller Integration

  • Added native DaemonSet adaptation to existing Rollout framework
    • Enhanced rollout_controller.go and rollout_status.go handling to support native DaemonSet workload references
    • Integrated DaemonSet-specific status calculation and reporting into the rollout workflow

5. Webhook Mutation Support

  • Enhanced mutation webhook in pkg/webhook/workload/mutating/workload_update_handler.go
    • Added handleNativeDaemonSet function for DaemonSet template change detection and integration with rollout progression workflow
    • Automatic rollout annotation injection and OnDelete strategy enforcement
  • Updated webhook registration in pkg/webhook/workload/mutating/webhooks.go
    • Added native DaemonSet to supported workload types

6. Configuration and Permissions

  • Updated RBAC in config/rbac/role.yaml
    • Added permissions for DaemonSet and ControllerRevision resources
  • Enhanced webhook configuration in config/webhook/manifests.yaml
    • Updated webhook rules to include DaemonSet resources
  • Added constants in pkg/util/constant.go
    • DaemonSet-specific annotation keys and configuration constants

What's Next

This is Part 1 of 3 in the Native DaemonSet Progressive Delivery implementation:

  • Part 2: Advanced DaemonSet Controller (separate controller for pod deletion logic based on partition)
  • Part 3: Integration tests and end-to-end validation

…ture

Signed-off-by: Marco Ma <marco.ma@sap.com>
Signed-off-by: Marco Ma <marco.ma@sap.com>
Signed-off-by: Marco Ma <marco.ma@sap.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 76.97368% with 70 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.60%. Comparing base (787bb9c) to head (838b6b6).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...bhook/workload/mutating/workload_update_handler.go 39.02% 23 Missing and 2 partials ⚠️
pkg/util/controller_finder.go 74.35% 10 Missing and 10 partials ⚠️
.../control/partitionstyle/nativedaemonset/control.go 93.70% 5 Missing and 4 partials ⚠️
...troller/batchrelease/batchrelease_event_handler.go 0.00% 4 Missing ⚠️
pkg/webhook/workload/mutating/webhooks.go 0.00% 4 Missing ⚠️
...g/controller/batchrelease/batchrelease_executor.go 50.00% 2 Missing and 1 partial ⚠️
pkg/util/rollout_utils.go 0.00% 3 Missing ⚠️
pkg/controller/rollout/rollout_status.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
+ Coverage   47.99%   50.60%   +2.61%     
==========================================
  Files          63       64       +1     
  Lines        9841     8201    -1640     
==========================================
- Hits         4723     4150     -573     
+ Misses       4559     3478    -1081     
- Partials      559      573      +14     
Flag Coverage Δ
unittests 50.60% <76.97%> (+2.61%) ⬆️

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: Marco Ma <qingjin_ma@163.com>
revision := &revisionList.Items[i]
// Check if this ControllerRevision is owned by the DaemonSet
if ref := metav1.GetControllerOf(revision); ref != nil {
if ref.Name == daemonSet.Name && ref.Kind == "DaemonSet" {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also check the apiversion field of the reference. Alternatively, just check the uid of the ref ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! You're right that we should validate the owner reference more strictly.
I'll update the code to check the UID directly, which is more reliable than checking
Name/Kind/APIVersion separately.

}

func (r *ControllerFinder) getStatefulSetLikeWorkload(namespace string, ref *rolloutv1beta1.ObjectRef) (*Workload, error) {
ok, _ := verifyGroupKind(ref, ControllerKindSts.Kind, []string{ControllerKindSts.Group})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why add these logic ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During testing, I discovered that getNativeDaemonSet was never being reached. After adding more logs, I found the reason is that because getStatefulSetLikeWorkload (earlier in the finder chain: return []ControllerFinderFunc{r.getKruiseCloneSet, r.getAdvancedDeployment, r.getStatefulSetLikeWorkload, r.getKruiseDaemonSet, r.getNativeDaemonSet}) was incorrectly entered due to missing validation. Adding these checks ensures each finder only matches its intended workload type, allowing all finders in the chain to function properly.

if newObj.Annotations == nil {
newObj.Annotations = map[string]string{}
}
newObj.Annotations[util.InRolloutProgressingAnnotation] = string(by)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we keep the original update strategy of daemonset in annotation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I initially thought DaemonSets wouldn't use OnDelete strategy in real scenarios, but that assumption was too optimistic. I'll add the logic to store the strategy type in the annotation to ensure we can properly restore it later.

// if batchPartition == nil, workload should be promoted to use the original update strategy
if release.Spec.ReleasePlan.BatchPartition == nil {
updateStrategy := apps.DaemonSetUpdateStrategy{
Type: apps.RollingUpdateDaemonSetStrategyType,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the original update strategy is already OnDelete?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. The logic will be changed to restore update strategy by annotation.

@zmberg
Copy link
Copy Markdown
Member

zmberg commented Oct 29, 2025

/lgtm

@marcoma2018
Copy link
Copy Markdown
Contributor Author

marcoma2018 commented Oct 29, 2025

@furykerry I'll address these two improvements: UID for owner reference and update strategy on annotation in my next PR.

Signed-off-by: Marco Ma <qingjin_ma@163.com>
@kruise-bot kruise-bot removed the lgtm label Oct 29, 2025
Signed-off-by: Marco Ma <qingjin_ma@163.com>
Copy link
Copy Markdown
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@kruise-bot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furykerry

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:

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

@kruise-bot kruise-bot merged commit 501d832 into openkruise:master Oct 30, 2025
34 checks passed
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.

5 participants