fix: GPUReset job creation failures and invalid partial drains

janitor/pkg/controller/gpureset_controller.go

@@ -876,7 +876,7 @@ func (r *GPUResetReconciler) expectedJobName(gr *v1alpha1.GPUReset) string {
 	}
 
 	if len(baseName) > maxJobNameBaseLength {
-		baseName = baseName[0:maxJobNameBaseLength]
+		baseName = strings.TrimRight(baseName[0:maxJobNameBaseLength], ".")
 	}
 
 	hash := sha256.Sum256([]byte(gr.Name))

janitor/pkg/controller/gpureset_controller_test.go

@@ -17,6 +17,7 @@ package controller
 import (
 	"context"
 	"strings"
+	"testing"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -31,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation"
 	ctrl "sigs.k8s.io/controller-runtime"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -1606,3 +1608,45 @@ var _ = Describe("GPUReset Controller", func() {
 		})
 	})
 })
+
+func TestExpectedJobName(t *testing.T) {
+	r := &GPUResetReconciler{}
+
+	cases := []struct {
+		name   string
+		crName string
+	}{
+		{
+			name:   "no truncation",
+			crName: "maintenance-ip-10-1-1-1.us-east-2.compute.internal-abc12345",
+		},
+		{
+			name:   "truncation lands on dot character",
+			crName: "maintenance-ip-10-1-115-210.us-east-2.compute.internal-6fca3f52-abcd1234",
+		},
+		{
+			name:   "truncation lands on alphanumeric character",
+			crName: "maintenance-ip-10-1-115-21.us-east-2.compute.internal-6fca3f52-abcd1234",
+		},
+		{
+			name:   "truncation lands on dash character",
+			crName: "maintenance-abcd-cloud-az61-gpu-worke-abcdef01-6fca3f52-abcd1234",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			gr := &v1alpha1.GPUReset{
+				ObjectMeta: metav1.ObjectMeta{Name: tc.crName},
+			}
+			jobName := r.expectedJobName(gr)
+
+			if errs := validation.IsDNS1123Subdomain(jobName); len(errs) != 0 {
+				t.Errorf("job name %q (from CR %q) is not a valid DNS subdomain: %v", jobName, tc.crName, errs)
+			}
+			if len(jobName) > validation.DNS1123LabelMaxLength {
+				t.Errorf("job name %q exceeds max length %d", jobName, validation.DNS1123LabelMaxLength)
+			}
+		})
+	}
+}

node-drainer/pkg/informers/informers.go

@@ -189,7 +189,7 @@ func (i *Informers) FindEvictablePodsInNamespaceAndNode(namespace, nodeName stri
 		}
 	}
 
