From 9813ec7e8a85b27daf455075998699602e5566f5 Mon Sep 17 00:00:00 2001 From: Rob Scott Date: Wed, 17 Nov 2021 15:39:09 -0800 Subject: [PATCH] Updating TopologyCache to disregard unready endpoints in calculations --- .../endpointslice/reconciler_test.go | 2 +- .../endpointslice/topologycache/sliceinfo.go | 8 +- .../topologycache/sliceinfo_test.go | 75 ++++-- .../topologycache/topologycache.go | 7 +- .../topologycache/topologycache_test.go | 227 ++++++++++++------ .../endpointslice/topologycache/utils.go | 22 +- .../endpointslice/topologycache/utils_test.go | 84 +++++-- pkg/controller/util/endpointslice/utils.go | 27 +++ 8 files changed, 333 insertions(+), 119 deletions(-) create mode 100644 pkg/controller/util/endpointslice/utils.go diff --git a/pkg/controller/endpointslice/reconciler_test.go b/pkg/controller/endpointslice/reconciler_test.go index 7e4ac3ce6f1..2e71419f352 100644 --- a/pkg/controller/endpointslice/reconciler_test.go +++ b/pkg/controller/endpointslice/reconciler_test.go @@ -516,7 +516,7 @@ func TestReconcile1EndpointSlicePublishNotReadyAddresses(t *testing.T) { endpointSlices := fetchEndpointSlices(t, client, namespace) for _, endpointSlice := range endpointSlices { for i, endpoint := range endpointSlice.Endpoints { - if !*endpoint.Conditions.Ready { + if !endpointsliceutil.EndpointReady(endpoint) { t.Errorf("Expected endpoints[%d] to be ready", i) } } diff --git a/pkg/controller/endpointslice/topologycache/sliceinfo.go b/pkg/controller/endpointslice/topologycache/sliceinfo.go index f525a1cdee7..6bfd350fdf7 100644 --- a/pkg/controller/endpointslice/topologycache/sliceinfo.go +++ b/pkg/controller/endpointslice/topologycache/sliceinfo.go @@ -30,16 +30,16 @@ type SliceInfo struct { Unchanged []*discovery.EndpointSlice } -func (si *SliceInfo) getTotalEndpoints() int { +func (si *SliceInfo) getTotalReadyEndpoints() int { totalEndpoints := 0 for _, slice := range si.ToCreate { - totalEndpoints += len(slice.Endpoints) + totalEndpoints += numReadyEndpoints(slice.Endpoints) } for _, slice := range si.ToUpdate { - totalEndpoints += len(slice.Endpoints) + totalEndpoints += numReadyEndpoints(slice.Endpoints) } for _, slice := range si.Unchanged { - totalEndpoints += len(slice.Endpoints) + totalEndpoints += numReadyEndpoints(slice.Endpoints) } return totalEndpoints } diff --git a/pkg/controller/endpointslice/topologycache/sliceinfo_test.go b/pkg/controller/endpointslice/topologycache/sliceinfo_test.go index 36627f637eb..27970a96fcc 100644 --- a/pkg/controller/endpointslice/topologycache/sliceinfo_test.go +++ b/pkg/controller/endpointslice/topologycache/sliceinfo_test.go @@ -21,13 +21,15 @@ import ( "testing" discovery "k8s.io/api/discovery/v1" + utilpointer "k8s.io/utils/pointer" ) -func TestGetTotalEndpoints(t *testing.T) { +func Test_getTotalReadyEndpoints(t *testing.T) { testCases := []struct { - name string - si *SliceInfo - expectedTotal int + name string + si *SliceInfo + expectedTotalReady int + expectedTotal int // this is really just testing the test helper }{{ name: "empty", si: &SliceInfo{}, @@ -35,30 +37,49 @@ func TestGetTotalEndpoints(t *testing.T) { }, { name: "empty slice", si: &SliceInfo{ - ToCreate: []*discovery.EndpointSlice{sliceWithNEndpoints(0)}, + ToCreate: []*discovery.EndpointSlice{sliceWithNEndpoints(0, 0)}, }, - expectedTotal: 0, + expectedTotalReady: 0, + expectedTotal: 0, }, { name: "multiple slices", si: &SliceInfo{ - ToCreate: []*discovery.EndpointSlice{sliceWithNEndpoints(15), sliceWithNEndpoints(8)}, + ToCreate: []*discovery.EndpointSlice{sliceWithNEndpoints(15, 0), sliceWithNEndpoints(8, 0)}, }, - expectedTotal: 23, + expectedTotalReady: 23, + expectedTotal: 23, }, { name: "slices for all", si: &SliceInfo{ - ToCreate: []*discovery.EndpointSlice{sliceWithNEndpoints(15), sliceWithNEndpoints(8)}, - ToUpdate: []*discovery.EndpointSlice{sliceWithNEndpoints(2)}, - Unchanged: []*discovery.EndpointSlice{sliceWithNEndpoints(100), sliceWithNEndpoints(90)}, + ToCreate: []*discovery.EndpointSlice{sliceWithNEndpoints(15, 0), sliceWithNEndpoints(8, 0)}, + ToUpdate: []*discovery.EndpointSlice{sliceWithNEndpoints(2, 0)}, + Unchanged: []*discovery.EndpointSlice{sliceWithNEndpoints(100, 0), sliceWithNEndpoints(90, 0)}, }, - expectedTotal: 215, + expectedTotalReady: 215, + expectedTotal: 215, + }, { + name: "slices for all with some unready", + si: &SliceInfo{ + ToCreate: []*discovery.EndpointSlice{sliceWithNEndpoints(15, 3), sliceWithNEndpoints(5, 4)}, + ToUpdate: []*discovery.EndpointSlice{sliceWithNEndpoints(3, 8)}, + Unchanged: []*discovery.EndpointSlice{sliceWithNEndpoints(98, 2), sliceWithNEndpoints(90, 6)}, + }, + expectedTotalReady: 211, + expectedTotal: 234, }} for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - actualTotal := tc.si.getTotalEndpoints() + actualTotal := countEndpoints(tc.si.ToCreate) + countEndpoints(tc.si.ToUpdate) + countEndpoints(tc.si.Unchanged) + if actualTotal != tc.expectedTotal { - t.Errorf("Expected %d, got %d", tc.expectedTotal, actualTotal) + // This likely means that sliceWithNEndpoints or countEndpoints are not working as expected + t.Errorf("Problem with test or test helper. Expected %d total endpoints, got %d", tc.expectedTotal, actualTotal) + } + + actualTotalReady := tc.si.getTotalReadyEndpoints() + if actualTotalReady != tc.expectedTotalReady { + t.Errorf("Expected %d, got %d", tc.expectedTotalReady, actualTotalReady) } }) } @@ -66,14 +87,32 @@ func TestGetTotalEndpoints(t *testing.T) { // helpers -func sliceWithNEndpoints(n int) *discovery.EndpointSlice { - endpoints := []discovery.Endpoint{} +func sliceWithNEndpoints(ready, unready int) *discovery.EndpointSlice { + endpoints := make([]discovery.Endpoint, ready+unready) - for i := 0; i < n; i++ { - endpoints = append(endpoints, discovery.Endpoint{Addresses: []string{fmt.Sprintf("10.1.2.%d", i)}}) + for i := 0; i < ready; i++ { + endpoints[i] = discovery.Endpoint{ + Addresses: []string{fmt.Sprintf("10.1.2.%d", i)}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, + } + } + + for i := 0; i < unready; i++ { + endpoints[ready+i] = discovery.Endpoint{ + Addresses: []string{fmt.Sprintf("10.1.2.%d", ready+i)}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(false)}, + } } return &discovery.EndpointSlice{ Endpoints: endpoints, } } + +func countEndpoints(slices []*discovery.EndpointSlice) int { + total := 0 + for _, slice := range slices { + total += len(slice.Endpoints) + } + return total +} diff --git a/pkg/controller/endpointslice/topologycache/topologycache.go b/pkg/controller/endpointslice/topologycache/topologycache.go index a286fd16011..b7c6feff42d 100644 --- a/pkg/controller/endpointslice/topologycache/topologycache.go +++ b/pkg/controller/endpointslice/topologycache/topologycache.go @@ -24,6 +24,7 @@ import ( discovery "k8s.io/api/discovery/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/klog/v2" + endpointsliceutil "k8s.io/kubernetes/pkg/controller/util/endpointslice" ) const ( @@ -84,7 +85,7 @@ func (t *TopologyCache) GetOverloadedServices() []string { // AddHints adds or updates topology hints on EndpointSlices and returns updated // lists of EndpointSlices to create and update. func (t *TopologyCache) AddHints(si *SliceInfo) ([]*discovery.EndpointSlice, []*discovery.EndpointSlice) { - totalEndpoints := si.getTotalEndpoints() + totalEndpoints := si.getTotalReadyEndpoints() allocations := t.getAllocations(totalEndpoints) if allocations == nil { @@ -103,6 +104,10 @@ func (t *TopologyCache) AddHints(si *SliceInfo) ([]*discovery.EndpointSlice, []* // step 1: assign same-zone hints for all endpoints as a starting point. for _, slice := range allocatableSlices { for i, endpoint := range slice.Endpoints { + if !endpointsliceutil.EndpointReady(endpoint) { + endpoint.Hints = nil + continue + } if endpoint.Zone == nil || *endpoint.Zone == "" { klog.InfoS("Endpoint found without zone specified, removing hints from service", "serviceKey", si.ServiceKey) t.RemoveHints(si.ServiceKey, si.AddressType) diff --git a/pkg/controller/endpointslice/topologycache/topologycache_test.go b/pkg/controller/endpointslice/topologycache/topologycache_test.go index febbdc37a71..a12dd97c40c 100644 --- a/pkg/controller/endpointslice/topologycache/topologycache_test.go +++ b/pkg/controller/endpointslice/topologycache/topologycache_test.go @@ -53,16 +53,18 @@ func TestAddHints(t *testing.T) { AddressType: discovery.AddressTypeIPv4, ToCreate: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.1.2.3"}, - Zone: utilpointer.StringPtr("zone-a"), + Addresses: []string{"10.1.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }}, }, expectedEndpointsByAddrType: nil, expectedSlicesToCreate: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.1.2.3"}, - Zone: utilpointer.StringPtr("zone-a"), + Addresses: []string{"10.1.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }}, expectedSlicesToUpdate: []*discovery.EndpointSlice{}, @@ -78,22 +80,26 @@ func TestAddHints(t *testing.T) { AddressType: discovery.AddressTypeIPv4, ToCreate: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.1.2.3"}, - Zone: utilpointer.StringPtr("zone-a"), + Addresses: []string{"10.1.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.1.2.4"}, - Zone: utilpointer.StringPtr("zone-b"), + Addresses: []string{"10.1.2.4"}, + Zone: utilpointer.StringPtr("zone-b"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }}, }, expectedEndpointsByAddrType: nil, expectedSlicesToCreate: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.1.2.3"}, - Zone: utilpointer.StringPtr("zone-a"), + Addresses: []string{"10.1.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.1.2.4"}, - Zone: utilpointer.StringPtr("zone-b"), + Addresses: []string{"10.1.2.4"}, + Zone: utilpointer.StringPtr("zone-b"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }}, expectedSlicesToUpdate: []*discovery.EndpointSlice{}, @@ -108,10 +114,60 @@ func TestAddHints(t *testing.T) { AddressType: discovery.AddressTypeIPv4, ToCreate: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.1.2.3"}, - Zone: utilpointer.StringPtr("zone-a"), + Addresses: []string{"10.1.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.1.2.4"}, + Addresses: []string{"10.1.2.4"}, + Zone: utilpointer.StringPtr("zone-b"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, + }}, + }}, + }, + expectedEndpointsByAddrType: map[discovery.AddressType]EndpointZoneInfo{ + discovery.AddressTypeIPv4: { + "zone-a": 1, + "zone-b": 1, + }, + }, + expectedSlicesToCreate: []*discovery.EndpointSlice{{ + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, + }, { + Addresses: []string{"10.1.2.4"}, + Zone: utilpointer.StringPtr("zone-b"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, + }}, + }}, + expectedSlicesToUpdate: []*discovery.EndpointSlice{}, + }, { + name: "slice to create with 2 ready, 1 unready, 1 unknown endpoints, zone ratios only require 2", + cpuRatiosByZone: map[string]float64{ + "zone-a": 0.45, + "zone-b": 0.55, + }, + sliceInfo: &SliceInfo{ + ServiceKey: "ns/svc", + AddressType: discovery.AddressTypeIPv4, + ToCreate: []*discovery.EndpointSlice{{ + Endpoints: []discovery.Endpoint{{ + Addresses: []string{"10.1.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, + }, { + Addresses: []string{"10.1.2.4"}, + Zone: utilpointer.StringPtr("zone-b"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, + }, { + Addresses: []string{"10.1.2.5"}, + Zone: utilpointer.StringPtr("zone-b"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(false)}, + }, { + Addresses: []string{"10.1.2.6"}, Zone: utilpointer.StringPtr("zone-b"), }}, }}, @@ -124,13 +180,24 @@ func TestAddHints(t *testing.T) { }, expectedSlicesToCreate: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.1.2.3"}, - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Addresses: []string{"10.1.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.1.2.4"}, + Addresses: []string{"10.1.2.4"}, + Zone: utilpointer.StringPtr("zone-b"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, + }, { + Addresses: []string{"10.1.2.5"}, + Zone: utilpointer.StringPtr("zone-b"), + Hints: nil, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(false)}, + }, { + Addresses: []string{"10.1.2.6"}, Zone: utilpointer.StringPtr("zone-b"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Hints: nil, }}, }}, expectedSlicesToUpdate: []*discovery.EndpointSlice{}, @@ -146,42 +213,52 @@ func TestAddHints(t *testing.T) { AddressType: discovery.AddressTypeIPv4, ToCreate: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.1.2.3"}, - Zone: utilpointer.StringPtr("zone-a"), + Addresses: []string{"10.1.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.1.2.4"}, - Zone: utilpointer.StringPtr("zone-b"), + Addresses: []string{"10.1.2.4"}, + Zone: utilpointer.StringPtr("zone-b"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }, { Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.1.3.3"}, - Zone: utilpointer.StringPtr("zone-c"), + Addresses: []string{"10.1.3.3"}, + Zone: utilpointer.StringPtr("zone-c"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.1.3.4"}, - Zone: utilpointer.StringPtr("zone-c"), + Addresses: []string{"10.1.3.4"}, + Zone: utilpointer.StringPtr("zone-c"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.1.3.4"}, - Zone: utilpointer.StringPtr("zone-a"), + Addresses: []string{"10.1.3.4"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }}, ToUpdate: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.2.2.3"}, - Zone: utilpointer.StringPtr("zone-a"), + Addresses: []string{"10.2.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.2.2.4"}, - Zone: utilpointer.StringPtr("zone-a"), + Addresses: []string{"10.2.2.4"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }, { Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.2.3.3"}, - Zone: utilpointer.StringPtr("zone-b"), + Addresses: []string{"10.2.3.3"}, + Zone: utilpointer.StringPtr("zone-b"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.2.3.4"}, - Zone: utilpointer.StringPtr("zone-c"), + Addresses: []string{"10.2.3.4"}, + Zone: utilpointer.StringPtr("zone-c"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.2.3.4"}, - Zone: utilpointer.StringPtr("zone-a"), + Addresses: []string{"10.2.3.4"}, + Zone: utilpointer.StringPtr("zone-a"), + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }}, }, @@ -194,52 +271,62 @@ func TestAddHints(t *testing.T) { }, expectedSlicesToCreate: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.1.2.3"}, - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Addresses: []string{"10.1.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.1.2.4"}, - Zone: utilpointer.StringPtr("zone-b"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Addresses: []string{"10.1.2.4"}, + Zone: utilpointer.StringPtr("zone-b"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }, { Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.1.3.3"}, - Zone: utilpointer.StringPtr("zone-c"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-c"}}}, + Addresses: []string{"10.1.3.3"}, + Zone: utilpointer.StringPtr("zone-c"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-c"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.1.3.4"}, - Zone: utilpointer.StringPtr("zone-c"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-c"}}}, + Addresses: []string{"10.1.3.4"}, + Zone: utilpointer.StringPtr("zone-c"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-c"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.1.3.4"}, - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Addresses: []string{"10.1.3.4"}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }}, expectedSlicesToUpdate: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.2.2.3"}, - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Addresses: []string{"10.2.2.3"}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.2.2.4"}, - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Addresses: []string{"10.2.2.4"}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }, { Endpoints: []discovery.Endpoint{{ - Addresses: []string{"10.2.3.3"}, - Zone: utilpointer.StringPtr("zone-b"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Addresses: []string{"10.2.3.3"}, + Zone: utilpointer.StringPtr("zone-b"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.2.3.4"}, - Zone: utilpointer.StringPtr("zone-c"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-c"}}}, + Addresses: []string{"10.2.3.4"}, + Zone: utilpointer.StringPtr("zone-c"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-c"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Addresses: []string{"10.2.3.4"}, - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Addresses: []string{"10.2.3.4"}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }}, }} diff --git a/pkg/controller/endpointslice/topologycache/utils.go b/pkg/controller/endpointslice/topologycache/utils.go index cb9dc3cee91..e650e7d7dd8 100644 --- a/pkg/controller/endpointslice/topologycache/utils.go +++ b/pkg/controller/endpointslice/topologycache/utils.go @@ -19,9 +19,10 @@ package topologycache import ( "math" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" discovery "k8s.io/api/discovery/v1" "k8s.io/klog/v2" + endpointsliceutil "k8s.io/kubernetes/pkg/controller/util/endpointslice" ) // RemoveHintsFromSlices removes topology hints on EndpointSlices and returns @@ -75,6 +76,10 @@ func redistributeHints(slices []*discovery.EndpointSlice, givingZones, receiving for _, slice := range slices { for i, endpoint := range slice.Endpoints { + if !endpointsliceutil.EndpointReady(endpoint) { + endpoint.Hints = nil + continue + } if len(givingZones) == 0 || len(receivingZones) == 0 { return redistributions } @@ -185,6 +190,9 @@ func getMost(zones map[string]float64) (string, float64) { func getHintsByZone(slice *discovery.EndpointSlice, allocatedHintsByZone EndpointZoneInfo, allocations map[string]Allocation) map[string]int { hintsByZone := map[string]int{} for _, endpoint := range slice.Endpoints { + if !endpointsliceutil.EndpointReady(endpoint) { + continue + } if endpoint.Hints == nil || len(endpoint.Hints.ForZones) == 0 { return nil } @@ -248,3 +256,15 @@ func NodeReady(nodeStatus v1.NodeStatus) bool { } return false } + +// numReadyEndpoints returns the number of Endpoints that are ready from the +// specified list. +func numReadyEndpoints(endpoints []discovery.Endpoint) int { + var total int + for _, ep := range endpoints { + if ep.Conditions.Ready != nil && *ep.Conditions.Ready { + total++ + } + } + return total +} diff --git a/pkg/controller/endpointslice/topologycache/utils_test.go b/pkg/controller/endpointslice/topologycache/utils_test.go index 8e911369ee9..10b092b8580 100644 --- a/pkg/controller/endpointslice/topologycache/utils_test.go +++ b/pkg/controller/endpointslice/topologycache/utils_test.go @@ -41,8 +41,9 @@ func Test_redistributeHints(t *testing.T) { name: "single endpoint", slices: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }}, givingZones: map[string]int{"zone-a": 1}, @@ -52,14 +53,17 @@ func Test_redistributeHints(t *testing.T) { name: "endpoints from 1 zone redistributed to 2 other zones", slices: []*discovery.EndpointSlice{{ Endpoints: []discovery.Endpoint{{ - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }}, }}, givingZones: map[string]int{"zone-a": 2}, @@ -211,6 +215,30 @@ func Test_getHintsByZone(t *testing.T) { name: "single zone hint", slice: discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{{ + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, + }}, + }, + allocations: map[string]Allocation{ + "zone-a": {Maximum: 3}, + }, + allocatedHintsByZone: EndpointZoneInfo{"zone-a": 1}, + expectedHintsByZone: map[string]int{ + "zone-a": 1, + }, + }, { + name: "single zone hint with 1 unready endpoint and 1 unknown endpoint", + slice: discovery.EndpointSlice{ + Endpoints: []discovery.Endpoint{{ + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, + }, { + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(false)}, + }, { Zone: utilpointer.StringPtr("zone-a"), Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, }}, @@ -227,16 +255,19 @@ func Test_getHintsByZone(t *testing.T) { slice: discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ { - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Zone: utilpointer.StringPtr("zone-b"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Zone: utilpointer.StringPtr("zone-b"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-b"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, }, }, @@ -255,8 +286,9 @@ func Test_getHintsByZone(t *testing.T) { slice: discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ { - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-non-existent"}}}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-non-existent"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, }, }, @@ -270,8 +302,9 @@ func Test_getHintsByZone(t *testing.T) { slice: discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ { - Zone: utilpointer.StringPtr("zone-a"), - Hints: nil, + Zone: utilpointer.StringPtr("zone-a"), + Hints: nil, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, }, }, @@ -285,8 +318,9 @@ func Test_getHintsByZone(t *testing.T) { slice: discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ { - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{}}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, }, }, @@ -300,12 +334,14 @@ func Test_getHintsByZone(t *testing.T) { slice: discovery.EndpointSlice{ Endpoints: []discovery.Endpoint{ { - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, { - Zone: utilpointer.StringPtr("zone-a"), - Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Zone: utilpointer.StringPtr("zone-a"), + Hints: &discovery.EndpointHints{ForZones: []discovery.ForZone{{Name: "zone-a"}}}, + Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, }, }, }, diff --git a/pkg/controller/util/endpointslice/utils.go b/pkg/controller/util/endpointslice/utils.go new file mode 100644 index 00000000000..3cc8b84e4bd --- /dev/null +++ b/pkg/controller/util/endpointslice/utils.go @@ -0,0 +1,27 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package endpointslice + +import ( + discovery "k8s.io/api/discovery/v1" +) + +// EndpointReady returns true if an Endpoint has the Ready condition set to +// true. +func EndpointReady(endpoint discovery.Endpoint) bool { + return endpoint.Conditions.Ready != nil && *endpoint.Conditions.Ready +}