Skip to content
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

Jump to bottom

DRA: test for structured parameters #123938

Merged
merged 7 commits into from Apr 18, 2024
Merged

DRA: test for structured parameters #123938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f149d6d
dra e2e: watch claims and validate them
pohly Mar 8, 2024
cf8fffa
dra e2e: sanity check resource handle
pohly Mar 11, 2024
95136db
dra scheduler: fix re-allocation of claim with structured parameters
pohly Mar 22, 2024
4126e37
dra controller: unit tests
pohly Mar 14, 2024
607261e
dra scheduler: spelling fix
pohly Mar 12, 2024
458e227
dra scheduler: unit tests
pohly Mar 14, 2024
6f5696b
dra scheduler: simplify unit tests
pohly Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
89 changes: 89 additions & 0 deletions pkg/controller/resourceclaim/controller_test.go
View file
Open in desktop
Expand Up @@ -293,6 +293,31 @@ func TestSyncHandler(t *testing.T) {
}(),
expectedMetrics: expectedMetrics{0, 0},
},
{
name: "clear-reserved-delayed-allocation-structured",
Copy link
Member

Choose a reason for hiding this comment

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

We are missing a test where the allocation isn't cleared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added (but not pushed yet - I'm not sure whether I'll be able finish today).

pods: []*v1.Pod{},
key: claimKey(testClaimReserved),
claims: []*resourcev1alpha2.ResourceClaim{structuredParameters(testClaimReserved)},
expectedClaims: func() []resourcev1alpha2.ResourceClaim {
claim := testClaimAllocated.DeepCopy()
claim.Finalizers = []string{}
claim.Status.Allocation = nil
return []resourcev1alpha2.ResourceClaim{*claim}
}(),
expectedMetrics: expectedMetrics{0, 0},
},
{
name: "dont-clear-reserved-delayed-allocation-structured",
pods: []*v1.Pod{testPodWithResource},
key: claimKey(testClaimReserved),
claims: func() []*resourcev1alpha2.ResourceClaim {
claim := structuredParameters(testClaimReserved)
claim = reserveClaim(claim, otherTestPod)
return []*resourcev1alpha2.ResourceClaim{claim}
}(),
expectedClaims: []resourcev1alpha2.ResourceClaim{*structuredParameters(testClaimReserved)},
expectedMetrics: expectedMetrics{0, 0},
},
{
name: "clear-reserved-immediate-allocation",
pods: []*v1.Pod{},
Expand All @@ -309,6 +334,62 @@ func TestSyncHandler(t *testing.T) {
}(),
expectedMetrics: expectedMetrics{0, 0},
},
{
name: "clear-reserved-immediate-allocation-structured",
pods: []*v1.Pod{},
key: claimKey(testClaimReserved),
claims: func() []*resourcev1alpha2.ResourceClaim {
claim := structuredParameters(testClaimReserved.DeepCopy())
claim.Spec.AllocationMode = resourcev1alpha2.AllocationModeImmediate
return []*resourcev1alpha2.ResourceClaim{claim}
}(),
expectedClaims: func() []resourcev1alpha2.ResourceClaim {
claim := structuredParameters(testClaimAllocated.DeepCopy())
claim.Spec.AllocationMode = resourcev1alpha2.AllocationModeImmediate
return []resourcev1alpha2.ResourceClaim{*claim}
}(),
expectedMetrics: expectedMetrics{0, 0},
},
{
name: "clear-reserved-immediate-allocation-structured-deleted",
pods: []*v1.Pod{},
key: claimKey(testClaimReserved),
claims: func() []*resourcev1alpha2.ResourceClaim {
claim := structuredParameters(testClaimReserved.DeepCopy())
claim.Spec.AllocationMode = resourcev1alpha2.AllocationModeImmediate
claim.DeletionTimestamp = &metav1.Time{}
return []*resourcev1alpha2.ResourceClaim{claim}
}(),
expectedClaims: func() []resourcev1alpha2.ResourceClaim {
claim := structuredParameters(testClaimAllocated.DeepCopy())
claim.Spec.AllocationMode = resourcev1alpha2.AllocationModeImmediate
claim.DeletionTimestamp = &metav1.Time{}
claim.Finalizers = []string{}
claim.Status.Allocation = nil
return []resourcev1alpha2.ResourceClaim{*claim}
}(),
expectedMetrics: expectedMetrics{0, 0},
},
{
name: "immediate-allocation-structured-deleted",
pods: []*v1.Pod{},
key: claimKey(testClaimReserved),
claims: func() []*resourcev1alpha2.ResourceClaim {
claim := structuredParameters(testClaimAllocated.DeepCopy())
claim.Spec.AllocationMode = resourcev1alpha2.AllocationModeImmediate
claim.DeletionTimestamp = &metav1.Time{}
return []*resourcev1alpha2.ResourceClaim{claim}
}(),
expectedClaims: func() []resourcev1alpha2.ResourceClaim {
claim := structuredParameters(testClaimAllocated.DeepCopy())
claim.Spec.AllocationMode = resourcev1alpha2.AllocationModeImmediate
claim.DeletionTimestamp = &metav1.Time{}
claim.Finalizers = []string{}
claim.Status.Allocation = nil
return []resourcev1alpha2.ResourceClaim{*claim}
}(),
expectedMetrics: expectedMetrics{0, 0},
},
{
name: "clear-reserved-when-done-delayed-allocation",
pods: func() []*v1.Pod {
Expand Down Expand Up @@ -546,6 +627,14 @@ func allocateClaim(claim *resourcev1alpha2.ResourceClaim) *resourcev1alpha2.Reso
return claim
}

func structuredParameters(claim *resourcev1alpha2.ResourceClaim) *resourcev1alpha2.ResourceClaim {
claim = claim.DeepCopy()
// As far the controller is concerned, a claim was allocated by us if it has
// this finalizer. For testing we don't need to update the allocation result.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a warning in the API comment for the Finalizer constant saying that third-party drivers shouldn't use this finalizer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add that in a separate PR for the API. I have another comment update pending.

claim.Finalizers = append(claim.Finalizers, resourcev1alpha2.Finalizer)
return claim
}

func reserveClaim(claim *resourcev1alpha2.ResourceClaim, pod *v1.Pod) *resourcev1alpha2.ResourceClaim {
claim = claim.DeepCopy()
claim.Status.ReservedFor = append(claim.Status.ReservedFor,
Expand Down
7 changes: 6 additions & 1 deletion pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go
View file
Open in desktop
Expand Up @@ -1209,7 +1209,7 @@ func (pl *dynamicResources) PostFilter(ctx context.Context, cs *framework.CycleS
// Then we can simply clear the allocation. Once the
// claim informer catches up, the controllers will
// be notified about this change.
clearAllocation := state.informationsForClaim[index].controller != nil
clearAllocation := state.informationsForClaim[index].structuredParameters

// Before we tell a driver to deallocate a claim, we
// have to stop telling it to allocate. Otherwise,
Expand All @@ -1228,6 +1228,7 @@ func (pl *dynamicResources) PostFilter(ctx context.Context, cs *framework.CycleS
claim := claim.DeepCopy()
claim.Status.ReservedFor = nil
if clearAllocation {
claim.Status.DriverName = ""
claim.Status.Allocation = nil
} else {
claim.Status.DeallocationRequested = true
Expand Down Expand Up @@ -1410,7 +1411,11 @@ func (pl *dynamicResources) Reserve(ctx context.Context, cs *framework.CycleStat
}
state.informationsForClaim[index].allocation = allocation
state.informationsForClaim[index].allocationDriverName = driverName
// Strictly speaking, we don't need to store the full modified object.
// The allocation would be enough. The full object is useful for
// debugging and testing, so let's make it realistic.
claim = claim.DeepCopy()
claim.Finalizers = append(claim.Finalizers, resourcev1alpha2.Finalizer)
Copy link
Member

Choose a reason for hiding this comment

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

does this all happen in memory? I don't suppose this object is persisted to the API, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All in memory. It's debatable whether it should store the whole "expected" claim at all. It's consistent, but not really required.

claim.Status.DriverName = driverName
claim.Status.Allocation = allocation
pl.inFlightAllocations.Store(claim.UID, claim)
Expand Down