Repository: kubernetes/kubernetes (language: go)

Issue:
Remove Kubelet soft-admission
Kubelet soft admission handlers were created as a way for the Kubelet to block pods that cannot be run for reasons that can be resolved. In practice, this mechanism was only ever used by AppArmor ([here](https://github.com/kubernetes/kubernetes/blob/eef6c6082d4e34fc4a0675a36ec5cc575cd13696/pkg/kubelet/kubelet.go#L912)).

Problems with soft-admission rejection:
1. This is only used by AppArmor, and only for situations that cannot realistically be resolved (host or build does not support apparmor)
2. Pods held in the blocked state must be manually cleaned up. While there they hold reserved capacity from the scheduler and continually retry syncpod.
3. No other features work this way - usually Kubelet hard rejects pods that it knows it cannot run, or the runtime fails to start the pod/container.

This was originally added to prevent controllers from continuously recreating pods that cannot run, but that is a separate problem that should be approached globally, rather than with this 1-off AppArmor solution.

I propose we remove Kubelet soft admission entirely, and move the AppArmor admission handler to the hard-admission list.

/sig node
/milestone v1.31
/kind bug


Discussion / hints:
This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the `triage/accepted` label and provide further guidance.

The `triage/accepted` label can be added by org members by writing `/triage accepted` in a comment.


<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
> No other features work this way - usually Kubelet hard rejects pods that it knows it cannot run, or the runtime fails to start the pod/container.

We have a _different_, alpha mechanism: [PodSchedulingContext](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-scheduling-context-v1alpha2/). We could change AppArmor, seccomp and other mechanisms to work either with PodSchedulingContext or its eventual replacement.

IMO we only need to sketch out an approach and see that it looks viable; we don't have to block graduation for AppArmor support on a fleshed-out design.
From my admittedly limited PoV I'm not against removing soft admission, and I got no opinions about AppArmor specifically. However this seems to connected to the recent conversation we're having in WG serving and SIG-node about retriable vs unretriable errors so I think we would need some coordination
cc @smarterclayton @SergeyKanzhelev 
+1
My understanding is that after this PR: https://github.com/kubernetes/kubernetes/pull/97968/files soft admission just doesn't make sense any longer.
Hello @tallclair ! Just a reminder, the code freeze is starting `02:00 UTC Wednesday 10th July 2024` (about `4 weeks` from now), and while there is still plenty of time, we want to ensure that each PR has a chance to be merged on time.

As the issue is tagged for `1.31`, is it still planned for this release?
Discussed in sig-node, decided to move forward with this (move AppArmor admission to the hard admission set, remove soft-admission code)
/assign
@tallclair - do you want me to incorporate this change into https://github.com/kubernetes/kubernetes/pull/125257?

Fix patch:
diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go
index b3a139c41f8c8..1041302e169e7 100644
--- a/pkg/kubelet/kubelet.go
+++ b/pkg/kubelet/kubelet.go
@@ -910,7 +910,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration,
 	if sysruntime.GOOS == "linux" {
 		// AppArmor is a Linux kernel security module and it does not support other operating systems.
 		klet.appArmorValidator = apparmor.NewValidator()
-		klet.softAdmitHandlers.AddPodAdmitHandler(lifecycle.NewAppArmorAdmitHandler(klet.appArmorValidator))
+		klet.admitHandlers.AddPodAdmitHandler(lifecycle.NewAppArmorAdmitHandler(klet.appArmorValidator))
 	}
 
 	leaseDuration := time.Duration(kubeCfg.NodeLeaseDurationSeconds) * time.Second
@@ -1292,12 +1292,6 @@ type Kubelet struct {
 	// the list of handlers to call during pod admission.
 	admitHandlers lifecycle.PodAdmitHandlers
 
-	// softAdmithandlers are applied to the pod after it is admitted by the Kubelet, but before it is
-	// run. A pod rejected by a softAdmitHandler will be left in a Pending state indefinitely. If a
-	// rejected pod should not be recreated, or the scheduler is not aware of the rejection rule, the
-	// admission rule should be applied by a softAdmitHandler.
-	softAdmitHandlers lifecycle.PodAdmitHandlers
-
 	// the list of handlers to call during pod sync loop.
 	lifecycle.PodSyncLoopHandlers
 
@@ -1795,31 +1789,6 @@ func (kl *Kubelet) SyncPod(ctx context.Context, updateType kubetypes.SyncPodType
 		return isTerminal, nil
 	}
 
-	// If the pod should not be running, we request the pod's containers be stopped. This is not the same
-	// as termination (we want to stop the pod, but potentially restart it later if soft admission allows
-	// it later). Set the status and phase appropriately
-	runnable := kl.canRunPod(pod)
-	if !runnable.Admit {
-		// Pod is not runnable; and update the Pod and Container statuses to why.
-		if apiPodStatus.Phase != v1.PodFailed && apiPodStatus.Phase != v1.PodSucceeded {
-			apiPodStatus.Phase = v1.PodPending
-		}
-		apiPodStatus.Reason = runnable.Reason
-		apiPodStatus.Message = runnable.Message
-		// Waiting containers are not creating.
-		const waitingReason = "Blocked"
-		for _, cs := range apiPodStatus.InitContainerStatuses {
-			if cs.State.Waiting != nil {
-				cs.State.Waiting.Reason = waitingReason
-			}
-		}
-		for _, cs := range apiPodStatus.ContainerStatuses {
-			if cs.State.Waiting != nil {
-				cs.State.Waiting.Reason = waitingReason
-			}
-		}
-	}
-
 	// Record the time it takes for the pod to become running
 	// since kubelet first saw the pod if firstSeenTime is set.
 	existingStatus, ok := kl.statusManager.GetPodStatus(pod.UID)
@@ -1830,25 +1799,6 @@ func (kl *Kubelet) SyncPod(ctx context.Context, updateType kubetypes.SyncPodType
 
 	kl.statusManager.SetPodStatus(pod, apiPodStatus)
 
-	// Pods that are not runnable must be stopped - return a typed error to the pod worker
-	if !runnable.Admit {
-		klog.V(2).InfoS("Pod is not runnable and must have running containers stopped", "pod", klog.KObj(pod), "podUID", pod.UID, "message", runnable.Message)
-		var syncErr error
-		p := kubecontainer.ConvertPodStatusToRunningPod(kl.getRuntime().Type(), podStatus)
-		if err := kl.killPod(ctx, pod, p, nil); err != nil {
-			if !wait.Interrupted(err) {
-				kl.recorder.Eventf(pod, v1.EventTypeWarning, events.FailedToKillPod, "error killing pod: %v", err)
-				syncErr = fmt.Errorf("error killing pod: %w", err)
-				utilruntime.HandleError(syncErr)
-			}
-		} else {
-			// There was no error killing the pod, but the pod cannot be run.
-			// Return an error to signal that the sync loop should back off.
-			syncErr = fmt.Errorf("pod cannot be run: %v", runnable.Message)
-		}
-		return false, syncErr
-	}
-
 	// If the network plugin is not ready, only start the pod if it uses the host network
 	if err := kl.runtimeState.networkErrors(); err != nil && !kubecontainer.IsHostNetworkPod(pod) {
 		kl.recorder.Eventf(pod, v1.EventTypeWarning, events.NetworkNotReady, "%s: %v", NetworkNotReadyErrorMsg, err)
@@ -2332,20 +2282,6 @@ func (kl *Kubelet) canAdmitPod(pods []*v1.Pod, pod *v1.Pod) (bool, string, strin
 	return true, "", ""
 }
 
-func (kl *Kubelet) canRunPod(pod *v1.Pod) lifecycle.PodAdmitResult {
-	attrs := &lifecycle.PodAdmitAttributes{Pod: pod}
-	// Get "OtherPods". Rejected pods are failed, so only include admitted pods that are alive.
-	attrs.OtherPods = kl.GetActivePods()
-
-	for _, handler := range kl.softAdmitHandlers {
-		if result := handler.Admit(attrs); !result.Admit {
-			return result
-		}
-	}
-
-	return lifecycle.PodAdmitResult{Admit: true}
-}
-
 // syncLoop is the main loop for processing changes. It watches for changes from
 // three channels (file, apiserver, and http) and creates a union of them. For
 // any new change seen, will run a sync against desired state and running state. If


Test patch: