Skip to content

backend: Skip DaemonSet pods during drain#5690

Open
harrshita123 wants to merge 1 commit into
kubernetes-sigs:mainfrom
harrshita123:issue-5687-skip-daemonset-pods
Open

backend: Skip DaemonSet pods during drain#5690
harrshita123 wants to merge 1 commit into
kubernetes-sigs:mainfrom
harrshita123:issue-5687-skip-daemonset-pods

Conversation

@harrshita123
Copy link
Copy Markdown
Contributor

Summary

This PR fixes backend node drain behavior so DaemonSet-owned pods are skipped even when they do not have the legacy kubernetes.io/created-by=daemonset-controller label.

Related Issue

Fixes #5687

Changes

  • Added DaemonSet pod detection using pod owner references.
  • Kept the existing legacy label check for compatibility.
  • Updated drain-node tests to verify DaemonSet-owned pods are not deleted.

Steps to Test

  1. Run cd backend && go test ./cmd -run 'TestDrainNodePodDeletionFailure|TestDrainNodeAllPodsDeletedSuccessfully'.
  2. Run cd backend && go test ./cmd.
  3. Run npm run backend:lint.

Screenshots (if applicable)

Not applicable. This is a backend node drain change.

Notes for the Reviewer

The change is limited to the backend drain loop. It does not change how regular pods are deleted during drain; it only avoids deleting pods owned by DaemonSets.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 16, 2026
@illume illume requested a review from Copilot May 16, 2026 14:57
Copy link
Copy Markdown
Contributor

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 updates the backend node drain logic to correctly skip DaemonSet-owned pods by detecting DaemonSet ownership via pod ownerReferences (while retaining the legacy kubernetes.io/created-by=daemonset-controller label check), aligning Headlamp’s drain behavior with modern Kubernetes semantics.

Changes:

  • Added isDaemonSetPod helper to identify DaemonSet pods via legacy label and ownerReferences.kind == "DaemonSet".
  • Updated drainNode to use the helper to skip DaemonSet pods during deletion.
  • Extended drain tests to validate deletion behavior and introduced a helper to extract deleted pod names from fake client actions.

Reviewed changes

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

File Description
backend/cmd/headlamp.go Adds DaemonSet pod detection helper and uses it to skip DaemonSet pods during node drain.
backend/cmd/headlamp_test.go Updates drain tests and adds a helper to assert which pods were deleted.

Comment thread backend/cmd/headlamp.go Outdated
Comment thread backend/cmd/headlamp_test.go
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

There are some open Copilot review comments — could you take a look at them? Please mark each one as resolved once you've addressed it.

@harrshita123 harrshita123 force-pushed the issue-5687-skip-daemonset-pods branch from 32a374f to 8f6fc4d Compare May 17, 2026 08:14
@harrshita123
Copy link
Copy Markdown
Contributor Author

Thanks for the contribution.

There are some open Copilot review comments — could you take a look at them? Please mark each one as resolved once you've addressed it.

@illume
I resolved all the comments of copilot .

Copy link
Copy Markdown
Contributor

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread backend/cmd/headlamp.go Outdated
@harrshita123 harrshita123 force-pushed the issue-5687-skip-daemonset-pods branch from 8f6fc4d to 3f0cec4 Compare May 17, 2026 11:32
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

🎉 thanks!

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2026
Comment thread backend/cmd/headlamp.go
Comment thread backend/cmd/headlamp_test.go
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

Sorry, noticed these two functions are missing some documentation. Could you please add it?

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: harrshita123
Once this PR has been reviewed and has the lgtm label, please assign joaquimrocha for approval. For more information see the 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

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2026
@harrshita123
Copy link
Copy Markdown
Contributor Author

Sorry, noticed these two functions are missing some documentation. Could you please add it?

Yeah sure .

Detect DaemonSet-owned pods by owner reference so node drain does not try to delete pods that should be ignored.
@harrshita123 harrshita123 force-pushed the issue-5687-skip-daemonset-pods branch from 3f0cec4 to d868d57 Compare May 17, 2026 11:59
@harrshita123
Copy link
Copy Markdown
Contributor Author

@illume Added docs for both helper functions and force-pushed the branch as a single commit: backend: Skip DaemonSet pods during drain. I also reran the targeted backend tests:\n\ncd backend && go test ./cmd -run 'TestDrainNodePodDeletionFailure|TestDrainNodeAllPodsDeletedSuccessfully'\n\nResult: passed.

Copy link
Copy Markdown
Contributor

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread backend/cmd/headlamp.go
Comment on lines +2735 to +2742
// It supports both the legacy daemonset-controller label and modern owner references.
func isDaemonSetPod(pod corev1.Pod) bool {
if pod.Labels["kubernetes.io/created-by"] == "daemonset-controller" {
return true
}

for _, owner := range pod.OwnerReferences {
if owner.Kind == "DaemonSet" && (owner.APIVersion == "" || strings.HasPrefix(owner.APIVersion, "apps/")) {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

backend: drainNode does not skip DaemonSet-owned pods

4 participants