From 1e8fc543cc7db8433ae0a5e5d48b5198b8cf7b91 Mon Sep 17 00:00:00 2001 From: John-Paul Sassine Date: Sat, 25 Oct 2025 01:14:11 +0000 Subject: [PATCH] Fix report health for templated and renamed DRA claims The Kubelet's DRA manager was failing to report device health status in a pod's status for certain types of resource claims. The logic incorrectly assumed that the claim name used within the container's spec (`container.resources.claims[*].name`) was the same as the metadata name of the actual ResourceClaim object. This assumption is false in two key scenarios: 1. When a claim is generated from a `ResourceClaimTemplate`, Kubernetes creates a `ResourceClaim` object with a randomized suffix in its name. 2. When a user defines a pre-existing claim in `pod.spec.resourceClaims`, they can provide a local name that differs from the actual `ResourceClaim` object's name. In both cases, the code would fail to find the claim's information in its internal cache, resulting in the health status not being populated, as reported in issue #134482. This fix corrects the logic by using `pod.Status.ResourceClaimStatuses` as the authoritative map to look up the actual, generated name of the `ResourceClaim` object. This ensures that both templated and renamed claims are resolved correctly before their health status is retrieved from the cache. Additionally, this change introduces a new node e2e test that specifically covers templated and renamed claims to prevent future regressions. --- pkg/kubelet/cm/dra/manager.go | 136 ++++++++++---------- pkg/kubelet/cm/dra/manager_test.go | 192 +++++++++++++++++++++++------ test/e2e_node/dra_test.go | 9 ++ 3 files changed, 231 insertions(+), 106 deletions(-) diff --git a/pkg/kubelet/cm/dra/manager.go b/pkg/kubelet/cm/dra/manager.go index 99360cdadcb..39e633f92eb 100644 --- a/pkg/kubelet/cm/dra/manager.go +++ b/pkg/kubelet/cm/dra/manager.go @@ -778,116 +778,116 @@ func (m *Manager) GetContainerClaimInfos(pod *v1.Pod, container *v1.Container) ( // UpdateAllocatedResourcesStatus updates the health status of allocated DRA resources in the pod's container statuses. func (m *Manager) UpdateAllocatedResourcesStatus(pod *v1.Pod, status *v1.PodStatus) { logger := klog.FromContext(context.Background()) - for _, container := range pod.Spec.Containers { - // Get all the DRA claim details associated with this specific container. - claimInfos, err := m.GetContainerClaimInfos(pod, &container) - if err != nil { - logger.Error(err, "Failed to get claim infos for container", "pod", klog.KObj(pod), "container", container.Name) + for i := range status.ContainerStatuses { + containerStatus := &status.ContainerStatuses[i] + + // Find the corresponding container spec in the pod spec. + var containerSpec *v1.Container + for j := range pod.Spec.Containers { + if pod.Spec.Containers[j].Name == containerStatus.Name { + containerSpec = &pod.Spec.Containers[j] + break + } + } + + // Skip if there's no matching container spec or it has no resource claims. + if containerSpec == nil || len(containerSpec.Resources.Claims) == 0 { continue } - // Find the corresponding container status - for i, containerStatus := range status.ContainerStatuses { - if containerStatus.Name != container.Name { + resourceStatusMap := make(map[v1.ResourceName]*v1.ResourceStatus) + if containerStatus.AllocatedResourcesStatus != nil { + for j := range containerStatus.AllocatedResourcesStatus { + resourceStatusMap[containerStatus.AllocatedResourcesStatus[j].Name] = &containerStatus.AllocatedResourcesStatus[j] + } + } + + // Iterate through the claims requested by this specific container. + for _, claim := range containerSpec.Resources.Claims { + // Find the actual name of the ResourceClaim object. + var actualClaimName string + for _, podClaimStatus := range pod.Status.ResourceClaimStatuses { + if podClaimStatus.Name == claim.Name { + if podClaimStatus.ResourceClaimName != nil { + actualClaimName = *podClaimStatus.ResourceClaimName + } + break + } + } + + if actualClaimName == "" { + logger.V(4).Info("Could not find generated name for resource claim in pod status", "pod", klog.KObj(pod), "container", containerSpec.Name, "claimName", claim.Name) continue } - // Ensure the slice exists. Use a map for efficient updates by resource name. - resourceStatusMap := make(map[v1.ResourceName]*v1.ResourceStatus) - if status.ContainerStatuses[i].AllocatedResourcesStatus != nil { - for idx := range status.ContainerStatuses[i].AllocatedResourcesStatus { - // Store pointers to modify in place - resourceStatusMap[status.ContainerStatuses[i].AllocatedResourcesStatus[idx].Name] = &status.ContainerStatuses[i].AllocatedResourcesStatus[idx] - } - } else { - status.ContainerStatuses[i].AllocatedResourcesStatus = []v1.ResourceStatus{} - } + func() { + m.cache.RLock() + defer m.cache.RUnlock() - // Loop through each claim associated with the container - for _, claimInfo := range claimInfos { - var resourceName v1.ResourceName - foundClaimInSpec := false - for _, cClaim := range container.Resources.Claims { - if cClaim.Name == claimInfo.ClaimName { - if cClaim.Request == "" { - resourceName = v1.ResourceName(fmt.Sprintf("claim:%s", cClaim.Name)) - } else { - resourceName = v1.ResourceName(fmt.Sprintf("claim:%s/%s", cClaim.Name, cClaim.Request)) - } - foundClaimInSpec = true - break - } - } - if !foundClaimInSpec { - logger.V(4).Info("Could not find matching resource claim in container spec", "pod", klog.KObj(pod), "container", container.Name, "claimName", claimInfo.ClaimName) - continue + // Use the actual claim name to look up claim info. + claimInfo, exists := m.cache.get(actualClaimName, pod.Namespace) + if !exists { + logger.V(4).Info("Could not find claim info for resource claim", "pod", klog.KObj(pod), "container", containerSpec.Name, "claimName", actualClaimName) + return + } + + resourceName := v1.ResourceName(fmt.Sprintf("claim:%s", claim.Name)) + if claim.Request != "" { + resourceName = v1.ResourceName(fmt.Sprintf("claim:%s/%s", claim.Name, claim.Request)) } - // Get or create the ResourceStatus entry for this claim resStatus, ok := resourceStatusMap[resourceName] - if !ok { - // Create a new entry and add it to the map and the slice newStatus := v1.ResourceStatus{ Name: resourceName, Resources: []v1.ResourceHealth{}, } - status.ContainerStatuses[i].AllocatedResourcesStatus = append(status.ContainerStatuses[i].AllocatedResourcesStatus, newStatus) - // Get pointer to the newly added element *after* appending - resStatus = &status.ContainerStatuses[i].AllocatedResourcesStatus[len(status.ContainerStatuses[i].AllocatedResourcesStatus)-1] + // Append and get a pointer to the new element. + if containerStatus.AllocatedResourcesStatus == nil { + containerStatus.AllocatedResourcesStatus = []v1.ResourceStatus{} + } + containerStatus.AllocatedResourcesStatus = append(containerStatus.AllocatedResourcesStatus, newStatus) + resStatus = &containerStatus.AllocatedResourcesStatus[len(containerStatus.AllocatedResourcesStatus)-1] resourceStatusMap[resourceName] = resStatus } - // Clear previous health entries for this resource before adding current ones - // Ensures we only report current health for allocated devices. + // Clear previous health entries before adding current ones. resStatus.Resources = []v1.ResourceHealth{} - // Iterate through the map holding the state specific to each driver for driverName, driverState := range claimInfo.DriverState { - // Iterate through each specific device allocated by this driver for _, device := range driverState.Devices { - healthStr := m.healthInfoCache.getHealthInfo(driverName, device.PoolName, device.DeviceName) - // Convert internal health string to API type var health v1.ResourceHealthStatus switch healthStr { case "Healthy": health = v1.ResourceHealthStatusHealthy case "Unhealthy": health = v1.ResourceHealthStatusUnhealthy - default: // Catches "Unknown" or any other case + default: health = v1.ResourceHealthStatusUnknown } - // Create the ResourceHealth entry - resourceHealth := v1.ResourceHealth{ - Health: health, - } - - // Use first CDI device ID as ResourceID, with fallback + resourceHealth := v1.ResourceHealth{Health: health} if len(device.CDIDeviceIDs) > 0 { resourceHealth.ResourceID = v1.ResourceID(device.CDIDeviceIDs[0]) } else { - // Fallback ID if no CDI ID is present resourceHealth.ResourceID = v1.ResourceID(fmt.Sprintf("%s/%s/%s", driverName, device.PoolName, device.DeviceName)) } - - // Append the health status for this specific device/resource ID resStatus.Resources = append(resStatus.Resources, resourceHealth) } } - } - // Rebuild the slice from the map values to ensure correctness - finalStatuses := make([]v1.ResourceStatus, 0, len(resourceStatusMap)) - for _, rs := range resourceStatusMap { - // Only add if it actually has resource health entries populated - if len(rs.Resources) > 0 { - finalStatuses = append(finalStatuses, *rs) - } - } - status.ContainerStatuses[i].AllocatedResourcesStatus = finalStatuses + }() } + + // Rebuild the slice from map to ensure correctness and remove empty entries. + finalStatuses := make([]v1.ResourceStatus, 0, len(resourceStatusMap)) + for _, rs := range resourceStatusMap { + if len(rs.Resources) > 0 { + finalStatuses = append(finalStatuses, *rs) + } + } + containerStatus.AllocatedResourcesStatus = finalStatuses } } diff --git a/pkg/kubelet/cm/dra/manager_test.go b/pkg/kubelet/cm/dra/manager_test.go index 2f769e46884..761cec11faa 100644 --- a/pkg/kubelet/cm/dra/manager_test.go +++ b/pkg/kubelet/cm/dra/manager_test.go @@ -1538,49 +1538,165 @@ func TestHandleWatchResourcesStream(t *testing.T) { }) } -// TestUpdateAllocatedResourcesStatus checks if the manager correctly updates the -// PodStatus with the health information of allocated DRA resources. It populates -// the caches with known claim and health data, then calls the function and verifies the resulting PodStatus. +// TestUpdateAllocatedResourcesStatus verifies that the manager can correctly +// update the PodStatus with health information for different types of DRA claims. +// It covers the main scenarios that were difficult to test reliably in an e2e +// environment due to timing issues: +// 1. Direct claims: Where the pod's resource claim name directly matches the ResourceClaim object name. +// 2. Renamed claims: Where the pod uses a local name for a ResourceClaim that has a different object name. +// 3. Templated claims: Where the claim is generated from a template, and the pod status contains the +// dynamically generated claim name. func TestUpdateAllocatedResourcesStatus(t *testing.T) { - tCtx := ktesting.Init(t) + directClaimName := "direct-claim-name" + renamedClaimObject := "renamed-claim-object" + templateName := "template-name" + templatedClaimName := "pod-templated-templated-claim" - // Setup Manager with caches - manager, err := NewManager(tCtx.Logger(), nil, t.TempDir()) - require.NoError(t, err) - - // Populate claimInfoCache - claimInfo := genTestClaimInfo(claimUID, []string{podUID}, true) - manager.cache.add(claimInfo) - - // Populate healthInfoCache - healthyDevice := state.DeviceHealth{PoolName: poolName, DeviceName: deviceName, Health: "Healthy", LastUpdated: time.Now()} - _, err = manager.healthInfoCache.updateHealthInfo(driverName, []state.DeviceHealth{healthyDevice}) - require.NoError(t, err) - - // Create Pod and Status objects - pod := genTestPod() - require.NotEmpty(t, pod.Spec.Containers, "genTestPod should create at least one container") - // Ensure the container has a name for matching - pod.Spec.Containers[0].Name = containerName - podStatus := &v1.PodStatus{ - ContainerStatuses: []v1.ContainerStatus{ - {Name: containerName}, + testCases := []struct { + name string + pod *v1.Pod + claimInfos []*ClaimInfo + initialStatus *v1.PodStatus + expectedAllocatedResourcesStatus []v1.ResourceStatus + }{ + { + name: "Direct claim", + pod: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "pod-direct", UID: "pod-direct-uid"}, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + {Name: "container1", Resources: v1.ResourceRequirements{Claims: []v1.ResourceClaim{{Name: "claim1"}}}}, + }, + ResourceClaims: []v1.PodResourceClaim{ + {Name: "claim1", ResourceClaimName: &directClaimName}, + }, + }, + Status: v1.PodStatus{ + ResourceClaimStatuses: []v1.PodResourceClaimStatus{ + {Name: "claim1", ResourceClaimName: &directClaimName}, + }, + }, + }, + claimInfos: []*ClaimInfo{ + { + ClaimInfoState: state.ClaimInfoState{ + ClaimName: directClaimName, + PodUIDs: sets.New("pod-direct-uid"), + DriverState: map[string]state.DriverState{ + "test-driver": {Devices: []state.Device{{PoolName: "pool", DeviceName: "dev-a"}}}, + }, + }, + }, + }, + initialStatus: &v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{{Name: "container1"}}}, + expectedAllocatedResourcesStatus: []v1.ResourceStatus{ + { + Name: "claim:claim1", + Resources: []v1.ResourceHealth{ + {ResourceID: "test-driver/pool/dev-a", Health: v1.ResourceHealthStatusHealthy}, + }, + }, + }, + }, + { + name: "Renamed claim", + pod: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "pod-renamed", UID: "pod-renamed-uid"}, + Spec: v1.PodSpec{ + ResourceClaims: []v1.PodResourceClaim{{Name: "renamed-pod-claim", ResourceClaimName: &renamedClaimObject}}, + Containers: []v1.Container{{Name: "container1", Resources: v1.ResourceRequirements{Claims: []v1.ResourceClaim{{Name: "renamed-pod-claim"}}}}}, + }, + Status: v1.PodStatus{ + ResourceClaimStatuses: []v1.PodResourceClaimStatus{ + {Name: "renamed-pod-claim", ResourceClaimName: &renamedClaimObject}, + }, + }, + }, + claimInfos: []*ClaimInfo{ + { + ClaimInfoState: state.ClaimInfoState{ + ClaimName: renamedClaimObject, + PodUIDs: sets.New("pod-renamed-uid"), + DriverState: map[string]state.DriverState{ + "test-driver": {Devices: []state.Device{{PoolName: "pool", DeviceName: "dev-b"}}}, + }, + }, + }, + }, + initialStatus: &v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{{Name: "container1"}}}, + expectedAllocatedResourcesStatus: []v1.ResourceStatus{ + { + Name: "claim:renamed-pod-claim", + Resources: []v1.ResourceHealth{ + {ResourceID: "test-driver/pool/dev-b", Health: v1.ResourceHealthStatusHealthy}, + }, + }, + }, + }, + { + name: "Templated claim", + pod: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "pod-templated", UID: "pod-templated-uid"}, + Spec: v1.PodSpec{ + ResourceClaims: []v1.PodResourceClaim{{Name: "templated-claim", ResourceClaimTemplateName: &templateName}}, + Containers: []v1.Container{{Name: "container1", Resources: v1.ResourceRequirements{Claims: []v1.ResourceClaim{{Name: "templated-claim"}}}}}, + }, + Status: v1.PodStatus{ + ResourceClaimStatuses: []v1.PodResourceClaimStatus{ + {Name: "templated-claim", ResourceClaimName: &templatedClaimName}, + }, + }, + }, + claimInfos: []*ClaimInfo{ + { + ClaimInfoState: state.ClaimInfoState{ + ClaimName: templatedClaimName, + PodUIDs: sets.New("pod-templated-uid"), + DriverState: map[string]state.DriverState{ + "test-driver": {Devices: []state.Device{{PoolName: "pool", DeviceName: "dev-c"}}}, + }, + }, + }, + }, + initialStatus: &v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{{Name: "container1"}}}, + expectedAllocatedResourcesStatus: []v1.ResourceStatus{ + { + Name: "claim:templated-claim", + Resources: []v1.ResourceHealth{ + {ResourceID: "test-driver/pool/dev-c", Health: v1.ResourceHealthStatusHealthy}, + }, + }, + }, }, } - // Call the function under test - manager.UpdateAllocatedResourcesStatus(pod, podStatus) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tCtx := ktesting.Init(t) + manager, err := NewManager(tCtx.Logger(), nil, t.TempDir()) + require.NoError(t, err) - require.Len(t, podStatus.ContainerStatuses, 1) - contStatus := podStatus.ContainerStatuses[0] - require.NotNil(t, contStatus.AllocatedResourcesStatus) - require.Len(t, contStatus.AllocatedResourcesStatus, 1, "Should have status for one resource claim") + for _, ci := range tc.claimInfos { + manager.cache.add(ci) + } - resourceStatus := contStatus.AllocatedResourcesStatus[0] - assert.Equal(t, v1.ResourceName("claim:"+claimName), resourceStatus.Name, "ResourceStatus Name mismatch") - // Check the Resources slice - require.Len(t, resourceStatus.Resources, 1, "Should have health info for one device") - resourceHealth := resourceStatus.Resources[0] - assert.Equal(t, v1.ResourceID(cdiID), resourceHealth.ResourceID, "ResourceHealth ResourceID mismatch") - assert.Equal(t, v1.ResourceHealthStatusHealthy, resourceHealth.Health, "ResourceHealth Health status mismatch") + // Set all devices to be healthy for the test. + devices := []state.DeviceHealth{} + for _, ci := range tc.claimInfos { + for driverName, ds := range ci.DriverState { + for _, dev := range ds.Devices { + devices = append(devices, state.DeviceHealth{PoolName: dev.PoolName, DeviceName: dev.DeviceName, Health: state.DeviceHealthStatusHealthy}) + } + _, err := manager.healthInfoCache.updateHealthInfo(driverName, devices) + require.NoError(t, err) + } + } + + status := tc.initialStatus.DeepCopy() + manager.UpdateAllocatedResourcesStatus(tc.pod, status) + + require.Len(t, status.ContainerStatuses, 1) + assert.Equal(t, tc.expectedAllocatedResourcesStatus, status.ContainerStatuses[0].AllocatedResourcesStatus) + }) + } } diff --git a/test/e2e_node/dra_test.go b/test/e2e_node/dra_test.go index 183bcf50987..219fec9ffdb 100644 --- a/test/e2e_node/dra_test.go +++ b/test/e2e_node/dra_test.go @@ -1584,6 +1584,15 @@ func createHealthTestPodAndClaim(ctx context.Context, f *framework.Framework, dr e2epod.DeletePodOrFail(ctx, f.ClientSet, createdPod.Namespace, createdPod.Name) }) + // Update the Pod's status to include the ResourceClaimStatuses, mimicking the scheduler. + podToUpdate, err := f.ClientSet.CoreV1().Pods(f.Namespace.Name).Get(ctx, createdPod.Name, metav1.GetOptions{}) + framework.ExpectNoError(err) + podToUpdate.Status.ResourceClaimStatuses = []v1.PodResourceClaimStatus{ + {Name: claimName, ResourceClaimName: &claimName}, + } + _, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).UpdateStatus(ctx, podToUpdate, metav1.UpdateOptions{}) + framework.ExpectNoError(err, "failed to update Pod status with ResourceClaimStatuses") + ginkgo.By(fmt.Sprintf("Allocating claim %q to pod %q with its real UID", claimName, podName)) // Get the created claim to ensure the latest version before updating. claimToUpdate, err := f.ClientSet.ResourceV1().ResourceClaims(f.Namespace.Name).Get(ctx, claimName, metav1.GetOptions{})