Skip to content

Commit

Permalink
Added more plugin logic
Browse files Browse the repository at this point in the history
  • Loading branch information
@michaelawyu
michaelawyu committed Jul 20, 2023
1 parent 5a03556 commit ee4da04
Show file tree
Hide file tree
Showing 2 changed files with 331 additions and 3 deletions.
52 changes: 49 additions & 3 deletions pkg/scheduler/framework/plugins/topologyspreadconstraints/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,55 @@ func countByDomain(clusters []fleetv1beta1.MemberCluster, state framework.CycleS
// willViolate returns whether producing one more binding in a domain would lead
// to violations; it will also return the skew change (the delta between the max skew after setting
// up a placement and the one before) caused by the provisional placement.
func willViolate(_ *bindingCounterByDomain, _ domainName, _ int) (violated bool, skewChange int, err error) {
// Not yet implemented.
return false, 0, nil
func willViolate(counter *bindingCounterByDomain, name domainName, maxSkew int) (violated bool, skewChange int, err error) {
count, ok := counter.Count(name)
if !ok {
// The domain is not registered in the counter; normally this would never
// happen as the state being evaluated is consistent and the counter tracks
// all domains.
return false, 0, fmt.Errorf("domain %s is not registered in the counter", name)
}

// Note if the earlier check passes, all special counts must be present.
smallest, _ := counter.Smallest()
secondSmallest, _ := counter.SecondSmallest()
largest, _ := counter.Largest()

if count < smallest || count > largest {
// Perform a sanity check here; normally this would never happen as the counter
// tracks all domains.
return false, 0, fmt.Errorf("the counter has invalid special counts: [%d, %d], received %d", smallest, largest, count)
}

currentSkew := int(largest - smallest)
switch {
case largest == smallest:
// Currently all domains have the same count of bindings.
//
// In this case, the placement will increase the skew by 1.
return currentSkew+1 > maxSkew, 1, nil
case count == smallest && smallest != secondSmallest:
// The plan is to place at the domain with the smallest count of bindings, and currently
// there are no other domains with the same smallest count.
//
// In this case, the placement will decrease the skew by 1.
return currentSkew-1 > maxSkew, -1, nil
case count == largest:
// The plan is to place at the domain with the largest count of bindings.
//
// In this case, the placement will increase the skew by 1.
return currentSkew+1 > maxSkew, 1, nil
default:
// In all the other cases, the skew will not be affected.
//
// These cases include:
//
// * place at the domain with the smallest count of bindings, but there are other
// domains with the same smallest count; and
// * place at the domain with a count of bindings that is greater than the smallest
// count, but less than the largest count.
return currentSkew > maxSkew, 0, nil
}
}

// classifyConstraints classifies topology spread constraints in a policy based on their
Expand Down
282 changes: 282 additions & 0 deletions pkg/scheduler/framework/plugins/topologyspreadconstraints/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -662,3 +662,285 @@ func TestClassifyConstriants(t *testing.T) {
})
}
}

