Skip to content

Comments

Add support for DaemonSet to AdvancedDaemonSet migration #131

Closed
GautamBytes wants to merge 1 commit intoopenkruise:masterfrom
GautamBytes:migrate-daemonset-to-ads
Closed

Add support for DaemonSet to AdvancedDaemonSet migration #131
GautamBytes wants to merge 1 commit intoopenkruise:masterfrom
GautamBytes:migrate-daemonset-to-ads

Conversation

@GautamBytes
Copy link
Contributor

Ⅰ. Describe what this PR does

This PR introduces the functionality to migrate a native Kubernetes DaemonSet to a Kruise AdvancedDaemonSet via the kubectl-kruise migrate command.

The implementation includes:

  1. New Migration Path: The migrate command now accepts DaemonSet as a source (--from) and AdvancedDaemonSet as a destination.
  2. Creation Logic: A --create flag is supported to generate a new AdvancedDaemonSet from an existing DaemonSet's specification without migrating the pods.
  3. Migration Logic: The full migration works by creating the new AdvancedDaemonSet and then performing an orphan deletion of the original DaemonSet. This leaves the original pods running so they can be adopted by the new AdvancedDaemonSet controller, ensuring zero downtime.
  4. Updated Docs: The CLI command's help text and examples have been updated to reflect the new functionality.

Example Usage:

# Create an empty AdvancedDaemonSet from an existing DaemonSet.
kubectl-kruise migrate AdvancedDaemonSet --from DaemonSet -n default --dst-name my-ads --create

# Migrate pods from an existing DaemonSet to an AdvancedDaemonSet.
kubectl-kruise migrate AdvancedDaemonSet --from DaemonSet -n default --src-name my-ds --dst-name my-ads

Ⅱ. Does this pull request fix one issue?

fixes #129

Ⅲ. Special notes for reviews

This implementation is modeled after the existing Deployment to CloneSet migration to maintain consistency within the project.

The core logic for the migration path relies on DeletePropagationOrphan to ensure a safe handover of pods from the old DaemonSet to the new AdvancedDaemonSet. The logic has been cleanly separated into pkg/creation/daemonset and pkg/migration/daemonset packages for maintainability.

@kruise-bot kruise-bot requested review from furykerry and hantmac June 30, 2025 15:33
@kruise-bot
Copy link

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

@GautamBytes
Copy link
Contributor Author

@furykerry can you review it whenever feasible and suggest improvements wherever needed. Thanks!

@hantmac hantmac requested a review from Copilot July 2, 2025 09:29
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for migrating a native Kubernetes DaemonSet to a Kruise AdvancedDaemonSet, including both a creation-only path and a full migration path with orphan-deletion handoff.

  • Introduces pkg/creation/daemonset and pkg/migration/daemonset controls for DaemonSet→AdvancedDaemonSet
  • Extends kubectl-kruise migrate to recognize DaemonSet/AdvancedDaemonSet in CLI and API refs
  • Updates documentation and examples to demonstrate new flags (--create, --copy, --max-surge)

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/migration/daemonset/daemonset_migration.go Implements Submit and Query for full DaemonSet migration
pkg/creation/daemonset/daemonset_creation.go Implements Create control for AdvancedDaemonSet from DaemonSet
pkg/cmd/migrate/migrate_daemonset.go Adds migrateDaemonSet handler for CLI
pkg/cmd/migrate/migrate.go Extends CLI parsing to support DaemonSet/AdvancedDaemonSet
pkg/api/api.go Defines new ResourceRef constructors for both DaemonSet kinds
docs/kubectl-kruise_migrate.md Updates migrate command examples for DaemonSet paths
Comments suppressed due to low confidence (3)

pkg/api/api.go:107

  • [nitpick] Using the CRD Kind DaemonSet for AdvancedDaemonSetRef may confuse tooling and users expecting AdvancedDaemonSet as the Kind. Consider aligning the Kind field with the CLI naming or documenting this discrepancy.
		Kind:       AdvancedDaemonSetKind.Kind,

docs/kubectl-kruise_migrate.md:26

  • The indentation of this example line is inconsistent with other examples in the code block. Align the leading spaces to match the surrounding formatting for readability.
    # Create an empty AdvancedDaemonSet from an existing DaemonSet.

pkg/migration/daemonset/daemonset_migration.go:1

  • There are no unit tests covering the new DaemonSet→AdvancedDaemonSet migration logic. Consider adding tests for both successful and failure scenarios to ensure correctness and prevent regressions.
package daemonset

@GautamBytes GautamBytes requested a review from hantmac July 2, 2025 12:13
@GautamBytes
Copy link
Contributor Author

@hantmac can you review it again ?

@furykerry
Copy link
Member

@GautamBytes do you have environment to the test migration operation? it seems that these two commands will not works in real enviroment

kubectl-kruise migrate CloneSet --from Deployment -n default --src-name cloneset-name --dst-name deployment-name --replicas 10 --max-surge=2

# Create an empty AdvancedDaemonSet from an existing DaemonSet.
kubectl-kruise migrate AdvancedDaemonSet --from DaemonSet -n default --dst-name ads-name --create
Copy link
Member

Choose a reason for hiding this comment

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

since daemonset does not have replica field, it is not possible to create "empty" daemon set , unless you hack the affinity of daemonset pod e.g. adding an non-existing affinity label or adding a pod anti-affinity rule. Alternatively, we can not support create empty advance daemonset at all

},
},
}
if err := c.client.Create(context.Background(), ads); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

we should orphan delete the native daemonset first, otherwise advance daemonset will create another set of daemon pods before adopting the pods of original daemonset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure , will make changes accordingly. Thanks!

@kruise-bot kruise-bot added size/XL and removed size/L labels Aug 20, 2025
@GautamBytes GautamBytes force-pushed the migrate-daemonset-to-ads branch from ec1c00d to 9b30ef4 Compare August 20, 2025 06:38
@kruise-bot kruise-bot added size/L and removed size/XL labels Aug 20, 2025
Signed-off-by: GautamBytes <manchandanigautam@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add kubectl kruise migrate support for advance daemonset

4 participants