diff --git a/cluster-autoscaler/simulator/clustersnapshot/predicate/predicate_snapshot.go b/cluster-autoscaler/simulator/clustersnapshot/predicate/predicate_snapshot.go index dbfe2305422728f750ba4d18eb8cf3e8d97e5b72..8c4f359327563a84094116f4c7d1ab5978516cc4 100644 --- a/cluster-autoscaler/simulator/clustersnapshot/predicate/predicate_snapshot.go +++ b/cluster-autoscaler/simulator/clustersnapshot/predicate/predicate_snapshot.go @@ -24,6 +24,7 @@ import ( "k8s.io/autoscaler/cluster-autoscaler/simulator/clustersnapshot" drautils "k8s.io/autoscaler/cluster-autoscaler/simulator/dynamicresources/utils" "k8s.io/autoscaler/cluster-autoscaler/simulator/framework" + "k8s.io/dynamic-resource-allocation/resourceclaim" schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework" ) @@ -245,7 +246,7 @@ func (s *PredicateSnapshot) modifyResourceClaimsForNewPod(podInfo *framework.Pod // so we don't add them. The claims should already be allocated in the provided PodInfo. var podOwnedClaims []*resourceapi.ResourceClaim for _, claim := range podInfo.NeededResourceClaims { - if ownerName, _ := drautils.ClaimOwningPod(claim); ownerName != "" { + if err := resourceclaim.IsForPod(podInfo.Pod, claim); err == nil { podOwnedClaims = append(podOwnedClaims, claim) } } diff --git a/cluster-autoscaler/simulator/dynamicresources/snapshot/snapshot.go b/cluster-autoscaler/simulator/dynamicresources/snapshot/snapshot.go index 35752b3d7fb3798a864d6f31a51c32994fa7ebc9..f9e2a3f2b5ba7bdd676cf6bb4ab78e8e28f6da5b 100644 --- a/cluster-autoscaler/simulator/dynamicresources/snapshot/snapshot.go +++ b/cluster-autoscaler/simulator/dynamicresources/snapshot/snapshot.go @@ -178,7 +178,7 @@ func (s Snapshot) RemovePodOwnedClaims(pod *apiv1.Pod) { // The claim isn't tracked in the snapshot for some reason. Nothing to remove/modify, so continue to the next claim. continue } - if ownerName, ownerUid := drautils.ClaimOwningPod(claim); ownerName == pod.Name && ownerUid == pod.UID { + if err := resourceclaim.IsForPod(pod, claim); err == nil { delete(s.resourceClaimsById, claimId) } else { drautils.ClearPodReservationInPlace(claim, pod) @@ -214,9 +214,7 @@ func (s Snapshot) UnreservePodClaims(pod *apiv1.Pod) error { return err } for _, claim := range claims { - ownerPodName, ownerPodUid := drautils.ClaimOwningPod(claim) - podOwnedClaim := ownerPodName == pod.Name && ownerPodUid == ownerPodUid - + podOwnedClaim := resourceclaim.IsForPod(pod, claim) == nil drautils.ClearPodReservationInPlace(claim, pod) if podOwnedClaim || !drautils.ClaimInUse(claim) { drautils.DeallocateClaimInPlace(claim) diff --git a/cluster-autoscaler/simulator/dynamicresources/utils/resource_claims.go b/cluster-autoscaler/simulator/dynamicresources/utils/resource_claims.go index 89d5547e25192d7b88fdb67ac273424cba6987c0..09cffb6d88f2b69887dc63ab78531169682dce4b 100644 --- a/cluster-autoscaler/simulator/dynamicresources/utils/resource_claims.go +++ b/cluster-autoscaler/simulator/dynamicresources/utils/resource_claims.go @@ -22,23 +22,10 @@ import ( apiv1 "k8s.io/api/core/v1" resourceapi "k8s.io/api/resource/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/component-helpers/scheduling/corev1" resourceclaim "k8s.io/dynamic-resource-allocation/resourceclaim" - "k8s.io/utils/ptr" ) -// ClaimOwningPod returns the name and UID of the Pod owner of the provided claim. If the claim isn't -// owned by a Pod, empty strings are returned. -func ClaimOwningPod(claim *resourceapi.ResourceClaim) (string, types.UID) { - for _, owner := range claim.OwnerReferences { - if ptr.Deref(owner.Controller, false) && owner.APIVersion == "v1" && owner.Kind == "Pod" { - return owner.Name, owner.UID - } - } - return "", "" -} - // ClaimAllocated returns whether the provided claim is allocated. func ClaimAllocated(claim *resourceapi.ResourceClaim) bool { return claim.Status.Allocation != nil diff --git a/cluster-autoscaler/simulator/dynamicresources/utils/resource_claims_test.go b/cluster-autoscaler/simulator/dynamicresources/utils/resource_claims_test.go index 6130d74d9ad000c316657e640b43aacbabf51616..2eb47aecf0d9d55b88690c8e7636de22246dacce 100644 --- a/cluster-autoscaler/simulator/dynamicresources/utils/resource_claims_test.go +++ b/cluster-autoscaler/simulator/dynamicresources/utils/resource_claims_test.go @@ -25,84 +25,8 @@ import ( apiv1 "k8s.io/api/core/v1" resourceapi "k8s.io/api/resource/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" ) -func TestClaimOwningPod(t *testing.T) { - truePtr := true - for _, tc := range []struct { - testName string - claim *resourceapi.ResourceClaim - wantName string - wantUid types.UID - }{ - { - testName: "claim with no owners", - claim: &resourceapi.ResourceClaim{ - ObjectMeta: metav1.ObjectMeta{ - Name: "claim", UID: "claim", Namespace: "default", - }, - }, - wantName: "", - wantUid: "", - }, - { - testName: "claim with non-Pod owners", - claim: &resourceapi.ResourceClaim{ - ObjectMeta: metav1.ObjectMeta{ - Name: "claim", UID: "claim", Namespace: "default", - OwnerReferences: []metav1.OwnerReference{ - {Name: "owner1", UID: "owner1uid", APIVersion: "v1", Kind: "ReplicationController", Controller: &truePtr}, - {Name: "owner2", UID: "owner2uid", APIVersion: "v1", Kind: "ConfigMap"}, - }, - }, - }, - wantName: "", - wantUid: "", - }, - { - testName: "claim with a Pod non-controller owner", - claim: &resourceapi.ResourceClaim{ - ObjectMeta: metav1.ObjectMeta{ - Name: "claim", UID: "claim", Namespace: "default", - OwnerReferences: []metav1.OwnerReference{ - {Name: "owner1", UID: "owner1uid", APIVersion: "v1", Kind: "ReplicationController"}, - {Name: "owner2", UID: "owner2uid", APIVersion: "v1", Kind: "ConfigMap"}, - {Name: "owner3", UID: "owner3uid", APIVersion: "v1", Kind: "Pod"}, - }, - }, - }, - wantName: "", - wantUid: "", - }, - { - testName: "claim with a Pod controller owner", - claim: &resourceapi.ResourceClaim{ - ObjectMeta: metav1.ObjectMeta{ - Name: "claim", UID: "claim", Namespace: "default", - OwnerReferences: []metav1.OwnerReference{ - {Name: "owner1", UID: "owner1uid", APIVersion: "v1", Kind: "ReplicationController"}, - {Name: "owner2", UID: "owner2uid", APIVersion: "v1", Kind: "ConfigMap"}, - {Name: "owner3", UID: "owner3uid", APIVersion: "v1", Kind: "Pod", Controller: &truePtr}, - }, - }, - }, - wantName: "owner3", - wantUid: "owner3uid", - }, - } { - t.Run(tc.testName, func(t *testing.T) { - name, uid := ClaimOwningPod(tc.claim) - if tc.wantName != name { - t.Errorf("ClaimOwningPod(): unexpected output name: want %s, got %s", tc.wantName, name) - } - if tc.wantUid != uid { - t.Errorf("ClaimOwningPod(): unexpected output UID: want %v, got %v", tc.wantUid, uid) - } - }) - } -} - func TestClaimAllocated(t *testing.T) { for _, tc := range []struct { testName string diff --git a/cluster-autoscaler/simulator/dynamicresources/utils/sanitize.go b/cluster-autoscaler/simulator/dynamicresources/utils/sanitize.go index 48ab44de82a221d124d69e3cbcc0ca2556e52f7c..00dd7f3ae2084f2b803e4e2b5790135e4bf322b5 100644 --- a/cluster-autoscaler/simulator/dynamicresources/utils/sanitize.go +++ b/cluster-autoscaler/simulator/dynamicresources/utils/sanitize.go @@ -23,6 +23,7 @@ import ( resourceapi "k8s.io/api/resource/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/uuid" + "k8s.io/dynamic-resource-allocation/resourceclaim" "k8s.io/utils/set" ) @@ -61,7 +62,7 @@ func SanitizedNodeResourceSlices(nodeLocalSlices []*resourceapi.ResourceSlice, n func SanitizedPodResourceClaims(newOwner, oldOwner *v1.Pod, claims []*resourceapi.ResourceClaim, nameSuffix, newNodeName, oldNodeName string, oldNodePoolNames set.Set[string]) ([]*resourceapi.ResourceClaim, error) { var result []*resourceapi.ResourceClaim for _, claim := range claims { - if ownerName, ownerUid := ClaimOwningPod(claim); ownerName != oldOwner.Name || ownerUid != oldOwner.UID { + if err := resourceclaim.IsForPod(oldOwner, claim); err != nil { // Only claims owned by the pod are bound to its lifecycle. The lifecycle of other claims is independent, and they're most likely shared // by multiple pods. They shouldn't be sanitized or duplicated - just add unchanged to the result. result = append(result, claim) diff --git a/cluster-autoscaler/simulator/node_info_utils_test.go b/cluster-autoscaler/simulator/node_info_utils_test.go index 70013cb927b7c722a523d4233cd2c1972b5e01f6..7e396f8c521c32bb78f115da6a89fe617ce07599 100644 --- a/cluster-autoscaler/simulator/node_info_utils_test.go +++ b/cluster-autoscaler/simulator/node_info_utils_test.go @@ -40,6 +40,7 @@ import ( "k8s.io/autoscaler/cluster-autoscaler/utils/labels" "k8s.io/autoscaler/cluster-autoscaler/utils/taints" . "k8s.io/autoscaler/cluster-autoscaler/utils/test" + "k8s.io/dynamic-resource-allocation/resourceclaim" ) var ( @@ -601,7 +602,7 @@ func verifySanitizedPods(initialPods, sanitizedPods []*framework.PodInfo, wantNo return fmt.Errorf("sanitized Pod unexpected diff (-want +got): %s", diff) } - if err := verifySanitizedPodResourceClaims(initialPod.NeededResourceClaims, sanitizedPod.NeededResourceClaims, nameSuffix); err != nil { + if err := verifySanitizedPodResourceClaims(initialPod, sanitizedPod, nameSuffix); err != nil { return err } } @@ -633,7 +634,11 @@ func verifySanitizedNodeResourceSlices(initialSlices, sanitizedSlices []*resourc return nil } -func verifySanitizedPodResourceClaims(initialClaims, sanitizedClaims []*resourceapi.ResourceClaim, nameSuffix string) error { +func verifySanitizedPodResourceClaims(initialPod, sanitizedPod *framework.PodInfo, nameSuffix string) error { + initialClaims := initialPod.NeededResourceClaims + sanitizedClaims := sanitizedPod.NeededResourceClaims + owningPod := initialPod.Pod + if len(initialClaims) != len(sanitizedClaims) { return fmt.Errorf("want %d NeededResourceClaims in sanitized NodeInfo, got %d", len(initialClaims), len(sanitizedClaims)) } @@ -642,7 +647,9 @@ func verifySanitizedPodResourceClaims(initialClaims, sanitizedClaims []*resource initialClaim := initialClaims[i] // Pod-owned claims should be sanitized, other claims shouldn't. - if owningPod, _ := drautils.ClaimOwningPod(initialClaim); owningPod != "" { + err := resourceclaim.IsForPod(owningPod, initialClaim) + isPodOwned := err == nil + if isPodOwned { // Pod-owned claim, verify that it was sanitized. if sanitizedClaim.Name == initialClaim.Name || !strings.HasSuffix(sanitizedClaim.Name, nameSuffix) { return fmt.Errorf("sanitized ResourceClaim name unexpected: want (different than %q, ending in %q), got %q", initialClaim.Name, nameSuffix, sanitizedClaim.Name)