// TestWillViolate tests the willViolate function.
func TestWillViolate(t *testing.T) {
topologyValue4 := domainName("topology-value-4")

testCases := []struct {
name string
counter *bindingCounterByDomain
dn domainName
maxSkew int
wantViolated bool
wantSkewChange int
expectedToFail bool
}{
{
name: "domain not registered",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
},
smallest: 1,
secondSmallest: 1,
largest: 1,
},
dn: topologyValue2,
expectedToFail: true,
},
{
name: "invalid count (smaller than smallest)",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
},
smallest: 2,
secondSmallest: 2,
largest: 2,
},
dn: topologyValue1,
expectedToFail: true,
},
{
name: "invalid count (larger than largest)",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 2,
},
smallest: 1,
secondSmallest: 1,
largest: 1,
},
dn: topologyValue2,
expectedToFail: true,
},
{
name: "one count only, no violation",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 2,
topologyValue2: 2,
topologyValue3: 2,
},
smallest: 2,
secondSmallest: 2,
largest: 2,
},
dn: topologyValue1,
maxSkew: 1,
wantViolated: false,
wantSkewChange: 1,
},
{
name: "one count only, violated",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 2,
topologyValue2: 2,
topologyValue3: 2,
},
smallest: 2,
secondSmallest: 2,
largest: 2,
},
dn: topologyValue2,
// This should never happen as the minimum required for maxSkew in API is 1; it is
// added here solely for the purpose of testing.
maxSkew: 0,
wantViolated: true,
wantSkewChange: 1,
},
{
name: "one smallest count, pick smallest count, no violation",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
topologyValue2: 2,
topologyValue3: 2,
},
smallest: 1,
secondSmallest: 2,
largest: 2,
},
dn: topologyValue1,
maxSkew: 1,
wantViolated: false,
wantSkewChange: -1,
},
{
name: "one smallest count, pick smallest count, violated",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
topologyValue2: 4,
topologyValue3: 4,
},
smallest: 1,
secondSmallest: 4,
largest: 4,
},
dn: topologyValue1,
maxSkew: 1,
wantViolated: true,
wantSkewChange: -1,
},
{
name: "multiple smallest counts, pick smallest count, no violation",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
topologyValue2: 1,
topologyValue3: 2,
},
smallest: 1,
secondSmallest: 1,
largest: 2,
},
dn: topologyValue2,
maxSkew: 1,
wantViolated: false,
wantSkewChange: 0,
},
{
name: "multiple smallest counts, pick smallest count, violated",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
topologyValue2: 1,
topologyValue3: 3,
},
smallest: 1,
secondSmallest: 1,
largest: 3,
},
dn: topologyValue1,
maxSkew: 1,
wantViolated: true,
wantSkewChange: 0,
},
{
name: "separate special counts, pick second smallest, no violation",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
topologyValue2: 2,
topologyValue3: 3,
},
smallest: 1,
secondSmallest: 2,
largest: 3,
},
dn: topologyValue2,
maxSkew: 2,
wantViolated: false,
wantSkewChange: 0,
},
{
name: "separate special counts, pick second smallest, violated",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
topologyValue2: 2,
topologyValue3: 3,
},
smallest: 1,
secondSmallest: 2,
largest: 3,
},
dn: topologyValue2,
maxSkew: 1,
wantViolated: true,
wantSkewChange: 0,
},
{
name: "separate special counts, pick a count larger than second smallest, less than largest, no violation",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
topologyValue2: 2,
topologyValue3: 5,
topologyValue4: 3,
},
smallest: 1,
secondSmallest: 2,
largest: 5,
},
dn: topologyValue4,
maxSkew: 5,
wantViolated: false,
wantSkewChange: 0,
},
{
name: "separate special counts, pick a count larger than second smallest, less than largest, violated",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
topologyValue2: 2,
topologyValue3: 5,
topologyValue4: 3,
},
smallest: 1,
secondSmallest: 2,
largest: 5,
},
dn: topologyValue4,
maxSkew: 3,
wantViolated: true,
wantSkewChange: 0,
},
{
name: "separate special counts, pick largest, no violation",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
topologyValue2: 2,
topologyValue3: 5,
},
smallest: 1,
secondSmallest: 2,
largest: 5,
},
dn: topologyValue3,
maxSkew: 5,
wantViolated: false,
wantSkewChange: 1,
},
{
name: "separate special counts, pick largest, violated",
counter: &bindingCounterByDomain{
counter: map[domainName]count{
topologyValue1: 1,
topologyValue2: 2,
topologyValue3: 5,
},
smallest: 1,
secondSmallest: 2,
largest: 5,
},
dn: topologyValue3,
maxSkew: 2,
wantViolated: true,
wantSkewChange: 1,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
violated, skewChange, err := willViolate(tc.counter, tc.dn, tc.maxSkew)
if tc.expectedToFail {
if err == nil {
t.Errorf("willViolate(), want error")
}

return
}
if violated != tc.wantViolated {
t.Errorf("willViolate() violated = %t, want %t", violated, tc.wantViolated)
}
if skewChange != tc.wantSkewChange {
t.Errorf("willViolate() skewChange: got %v, want %v", skewChange, tc.wantSkewChange)
}
})
}
}

0 comments on commit ee4da04

Please sign in to comment.