From 9513f75089fc3f708126134b97814beb3589693d Mon Sep 17 00:00:00 2001 From: Gaurav Ghildiyal Date: Fri, 23 Feb 2024 19:24:50 -0800 Subject: [PATCH] Add trafficdist package for handling reconciliation of new field --- .../endpointslice/trafficdist/trafficdist.go | 143 ++++++ .../trafficdist/trafficdist_test.go | 470 ++++++++++++++++++ 2 files changed, 613 insertions(+) create mode 100644 staging/src/k8s.io/endpointslice/trafficdist/trafficdist.go create mode 100644 staging/src/k8s.io/endpointslice/trafficdist/trafficdist_test.go diff --git a/staging/src/k8s.io/endpointslice/trafficdist/trafficdist.go b/staging/src/k8s.io/endpointslice/trafficdist/trafficdist.go new file mode 100644 index 00000000000..cf6c7b42c6a --- /dev/null +++ b/staging/src/k8s.io/endpointslice/trafficdist/trafficdist.go @@ -0,0 +1,143 @@ +/* +Copyright 2024 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. +*/ + +// trafficdist handles reconciliation of hints for trafficDistribution field. +package trafficdist + +import ( + corev1 "k8s.io/api/core/v1" + discoveryv1 "k8s.io/api/discovery/v1" +) + +// ReconcileHints will reconcile hints for the given EndpointSlices. +// +// EndpointSlice resources within slicesUnchanged will not be modified. +func ReconcileHints(trafficDistribution *string, slicesToCreate, slicesToUpdate, slicesUnchanged []*discoveryv1.EndpointSlice) ([]*discoveryv1.EndpointSlice, []*discoveryv1.EndpointSlice, []*discoveryv1.EndpointSlice) { + var h heuristic = &defaultHeuristic{} + + if trafficDistribution != nil && *trafficDistribution == corev1.ServiceTrafficDistributionPreferClose { + h = &preferCloseHeuristic{} + } + + // Identify the Unchanged slices that need an update because of missing or + // incorrect zone hint. + // + // Uses filtering in place to remove any endpoints that are no longer + // unchanged and need to be moved to slicesToUpdate + // (https://github.com/golang/go/wiki/SliceTricks#filter-in-place) + j := 0 + for _, slice := range slicesUnchanged { + if h.needsUpdate(slice) { + // Unchanged slices are direct copies from informer cache. We need to deep + // copy an unchanged slice before making any modifications to it so that we do + // not modify the slice within the informer cache. + slicesToUpdate = append(slicesToUpdate, slice.DeepCopy()) + } else { + slicesUnchanged[j] = slice + j++ + } + } + // Truncate slicesUnchanged so it only includes slices that are still + // unchanged. + slicesUnchanged = slicesUnchanged[:j] + + // Add zone hints to all slices that need to be created or updated. + for _, slice := range slicesToCreate { + h.update(slice) + } + for _, slice := range slicesToUpdate { + h.update(slice) + } + + return slicesToCreate, slicesToUpdate, slicesUnchanged +} + +type heuristic interface { + needsUpdate(*discoveryv1.EndpointSlice) bool + update(*discoveryv1.EndpointSlice) +} + +// endpointReady returns true if an Endpoint has the Ready condition set to +// true. +func endpointReady(endpoint discoveryv1.Endpoint) bool { + return endpoint.Conditions.Ready != nil && *endpoint.Conditions.Ready +} + +// defaultHeuristic means cluster wide routing, hence it will remove any hints +// present in the EndpointSlice. +type defaultHeuristic struct { +} + +// needsUpdate returns true if any endpoint in the slice has a zone hint. +func (defaultHeuristic) needsUpdate(slice *discoveryv1.EndpointSlice) bool { + if slice == nil { + return false + } + for _, endpoint := range slice.Endpoints { + if endpoint.Hints != nil { + return true + } + } + return false +} + +// update removes zone hints from all endpoints. +func (defaultHeuristic) update(slice *discoveryv1.EndpointSlice) { + for i := range slice.Endpoints { + slice.Endpoints[i].Hints = nil + } +} + +// preferCloseHeuristic adds +type preferCloseHeuristic struct { +} + +// needsUpdate returns true if any ready endpoint in the slice has a +// missing or incorrect hint. +func (preferCloseHeuristic) needsUpdate(slice *discoveryv1.EndpointSlice) bool { + if slice == nil { + return false + } + for _, endpoint := range slice.Endpoints { + if !endpointReady(endpoint) { + continue + } + var zone string + if endpoint.Zone != nil { + zone = *endpoint.Zone + } + + if endpoint.Hints == nil || len(endpoint.Hints.ForZones) != 1 || endpoint.Hints.ForZones[0].Name != zone { + return true + } + } + return false +} + +// update adds a same zone topology hint for all ready endpoints +func (preferCloseHeuristic) update(slice *discoveryv1.EndpointSlice) { + for i, endpoint := range slice.Endpoints { + if !endpointReady(endpoint) { + continue + } + + var zone string + if endpoint.Zone != nil { + zone = *endpoint.Zone + } + slice.Endpoints[i].Hints = &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: zone}}} + } +} diff --git a/staging/src/k8s.io/endpointslice/trafficdist/trafficdist_test.go b/staging/src/k8s.io/endpointslice/trafficdist/trafficdist_test.go new file mode 100644 index 00000000000..9e3cd950368 --- /dev/null +++ b/staging/src/k8s.io/endpointslice/trafficdist/trafficdist_test.go @@ -0,0 +1,470 @@ +/* +Copyright 2024 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 trafficdist + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + corev1 "k8s.io/api/core/v1" + discoveryv1 "k8s.io/api/discovery/v1" + "k8s.io/utils/ptr" +) + +func TestReconcileHints_trafficDistribution_is_PreferClose(t *testing.T) { + testCases := []struct { + name string + + trafficDistribution *string + slicesToCreate []*discoveryv1.EndpointSlice + slicesToUpdate []*discoveryv1.EndpointSlice + slicesUnchanged []*discoveryv1.EndpointSlice + + wantSlicesToCreate []*discoveryv1.EndpointSlice + wantSlicesToUpdate []*discoveryv1.EndpointSlice + wantSlicesUnchanged []*discoveryv1.EndpointSlice + }{ + { + name: "should set same zone hints", + trafficDistribution: ptrTo(corev1.ServiceTrafficDistributionPreferClose), + slicesToCreate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.1"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + { + Addresses: []string{"10.0.0.2"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + }, + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.3"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + { + Addresses: []string{"10.0.0.4"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + }, + }, + slicesToUpdate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.5"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + { + Addresses: []string{"10.0.0.6"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + }, + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.7"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + { + Addresses: []string{"10.0.0.8"}, + Zone: ptr.To("zone-c"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + }, + }, + wantSlicesToCreate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.1"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-a"}}}, + }, + { + Addresses: []string{"10.0.0.2"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-b"}}}, + }, + }, + }, + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.3"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-a"}}}, + }, + { + Addresses: []string{"10.0.0.4"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-b"}}}, + }, + }, + }, + }, + wantSlicesToUpdate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.5"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-a"}}}, + }, + { + Addresses: []string{"10.0.0.6"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-a"}}}, + }, + }, + }, + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.7"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-b"}}}, + }, + { + Addresses: []string{"10.0.0.8"}, + Zone: ptr.To("zone-c"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-c"}}}, + }, + }, + }, + }, + }, + { + name: "incorrect hints should be corrected", + trafficDistribution: ptrTo(corev1.ServiceTrafficDistributionPreferClose), + slicesToUpdate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.1"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-b"}}}, // incorrect hint as per new heuristic + }, + }, + }, + }, + slicesUnchanged: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.2"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-c"}}}, + }, + }, + }, + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.3"}, + Zone: ptr.To("zone-c"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + }, + }, + wantSlicesToUpdate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.1"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-a"}}}, + }, + }, + }, + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.2"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-b"}}}, + }, + }, + }, + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.3"}, + Zone: ptr.To("zone-c"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-c"}}}, + }, + }, + }, + }, + }, + { + name: "unready endpoints should not trigger updates", + trafficDistribution: ptrTo(corev1.ServiceTrafficDistributionPreferClose), + slicesUnchanged: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.2"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(false)}, // endpoint is not ready + }, + }, + }, + }, + wantSlicesUnchanged: []*discoveryv1.EndpointSlice{ // ... so there should be no updates + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.2"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(false)}, + }, + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gotSlicesToCreate, gotSlicesToUpdate, gotSlicesUnchanged := ReconcileHints(tc.trafficDistribution, tc.slicesToCreate, tc.slicesToUpdate, tc.slicesUnchanged) + + if diff := cmp.Diff(tc.wantSlicesToCreate, gotSlicesToCreate, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("ReconcileHints(...) returned unexpected diff in 'slicesToCreate': (-want, +got)\n%v", diff) + } + if diff := cmp.Diff(tc.wantSlicesToUpdate, gotSlicesToUpdate, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("ReconcileHints(...) returned unexpected diff in 'slicesToUpdate': (-want, +got)\n%v", diff) + } + if diff := cmp.Diff(tc.wantSlicesUnchanged, gotSlicesUnchanged, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("ReconcileHints(...) returned unexpected diff in 'slicesUnchanged': (-want, +got)\n%v", diff) + } + }) + } +} + +func TestReconcileHints_trafficDistribution_is_nil_or_empty(t *testing.T) { + testCases := []struct { + name string + + trafficDistribution *string + slicesToCreate []*discoveryv1.EndpointSlice + slicesToUpdate []*discoveryv1.EndpointSlice + slicesUnchanged []*discoveryv1.EndpointSlice + + wantSlicesToCreate []*discoveryv1.EndpointSlice + wantSlicesToUpdate []*discoveryv1.EndpointSlice + wantSlicesUnchanged []*discoveryv1.EndpointSlice + }{ + { + name: "trafficDistribution='' should remove zone hints", + trafficDistribution: ptrTo(""), + slicesToCreate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.1"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-a"}}}, + }, + { + Addresses: []string{"10.0.0.2"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-b"}}}, + }, + }, + }, + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.3"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-a"}}}, + }, + }, + }, + }, + slicesToUpdate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.5"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-a"}}}, + }, + }, + }, + }, + wantSlicesToCreate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.1"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + { + Addresses: []string{"10.0.0.2"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + }, + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.3"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + }, + }, + wantSlicesToUpdate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.5"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + }, + }, + }, + { + name: "trafficDistribution=nil should remove zone hints", + trafficDistribution: nil, + slicesToUpdate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.5"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-a"}}}, + }, + }, + }, + }, + slicesUnchanged: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.6"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + Hints: &discoveryv1.EndpointHints{ForZones: []discoveryv1.ForZone{{Name: "zone-b"}}}, + }, + }, + }, + }, + wantSlicesToUpdate: []*discoveryv1.EndpointSlice{ + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.5"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + }, + { + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.6"}, + Zone: ptr.To("zone-b"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gotSlicesToCreate, gotSlicesToUpdate, gotSlicesUnchanged := ReconcileHints(tc.trafficDistribution, tc.slicesToCreate, tc.slicesToUpdate, tc.slicesUnchanged) + + if diff := cmp.Diff(tc.wantSlicesToCreate, gotSlicesToCreate, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("ReconcileHints(...) returned unexpected diff in 'slicesToCreate': (-want, +got)\n%v", diff) + } + if diff := cmp.Diff(tc.wantSlicesToUpdate, gotSlicesToUpdate, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("ReconcileHints(...) returned unexpected diff in 'slicesToUpdate': (-want, +got)\n%v", diff) + } + if diff := cmp.Diff(tc.wantSlicesUnchanged, gotSlicesUnchanged, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("ReconcileHints(...) returned unexpected diff in 'slicesUnchanged': (-want, +got)\n%v", diff) + } + }) + } +} + +// Ensure that the EndpointSlice objects within `slicesUnchanged` don't get modified. +func TestReconcileHints_doesNotMutateUnchangedSlices(t *testing.T) { + originalEps := &discoveryv1.EndpointSlice{ + Endpoints: []discoveryv1.Endpoint{ + { + Addresses: []string{"10.0.0.1"}, + Zone: ptr.To("zone-a"), + Conditions: discoveryv1.EndpointConditions{Ready: ptr.To(true)}, + }, + }, + } + clonedEps := originalEps.DeepCopy() + + // originalEps should not get modified. + ReconcileHints(ptrTo(corev1.ServiceTrafficDistributionPreferClose), nil, nil, []*discoveryv1.EndpointSlice{originalEps}) + if diff := cmp.Diff(clonedEps, originalEps); diff != "" { + t.Errorf("ReconcileHints(...) modified objects within slicesUnchanged, want objects within slicesUnchanged to remain unmodified: (-want, +got)\n%v", diff) + } +} + +func ptrTo[T any](obj T) *T { + return &obj +}