From 147114e64852319f2408a89afc2e68ed0fac6c25 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sun, 26 Nov 2023 16:25:21 -0500 Subject: [PATCH] Update some change tracker doc comments In particular, fix the description of ServiceChangeTracker.Update's return value, and point out that it's different from EndpointsChangeTracker.EndpointSliceUpdate's (though fortunately, in a way that doesn't matter for any existing code). --- pkg/proxy/endpointschangetracker.go | 27 ++++++++++++++----------- pkg/proxy/servicechangetracker.go | 31 +++++++++++++---------------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/pkg/proxy/endpointschangetracker.go b/pkg/proxy/endpointschangetracker.go index 3347e55709a..8b532759754 100644 --- a/pkg/proxy/endpointschangetracker.go +++ b/pkg/proxy/endpointschangetracker.go @@ -40,23 +40,25 @@ type EndpointsChangeTracker struct { // lock protects lastChangeTriggerTimes lock sync.Mutex + // processEndpointsMapChange is invoked by the apply function on every change. + // This function should not modify the EndpointsMaps, but just use the changes for + // any Proxier-specific cleanup. processEndpointsMapChange processEndpointsMapChangeFunc + // endpointSliceCache holds a simplified version of endpoint slices. endpointSliceCache *EndpointSliceCache - // Map from the Endpoints namespaced-name to the times of the triggers that caused the endpoints - // object to change. Used to calculate the network-programming-latency. + + // lastChangeTriggerTimes maps from the Service's NamespacedName to the times of + // the triggers that caused its EndpointSlice objects to change. Used to calculate + // the network-programming-latency metric. lastChangeTriggerTimes map[types.NamespacedName][]time.Time - // record the time when the endpointsChangeTracker was created so we can ignore the endpoints - // that were generated before, because we can't estimate the network-programming-latency on those. - // This is specially problematic on restarts, because we process all the endpoints that may have been - // created hours or days before. + // trackerStartTime is the time when the EndpointsChangeTracker was created, so + // we can avoid generating network-programming-latency metrics for changes that + // occurred before that. trackerStartTime time.Time } type makeEndpointFunc func(info *BaseEndpointInfo, svcPortName *ServicePortName) Endpoint - -// This handler is invoked by the apply function on every change. This function should not modify the -// EndpointsMap's but just use the changes for any Proxier specific cleanup. type processEndpointsMapChangeFunc func(oldEndpointsMap, newEndpointsMap EndpointsMap) // NewEndpointsChangeTracker initializes an EndpointsChangeTracker @@ -69,9 +71,10 @@ func NewEndpointsChangeTracker(hostname string, makeEndpointInfo makeEndpointFun } } -// EndpointSliceUpdate updates given service's endpoints change map based on the endpoints pair. -// It returns true if items changed, otherwise return false. Will add/update/delete items of EndpointsChangeTracker. -// If removeSlice is true, slice will be removed, otherwise it will be added or updated. +// EndpointSliceUpdate updates the EndpointsChangeTracker by adding/updating or removing +// endpointSlice (depending on removeSlice). It returns true if this update contained a +// change that needs to be synced; note that this is different from the return value of +// ServiceChangeTracker.Update(). func (ect *EndpointsChangeTracker) EndpointSliceUpdate(endpointSlice *discovery.EndpointSlice, removeSlice bool) bool { if !supportedEndpointSliceAddressTypes.Has(string(endpointSlice.AddressType)) { klog.V(4).InfoS("EndpointSlice address type not supported by kube-proxy", "addressType", endpointSlice.AddressType) diff --git a/pkg/proxy/servicechangetracker.go b/pkg/proxy/servicechangetracker.go index 37d22357262..fb5fd4de406 100644 --- a/pkg/proxy/servicechangetracker.go +++ b/pkg/proxy/servicechangetracker.go @@ -36,18 +36,20 @@ type ServiceChangeTracker struct { lock sync.Mutex // items maps a service to its serviceChange. items map[types.NamespacedName]*serviceChange - // makeServiceInfo allows proxier to inject customized information when processing service. - makeServiceInfo makeServicePortFunc - processServiceMapChange processServiceMapChangeFunc - ipFamily v1.IPFamily + // makeServiceInfo allows the proxier to inject customized information when + // processing services. + makeServiceInfo makeServicePortFunc + // processServiceMapChange is invoked by the apply function on every change. This + // function should not modify the ServicePortMaps, but just use the changes for + // any Proxier-specific cleanup. + processServiceMapChange processServiceMapChangeFunc + + ipFamily v1.IPFamily recorder events.EventRecorder } type makeServicePortFunc func(*v1.ServicePort, *v1.Service, *BaseServicePortInfo) ServicePort - -// This handler is invoked by the apply function on every change. This function should not modify the -// ServicePortMap's but just use the changes for any Proxier specific cleanup. type processServiceMapChangeFunc func(previous, current ServicePortMap) // serviceChange contains all changes to services that happened since proxy rules were synced. For a single object, @@ -69,16 +71,11 @@ func NewServiceChangeTracker(makeServiceInfo makeServicePortFunc, ipFamily v1.IP } } -// Update updates given service's change map based on the service pair. It returns true if items changed, -// otherwise return false. Update can be used to add/update/delete items of ServiceChangeMap. For example, -// Add item -// - pass as the pair. -// -// Update item -// - pass as the pair. -// -// Delete item -// - pass as the pair. +// Update updates the ServiceChangeTracker based on the service pair +// (where either previous or current, but not both, can be nil). It returns true if sct +// contains changes that need to be synced (whether or not those changes were caused by +// this update); note that this is different from the return value of +// EndpointChangeTracker.EndpointSliceUpdate(). func (sct *ServiceChangeTracker) Update(previous, current *v1.Service) bool { // This is unexpected, we should return false directly. if previous == nil && current == nil {