-
Notifications
You must be signed in to change notification settings - Fork 6
SPLAT-2238: Redesign logic to handle 4 pools with 3 max vcenters #69
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.
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
Merged
openshift-merge-bot
merged 1 commit into
openshift-eng:main
from
vr4manta:SPLAT-2238_fix3
Jul 13, 2026
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,237 @@ | ||
| package controller | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| v1 "github.com/openshift-splat-team/vsphere-capacity-manager/pkg/apis/vspherecapacitymanager.splat.io/v1" | ||
| "github.com/openshift-splat-team/vsphere-capacity-manager/pkg/utils" | ||
| configv1 "github.com/openshift/api/config/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| // TestDynamicVCenterFiltering tests the dynamic filtering logic that adapts | ||
| // based on remaining vCenter slots and remaining pools needed. | ||
| func TestDynamicVCenterFiltering(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| requiredPools int | ||
| vcentersLimit int | ||
| assignedPools int // How many pools already assigned | ||
| vcentersInUse int // How many distinct vCenters in use | ||
| availablePools []*v1.Pool | ||
| expectExclusions bool | ||
| expectedMinPoolsNeeded int // Expected minPoolsPerVCenter threshold | ||
| description string | ||
| }{ | ||
| { | ||
| name: "cap reached - only allow vcenters in use", | ||
| requiredPools: 4, | ||
| vcentersLimit: 3, | ||
| assignedPools: 3, // Already have 3 pools | ||
| vcentersInUse: 3, // From 3 different vCenters (cap reached) | ||
| availablePools: []*v1.Pool{ | ||
| createPool("vcenter-A", "pool-a", 100, 1000), | ||
| createPool("vcenter-B", "pool-b", 100, 1000), | ||
| createPool("vcenter-C", "pool-c", 100, 1000), | ||
| createPool("vcenter-D", "pool-d", 100, 1000), // Should be excluded | ||
| }, | ||
| expectExclusions: true, // Should exclude vcenter-D | ||
| description: "Cap reached: should exclude all vCenters not in use", | ||
| }, | ||
| { | ||
| name: "one slot left for two pools - require multi-pool vcenter", | ||
| requiredPools: 4, | ||
| vcentersLimit: 3, | ||
| assignedPools: 2, // Already have 2 pools | ||
| vcentersInUse: 2, // From 2 different vCenters | ||
| availablePools: []*v1.Pool{ | ||
| // vcenter-A and B already in use (not counted here) | ||
| // vcenter-C has 1 pool | ||
| createPool("vcenter-C", "pool-c1", 100, 1000), | ||
| // vcenter-D has 2 pools | ||
| createPool("vcenter-D", "pool-d1", 100, 1000), | ||
| createPool("vcenter-D", "pool-d2", 100, 1000), | ||
| }, | ||
| expectExclusions: true, | ||
| expectedMinPoolsNeeded: 2, // ceil(2 remaining pools / 1 remaining slot) = 2 | ||
| description: "Need 2 pools from 1 slot: should exclude single-pool vCenters", | ||
| }, | ||
| { | ||
| name: "two slots left for three pools - require 2 per vcenter", | ||
| requiredPools: 4, | ||
| vcentersLimit: 3, | ||
| assignedPools: 1, // Already have 1 pool | ||
| vcentersInUse: 1, // From 1 vCenter | ||
| availablePools: []*v1.Pool{ | ||
| // vcenter-A already in use | ||
| // vcenter-B has 1 pool - should be excluded | ||
| createPool("vcenter-B", "pool-b1", 100, 1000), | ||
| // vcenter-C has 2 pools - should be allowed | ||
| createPool("vcenter-C", "pool-c1", 100, 1000), | ||
| createPool("vcenter-C", "pool-c2", 100, 1000), | ||
| // vcenter-D has 3 pools - should be allowed | ||
| createPool("vcenter-D", "pool-d1", 100, 1000), | ||
| createPool("vcenter-D", "pool-d2", 100, 1000), | ||
| createPool("vcenter-D", "pool-d3", 100, 1000), | ||
| }, | ||
| expectExclusions: true, | ||
| expectedMinPoolsNeeded: 2, // ceil(3 remaining / 2 slots) = 2 | ||
| description: "Need 3 pools from 2 slots: exclude vCenters with < 2 pools", | ||
| }, | ||
| { | ||
| name: "plenty of slots - no dynamic filtering", | ||
| requiredPools: 4, | ||
| vcentersLimit: 5, | ||
| assignedPools: 1, | ||
| vcentersInUse: 1, | ||
| availablePools: []*v1.Pool{ | ||
| // Remaining: 3 pools needed, 4 slots available | ||
| // 3 <= 4, so no dynamic filtering needed | ||
| createPool("vcenter-B", "pool-b1", 100, 1000), | ||
| createPool("vcenter-C", "pool-c1", 100, 1000), | ||
| createPool("vcenter-D", "pool-d1", 100, 1000), | ||
| }, | ||
| expectExclusions: false, | ||
| description: "More slots than pools: no dynamic filtering needed", | ||
| }, | ||
| { | ||
| name: "all remaining vcenters excluded by dynamic filter - should trigger recovery", | ||
| requiredPools: 4, | ||
| vcentersLimit: 3, | ||
| assignedPools: 1, // Already have 1 pool from vcenter-A | ||
| vcentersInUse: 1, | ||
| availablePools: []*v1.Pool{ | ||
| // vcenter-A already in use | ||
| // Need 3 more pools, have 2 slots left | ||
| // minPoolsPerVCenter = ceil(3/2) = 2 | ||
| // All remaining vCenters have only 1 pool → ALL excluded | ||
| createPool("vcenter-B", "pool-b1", 100, 1000), // 1 pool < 2 | ||
| createPool("vcenter-C", "pool-c1", 100, 1000), // 1 pool < 2 | ||
| createPool("vcenter-D", "pool-d1", 100, 1000), // 1 pool < 2 | ||
| }, | ||
| expectExclusions: true, | ||
| expectedMinPoolsNeeded: 2, // ceil(3/2) = 2 | ||
| description: "All remaining vCenters excluded: should trigger deadlock recovery", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| lease := &v1.Lease{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-lease", | ||
| Namespace: "default", | ||
| }, | ||
| Spec: v1.LeaseSpec{ | ||
| VCpus: 24, | ||
| Memory: 96, | ||
| Pools: tt.requiredPools, | ||
| VCenters: tt.vcentersLimit, | ||
| }, | ||
| } | ||
|
|
||
| // Simulate assigned pools (create dummy pools) | ||
| assignedPools := make([]*v1.Pool, tt.assignedPools) | ||
| vcentersInUse := make(map[string]bool) | ||
| for i := 0; i < tt.assignedPools; i++ { | ||
| // Use different vCenters to match vcentersInUse count | ||
| vcenterName := "" | ||
| if i < tt.vcentersInUse { | ||
| vcenterName = string(rune('A' + i)) // A, B, C, ... | ||
| } else { | ||
| // Reuse an earlier vCenter | ||
| vcenterName = "A" | ||
| } | ||
| assignedPools[i] = createPool("vcenter-"+vcenterName, "assigned-pool-"+string(rune('1'+i)), 100, 1000) | ||
| vcentersInUse["vcenter-"+vcenterName] = true | ||
| } | ||
|
|
||
| // Calculate what the controller would calculate | ||
| remainingSlots := tt.vcentersLimit - len(vcentersInUse) | ||
| remainingPools := tt.requiredPools - len(assignedPools) | ||
|
|
||
| t.Logf("%s", tt.description) | ||
| t.Logf("Remaining slots: %d, Remaining pools: %d", remainingSlots, remainingPools) | ||
|
|
||
| var excludedVCenters map[string]bool | ||
|
|
||
| // Replicate the controller's logic | ||
| if len(vcentersInUse) >= lease.Spec.VCenters { | ||
| // Cap reached | ||
| excludedVCenters = make(map[string]bool) | ||
| for _, p := range tt.availablePools { | ||
| if !vcentersInUse[p.Spec.Server] { | ||
| excludedVCenters[p.Spec.Server] = true | ||
| } | ||
| } | ||
| t.Logf("Cap reached - excluded %d vCenters", len(excludedVCenters)) | ||
| } else if remainingSlots > 0 && remainingPools > remainingSlots { | ||
| // Dynamic filtering | ||
| minPoolsPerVCenter := (remainingPools-1)/remainingSlots + 1 | ||
| t.Logf("Dynamic filtering: minPoolsPerVCenter = %d", minPoolsPerVCenter) | ||
|
|
||
| if tt.expectedMinPoolsNeeded > 0 && minPoolsPerVCenter != tt.expectedMinPoolsNeeded { | ||
| t.Errorf("Expected minPoolsPerVCenter=%d, got %d", | ||
| tt.expectedMinPoolsNeeded, minPoolsPerVCenter) | ||
| } | ||
|
|
||
| // Count pools per vCenter | ||
| fittingPools, _ := utils.GetFittingPools(lease, tt.availablePools, nil) | ||
| poolsPerVCenter := make(map[string]int) | ||
| for _, p := range fittingPools { | ||
| if !vcentersInUse[p.Spec.Server] { | ||
| poolsPerVCenter[p.Spec.Server]++ | ||
| } | ||
| } | ||
|
|
||
| // Exclude vCenters with insufficient pools | ||
| excludedVCenters = make(map[string]bool) | ||
| for _, p := range tt.availablePools { | ||
| if !vcentersInUse[p.Spec.Server] { | ||
| if poolsPerVCenter[p.Spec.Server] < minPoolsPerVCenter { | ||
| excludedVCenters[p.Spec.Server] = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| t.Logf("Dynamic filter excluded %d vCenters (with < %d pools)", | ||
| len(excludedVCenters), minPoolsPerVCenter) | ||
|
|
||
| for server, count := range poolsPerVCenter { | ||
| excluded := excludedVCenters[server] | ||
| t.Logf(" %s: %d pools → excluded=%v", server, count, excluded) | ||
| } | ||
| } | ||
|
|
||
| // Verify expectations | ||
| if tt.expectExclusions && len(excludedVCenters) == 0 { | ||
| t.Error("Expected exclusions but got none") | ||
| } | ||
| if !tt.expectExclusions && len(excludedVCenters) > 0 { | ||
| t.Errorf("Expected no exclusions but got %d", len(excludedVCenters)) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // Helper function to create a pool for testing | ||
| func createPool(vcenterServer, poolName string, vcpus, memory int) *v1.Pool { | ||
| return &v1.Pool{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: poolName, | ||
| }, | ||
| Spec: v1.PoolSpec{ | ||
| FailureDomainSpec: v1.FailureDomainSpec{ | ||
| VSpherePlatformFailureDomainSpec: configv1.VSpherePlatformFailureDomainSpec{ | ||
| Server: vcenterServer, | ||
| }, | ||
| }, | ||
| VCpus: vcpus, | ||
| Memory: memory, | ||
| }, | ||
| Status: v1.PoolStatus{ | ||
| VCpusAvailable: vcpus, | ||
| MemoryAvailable: memory, | ||
| }, | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how many times around will we go around before we reset the condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea here is that we move lease back to pending and return. next reconcile loop will attempt again. this way if logic is still broke, we have a chance of the other pools getting resources to allow it to be fulfilled. If this is bad, i can change back.