Skip to content

Add support for a new Pod Deletion Policy #4162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for a new Pod Deletion Policy #4162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions controller/kubernetes_pod_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ const (
controllerAgentName = "longhorn-kubernetes-pod-controller"

remountRequestDelayDuration = 5 * time.Second

podDeletionAnnotation = "longhorn.io/allow-deletion"
Copy link
Member

@innobead innobead Nov 17, 2025

Choose a reason for hiding this comment

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

Suggested change
podDeletionAnnotation = "longhorn.io/allow-deletion"
podDeletionAnnotation = "longhorn.io/allow-pod-deletion-if-node-down"

)

type KubernetesPodController struct {
Expand Down Expand Up @@ -501,7 +503,7 @@ func (kc *KubernetesPodController) getPodWithConflictedAttachment(pods []*corev1
//
// Force delete a pod when all of the below conditions are meet:
// 1. NodeDownPodDeletionPolicy is different than DoNothing
// 2. pod belongs to a StatefulSet/Deployment depend on NodeDownPodDeletionPolicy
// 2. pod belongs to a StatefulSet/Deployment depend on NodeDownPodDeletionPolicy OR is explicitly marked for deletion.
// 3. node containing the pod is down
// 4. the pod is terminating and the DeletionTimestamp has passed.
// 5. pod has a PV with provisioner driver.longhorn.io
Expand All @@ -513,7 +515,8 @@ func (kc *KubernetesPodController) handlePodDeletionIfNodeDown(pod *corev1.Pod,

shouldDelete := (deletionPolicy == types.NodeDownPodDeletionPolicyDeleteStatefulSetPod && isOwnedByStatefulSet(pod)) ||
(deletionPolicy == types.NodeDownPodDeletionPolicyDeleteDeploymentPod && isOwnedByDeployment(pod)) ||
(deletionPolicy == types.NodeDownPodDeletionPolicyDeleteBothStatefulsetAndDeploymentPod && (isOwnedByStatefulSet(pod) || isOwnedByDeployment(pod)))
(deletionPolicy == types.NodeDownPodDeletionPolicyDeleteBothStatefulsetAndDeploymentPod && (isOwnedByStatefulSet(pod) || isOwnedByDeployment(pod))) ||
(deletionPolicy == types.NodeDownPodDeletionPolicyDeleteBothStatefulsetAndDeploymentPodAndAnnotatedPods && (isOwnedByStatefulSet(pod) || isOwnedByDeployment(pod) || isAnnotatedWithDeletionAnnotation(pod)))

if !shouldDelete {
return nil
Expand Down Expand Up @@ -712,6 +715,15 @@ func isOwnedByDeployment(pod *corev1.Pod) bool {
return false
}

func isAnnotatedWithDeletionAnnotation(pod *corev1.Pod) bool {
if val, ok := pod.ObjectMeta.Annotations[podDeletionAnnotation]; ok {
if strings.ToLower(val) == "true" {
return true
}
}
return false // We did not find the annotation, or the annotation is something else than "true" or "True"
}

// enqueuePodChange determines if the pod requires processing based on whether the pod has a PV created by us (driver.longhorn.io)
func (kc *KubernetesPodController) enqueuePodChange(obj interface{}) {
key, err := controller.KeyFunc(obj)
Expand Down
9 changes: 5 additions & 4 deletions types/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1781,10 +1781,11 @@ var (
type NodeDownPodDeletionPolicy string

const (
NodeDownPodDeletionPolicyDoNothing = NodeDownPodDeletionPolicy("do-nothing") // Kubernetes default behavior
NodeDownPodDeletionPolicyDeleteStatefulSetPod = NodeDownPodDeletionPolicy("delete-statefulset-pod")
NodeDownPodDeletionPolicyDeleteDeploymentPod = NodeDownPodDeletionPolicy("delete-deployment-pod")
NodeDownPodDeletionPolicyDeleteBothStatefulsetAndDeploymentPod = NodeDownPodDeletionPolicy("delete-both-statefulset-and-deployment-pod")
NodeDownPodDeletionPolicyDoNothing = NodeDownPodDeletionPolicy("do-nothing") // Kubernetes default behavior
NodeDownPodDeletionPolicyDeleteStatefulSetPod = NodeDownPodDeletionPolicy("delete-statefulset-pod")
NodeDownPodDeletionPolicyDeleteDeploymentPod = NodeDownPodDeletionPolicy("delete-deployment-pod")
NodeDownPodDeletionPolicyDeleteBothStatefulsetAndDeploymentPod = NodeDownPodDeletionPolicy("delete-both-statefulset-and-deployment-pod")
NodeDownPodDeletionPolicyDeleteBothStatefulsetAndDeploymentPodAndAnnotatedPods = NodeDownPodDeletionPolicy("delete-both-statefulset-and-deployment-pod-and-annotated-pods")
Comment on lines +1787 to +1788
Copy link
Member

@innobead innobead Nov 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Should we retain one? Given that the new policy is essentially a superset of the current one, delete-both-statefulset-and-deployment-pod.

Additionally, we might consider implementing this resilience for all pods managed by controllers, rather than exclusively for SS and DS workloads, as we currently do for the setting auto-delete-pod-when-volume-detached-unexpectedly handling mentioned below. @derekbit @longhorn/maintainer WDYT?

/controller/kubernetes_pod_controller.go#L634-L636

	if ownerRef := metav1.GetControllerOf(pod); ownerRef == nil || kc.isControllerInBlacklist(ownerRef) {
		return nil
	}

Copy link
Member

@derekbit derekbit Nov 17, 2025

Choose a reason for hiding this comment

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

Additionally, we might consider implementing this resilience for all pods managed by controllers, rather than exclusively for SS and DS workloads, as we currently do for the setting auto-delete-pod-when-volume-detached-unexpectedly handling mentioned below. @derekbit @longhorn/maintainer WDYT?

Agree with the proposal!

Copy link
Author

@FireDrunk FireDrunk Nov 19, 2025

Choose a reason for hiding this comment

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

I've checked, and both our CNPG Operator pods and our Strimzi PodSet pods both have that ownerReference correctly set, which makes me believe that using that GetControllerOf function would work properly.

Does that mean it's no longer useful to have the annotation for pods for which the ownerReference is not explicitly set, and only support properly owned pods?

I agree, but it's a bit of a reduction in functionality.

Copy link
Author

@FireDrunk FireDrunk Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Would this code change be OK?

SettingNameEnableDeletionOfControllerOwnedPods = SettingName("enable-deletion-of-controller-owned-pods")
// Check if we are allowed to delete the pod regardless of its type because it's owned by a Controller.
deletePodsOwnedByControllers, err := kc.ds.GetSettingAsBool(types.SettingNameEnableDeletionOfControllerOwnedPods)
if err != nil {
  return err
}

	// Check if the pod has an owning Controller reference
shouldDeleteBecauseOwnedByController := false
if deletePodsOwnedByControllers {
  if ownerRef := metav1.GetControllerOf(pod); ownerRef != nil && !kc.isControllerInBlacklist(ownerRef) {
	shouldDeleteBecauseOwnedByController = true
  }
}

// Check if the pod would be delted by a Policy
shouldDeleteBecauseOfPolicy := (deletionPolicy == types.NodeDownPodDeletionPolicyDeleteStatefulSetPod && isOwnedByStatefulSet(pod)) ||
		(deletionPolicy == types.NodeDownPodDeletionPolicyDeleteDeploymentPod && isOwnedByDeployment(pod)) ||
		(deletionPolicy == types.NodeDownPodDeletionPolicyDeleteBothStatefulsetAndDeploymentPod && (isOwnedByStatefulSet(pod) || isOwnedByDeployment(pod))) ||
		(deletionPolicy == types.NodeDownPodDeletionPolicyDeleteBothStatefulsetAndDeploymentPodAndAnnotatedPods && (isOwnedByStatefulSet(pod) || isOwnedByDeployment(pod) || isAnnotatedWithDeletionAnnotation(pod)))

// If both checks fail to decide to delete the pod, don't do anything.
if !shouldDeleteBecauseOwnedByController && !shouldDeleteBecauseOfPolicy {
  return nil
}

)

type NodeDrainPolicy string
Expand Down
Loading