-	pods = i.filterEvictablePods(pods)
+	pods = i.filterEvictablePods(pods, partialDrainEntity)
 
 	if i.drainGPUPods && partialDrainEntity == nil {
 		pods = i.filterPodsWithGPURequests(pods)
@@ -329,7 +329,7 @@ func areContainersRequestingDevice(containers []v1.Container, resourceNames []st
 	return false
 }
 
-func (i *Informers) filterEvictablePods(pods []*v1.Pod) []*v1.Pod {
+func (i *Informers) filterEvictablePods(pods []*v1.Pod, partialDrainEntity *protos.Entity) []*v1.Pod {
 	filteredPods := []*v1.Pod{}
 
 	for _, pod := range pods {
@@ -344,7 +344,10 @@ func (i *Informers) filterEvictablePods(pods []*v1.Pod) []*v1.Pod {
 			continue
 		}
 
-		if i.isPodStuckInTerminating(pod) || i.isPodNotReady(pod) {
+		// Only skip draining for pods stuck terminating or not ready if we are executing a full drain.
+		// There is no guarantee that a pod in either state does not still have a GPU device handle that
+		// would cause a GPU reset to fail.
+		if partialDrainEntity == nil && (i.isPodStuckInTerminating(pod) || i.isPodNotReady(pod)) {
 			slog.Info("Ignoring pod in namespace on node",
 				"pod", pod.Name,
 				"namespace", pod.Namespace,

node-drainer/pkg/reconciler/reconciler_integration_test.go

@@ -568,6 +568,56 @@ func TestReconciler_ProcessEvent(t *testing.T) {
 				}
 			},
 		},
+		{
+			name:            "Partial drain for ImmediateEviction mode only evicts NotReady but not Failed pods leveraging the given GPU UUID",
+			nodeName:        "test-node",
+			namespaces:      []string{"immediate-test"},
+			nodeQuarantined: model.Quarantined,
+			pods: []*v1.Pod{
+				{
+					ObjectMeta: metav1.ObjectMeta{Name: "pod-1", Namespace: "immediate-test", Annotations: map[string]string{
+						model.PodDeviceAnnotationName: "{\"devices\":{\"nvidia.com/gpu\":[\"GPU-123\"]}}",
+					}},
+					Spec:   v1.PodSpec{NodeName: "test-node", Containers: []v1.Container{{Name: "c", Image: "nginx"}}},
+					Status: v1.PodStatus{Phase: v1.PodFailed},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{Name: "pod-2", Namespace: "immediate-test", Annotations: map[string]string{
+						model.PodDeviceAnnotationName: "{\"devices\":{\"nvidia.com/gpu\":[\"GPU-123\"]}}",
+					}},
+					Spec:   v1.PodSpec{NodeName: "test-node", Containers: []v1.Container{{Name: "c", Image: "nginx"}}},
+					Status: v1.PodStatus{Phase: v1.PodRunning, Conditions: []v1.PodCondition{{Type: v1.PodReady, Status: v1.ConditionFalse}}},
+				},
+			},
+			entitiesImpacted: []*protos.Entity{
+				{
+					EntityType:  "GPU_UUID",
+					EntityValue: "GPU-123",
+				},
+				{
+					EntityType:  "PCI",
+					EntityValue: "PCI:0000:00:08",
+				},
+			},
+			recommendedAction: protos.RecommendedAction_COMPONENT_RESET,
+			expectError:       true,
+			expectedNodeLabel: ptr.To(string(statemanager.DrainingLabelValue)),
+			validateFunc: func(t *testing.T, client kubernetes.Interface, ctx context.Context, nodeName string, err error) {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), "immediate eviction completed, requeuing for status verification")
+
+				// confirm that pod-2 was deleted but not pod-1
+				expectDeletedForPods := map[string]bool{
+					"pod-1": false,
+					"pod-2": true,
+				}
+				for podName, expectDeleted := range expectDeletedForPods {
+					pod, err := client.CoreV1().Pods("immediate-test").Get(ctx, podName, metav1.GetOptions{})
+					require.NoError(t, err)
+					assert.True(t, pod.DeletionTimestamp != nil == expectDeleted)
+				}
+			},
+		},
 		{
 			name:            "Partial drain for DeleteAfterTimeout mode only evicts for pods leveraging given GPU UUID",
 			nodeName:        "test-node",
@@ -896,7 +946,7 @@ func TestReconciler_ProcessEvent(t *testing.T) {
 			}
 
 			for _, pod := range tt.pods {
-				createPod(setup.ctx, t, setup.client, pod.Namespace, pod.Name, tt.nodeName, pod.Status.Phase, pod.Annotations, pod.Spec.Containers[0].Resources.Limits)
+				createPod(setup.ctx, t, setup.client, pod.Namespace, pod.Name, tt.nodeName, pod.Status.Phase, pod.Annotations, pod.Spec.Containers[0].Resources.Limits, pod.Status.Conditions)
 			}
 
 			if tt.findHealthEventsByQueryResponse != nil {
@@ -964,7 +1014,7 @@ func TestReconciler_DryRunMode(t *testing.T) {
 	nodeName := "dry-run-node"
 	createNode(setup.ctx, t, setup.client, nodeName)
 	createNamespace(setup.ctx, t, setup.client, "immediate-test")
-	createPod(setup.ctx, t, setup.client, "immediate-test", "dry-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "immediate-test", "dry-pod", nodeName, v1.PodRunning, nil, nil, nil)
 
 	err := processHealthEvent(setup.ctx, t, setup.reconciler, setup.mockCollection, setup.healthEventStore,
 		healthEventOptions{
@@ -999,7 +1049,7 @@ func TestReconciler_RequeueMechanism(t *testing.T) {
 
 	initialDepth := getGaugeValue(t, metrics.QueueDepth)
 
-	createPod(setup.ctx, t, setup.client, "immediate-test", "test-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "immediate-test", "test-pod", nodeName, v1.PodRunning, nil, nil, nil)
 	enqueueHealthEvent(setup.ctx, t, setup.queueMgr, setup.mockCollection, setup.healthEventStore, nodeName)
 
 	require.Eventually(t, func() bool {
@@ -1030,7 +1080,7 @@ func TestReconciler_AllowCompletionRequeue(t *testing.T) {
 	_, err := setup.client.CoreV1().Namespaces().Create(setup.ctx, ns, metav1.CreateOptions{})
 	require.NoError(t, err)
 
-	createPod(setup.ctx, t, setup.client, "completion-test", "running-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "completion-test", "running-pod", nodeName, v1.PodRunning, nil, nil, nil)
 	enqueueHealthEvent(setup.ctx, t, setup.queueMgr, setup.mockCollection, setup.healthEventStore, nodeName)
 
 	assertNodeLabel(t, setup.client, setup.ctx, nodeName, statemanager.DrainingLabelValue)
@@ -1081,7 +1131,7 @@ func TestReconciler_MultipleNodesRequeue(t *testing.T) {
 	beforeReceived := getCounterValue(t, metrics.TotalEventsReceived)
 
 	for _, nodeName := range nodeNames {
-		createPod(setup.ctx, t, setup.client, "immediate-test", fmt.Sprintf("pod-%s", nodeName), nodeName, v1.PodRunning, nil, nil)
+		createPod(setup.ctx, t, setup.client, "immediate-test", fmt.Sprintf("pod-%s", nodeName), nodeName, v1.PodRunning, nil, nil, nil)
 		enqueueHealthEvent(setup.ctx, t, setup.queueMgr, setup.mockCollection, setup.healthEventStore, nodeName)
 	}
 
@@ -1109,8 +1159,8 @@ func TestReconciler_DeleteAfterTimeoutThenAllowCompletion(t *testing.T) {
 	createNamespace(setup.ctx, t, setup.client, "timeout-test")
 	createNamespace(setup.ctx, t, setup.client, "completion-test")
 
-	createPod(setup.ctx, t, setup.client, "timeout-test", "timeout-pod", nodeName, v1.PodRunning, nil, nil)
-	createPod(setup.ctx, t, setup.client, "completion-test", "completion-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "timeout-test", "timeout-pod", nodeName, v1.PodRunning, nil, nil, nil)
+	createPod(setup.ctx, t, setup.client, "completion-test", "completion-pod", nodeName, v1.PodRunning, nil, nil, nil)
 
 	enqueueHealthEvent(setup.ctx, t, setup.queueMgr, setup.mockCollection, setup.healthEventStore, nodeName)
 
@@ -1534,13 +1584,13 @@ func createNamespace(ctx context.Context, t *testing.T, client kubernetes.Interf
 }
 
 func createPod(ctx context.Context, t *testing.T, client kubernetes.Interface, namespace, name, nodeName string, phase v1.PodPhase,
-	annotations map[string]string, limits v1.ResourceList) {
+	annotations map[string]string, limits v1.ResourceList, conditions []v1.PodCondition) {
 	t.Helper()
 	pod := &v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace, Annotations: annotations},
 		Spec: v1.PodSpec{NodeName: nodeName, Containers: []v1.Container{{Name: "c", Image: "nginx",
 			Resources: v1.ResourceRequirements{Limits: limits}}}},
-		Status: v1.PodStatus{Phase: phase},
+		Status: v1.PodStatus{Phase: phase, Conditions: conditions},
 	}
 	po, err := client.CoreV1().Pods(namespace).Create(ctx, pod, metav1.CreateOptions{})
 	require.NoError(t, err)
@@ -1685,7 +1735,7 @@ func TestMetrics_NodeDrainTimeout(t *testing.T) {
 	nodeName := "metrics-timeout-node"
 	createNode(setup.ctx, t, setup.client, nodeName)
 	createNamespace(setup.ctx, t, setup.client, "timeout-test")
-	createPod(setup.ctx, t, setup.client, "timeout-test", "timeout-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "timeout-test", "timeout-pod", nodeName, v1.PodRunning, nil, nil, nil)
 
 	_ = processHealthEvent(setup.ctx, t, setup.reconciler, setup.mockCollection, setup.healthEventStore, healthEventOptions{
 		nodeName:        nodeName,
@@ -1808,7 +1858,7 @@ func TestReconciler_CancelledEventWithOngoingDrain(t *testing.T) {
 
 	createNamespace(setup.ctx, t, setup.client, "timeout-test")
 
-	createPod(setup.ctx, t, setup.client, "timeout-test", "stuck-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "timeout-test", "stuck-pod", nodeName, v1.PodRunning, nil, nil, nil)
 
 	beforeCancelled := getCounterVecValue(t, metrics.CancelledEvent, nodeName, "test-check")
 
@@ -1858,7 +1908,7 @@ func TestReconciler_UnQuarantinedEventCancelsOngoingDrain(t *testing.T) {
 
 	createNamespace(setup.ctx, t, setup.client, "timeout-test")
 
-	createPod(setup.ctx, t, setup.client, "timeout-test", "stuck-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "timeout-test", "stuck-pod", nodeName, v1.PodRunning, nil, nil, nil)
 
 	t.Log("Enqueue Quarantined event - should start deleteAfterTimeout drain")
 	quarantinedEvent := createHealthEvent(healthEventOptions{
@@ -1915,8 +1965,8 @@ func TestReconciler_MultipleEventsOnNodeCancelledByUnQuarantine(t *testing.T) {
 
 	createNamespace(setup.ctx, t, setup.client, "timeout-test")
 
-	createPod(setup.ctx, t, setup.client, "timeout-test", "pod-1", nodeName, v1.PodRunning, nil, nil)
-	createPod(setup.ctx, t, setup.client, "timeout-test", "pod-2", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "timeout-test", "pod-1", nodeName, v1.PodRunning, nil, nil, nil)
+	createPod(setup.ctx, t, setup.client, "timeout-test", "pod-2", nodeName, v1.PodRunning, nil, nil, nil)
 
 	t.Log("Enqueue two Quarantined events for the same node")
 	event1 := createHealthEvent(healthEventOptions{
@@ -1981,7 +2031,7 @@ func TestReconciler_UnQuarantineCancellationDoesNotCancelFreshSession(t *testing
 	nodeName := testutils.GenerateTestNodeName("fresh-session-after-unquarantine")
 	createNode(setup.ctx, t, setup.client, nodeName)
 	createNamespace(setup.ctx, t, setup.client, "allowcompletion-test")
-	createPod(setup.ctx, t, setup.client, "allowcompletion-test", "held-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "allowcompletion-test", "held-pod", nodeName, v1.PodRunning, nil, nil, nil)
 
 	baseTime := time.Now().UTC()
 	oldEvent := createHealthEvent(healthEventOptions{
@@ -2043,7 +2093,7 @@ func TestReconciler_UnQuarantineCutoffStillCancelsUntrackedOldEventsAfterFreshSe
 	nodeName := testutils.GenerateTestNodeName("fresh-session-before-untracked-old")
 	createNode(setup.ctx, t, setup.client, nodeName)
 	createNamespace(setup.ctx, t, setup.client, "allowcompletion-test")
-	createPod(setup.ctx, t, setup.client, "allowcompletion-test", "held-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "allowcompletion-test", "held-pod", nodeName, v1.PodRunning, nil, nil, nil)
 
 	baseTime := time.Now().UTC()
 	staleQueuedEvent := createHealthEvent(healthEventOptions{
@@ -2101,7 +2151,7 @@ func TestReconciler_UnQuarantineCutoffDoesNotCancelFreshTimeoutEviction(t *testi
 	nodeName := testutils.GenerateTestNodeName("fresh-timeout-after-unquarantine")
 	createNode(setup.ctx, t, setup.client, nodeName)
 	createNamespace(setup.ctx, t, setup.client, "timeout-test")
-	createPod(setup.ctx, t, setup.client, "timeout-test", "held-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "timeout-test", "held-pod", nodeName, v1.PodRunning, nil, nil, nil)
 
 	baseTime := time.Now().UTC()
 	freshEvent := createHealthEvent(healthEventOptions{
@@ -2169,8 +2219,8 @@ func TestReconciler_CustomDrainHappyPath(t *testing.T) {
 	createNamespace(setup.ctx, t, setup.client, "default")
 	createNamespace(setup.ctx, t, setup.client, "app-ns")
 
-	createPod(setup.ctx, t, setup.client, "default", "pod-1", nodeName, v1.PodRunning, nil, nil)
-	createPod(setup.ctx, t, setup.client, "app-ns", "app-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "default", "pod-1", nodeName, v1.PodRunning, nil, nil, nil)
+	createPod(setup.ctx, t, setup.client, "app-ns", "app-pod", nodeName, v1.PodRunning, nil, nil, nil)
 
 	gvr := schema.GroupVersionResource{
 		Group:    "drain.example.com",
@@ -2423,7 +2473,7 @@ func TestMetrics_PodEvictionDuration(t *testing.T) {
 
 	createNodeWithLabelsAndAnnotations(setup.ctx, t, setup.client, nodeName, nodeLabels, nil)
 	createNamespace(setup.ctx, t, setup.client, "immediate-test")
-	createPod(setup.ctx, t, setup.client, "immediate-test", "test-pod", nodeName, v1.PodRunning, nil, nil)
+	createPod(setup.ctx, t, setup.client, "immediate-test", "test-pod", nodeName, v1.PodRunning, nil, nil, nil)
 
 	beforeEvictionDuration := getHistogramCount(t, metrics.PodEvictionDuration)