diff --git a/pkg/endpoints/endpoints_bgp.go b/pkg/endpoints/endpoints_bgp.go index 47abbe67..b68123ef 100644 --- a/pkg/endpoints/endpoints_bgp.go +++ b/pkg/endpoints/endpoints_bgp.go @@ -66,9 +66,7 @@ func (b *BGP) clear(svcCtx *servicecontext.Context, lastKnownGoodEndpoint *strin b.clearEgress(lastKnownGoodEndpoint, service) - if svcCtx.LeaderCancel != nil { - svcCtx.LeaderCancel() - } + svcCtx.CallLeaderCancel() } func (b *BGP) getEndpoints(service *v1.Service, id string) ([]string, error) { diff --git a/pkg/endpoints/endpoints_generic.go b/pkg/endpoints/endpoints_generic.go index 096907a0..b5d05eaa 100644 --- a/pkg/endpoints/endpoints_generic.go +++ b/pkg/endpoints/endpoints_generic.go @@ -64,9 +64,7 @@ func (g *generic) processInstance(_ *servicecontext.Context, _ *v1.Service) erro func (g *generic) clear(svcCtx *servicecontext.Context, lastKnownGoodEndpoint *string, service *v1.Service) { g.clearEgress(lastKnownGoodEndpoint, service) - if svcCtx.LeaderCancel != nil { - svcCtx.LeaderCancel() - } + svcCtx.CallLeaderCancel() } func (g *generic) clearEgress(lastKnownGoodEndpoint *string, service *v1.Service) { diff --git a/pkg/endpoints/endpoints_routing_table.go b/pkg/endpoints/endpoints_routing_table.go index ca02f087..e021ef64 100644 --- a/pkg/endpoints/endpoints_routing_table.go +++ b/pkg/endpoints/endpoints_routing_table.go @@ -66,9 +66,7 @@ func (rt *RoutingTable) clear(svcCtx *servicecontext.Context, lastKnownGoodEndpo rt.clearEgress(lastKnownGoodEndpoint, service) - if svcCtx.LeaderCancel != nil { - svcCtx.LeaderCancel() - } + svcCtx.CallLeaderCancel() } func (rt *RoutingTable) getEndpoints(service *v1.Service, id string) ([]string, error) { diff --git a/pkg/endpoints/endpoints_wireguard.go b/pkg/endpoints/endpoints_wireguard.go index 431ab896..c5d3eed6 100644 --- a/pkg/endpoints/endpoints_wireguard.go +++ b/pkg/endpoints/endpoints_wireguard.go @@ -212,8 +212,8 @@ func (w *wireguardWorker) clear(svcCtx *servicecontext.Context, lastKnownGoodEnd } } - if svcCtx != nil && svcCtx.LeaderCancel != nil { - svcCtx.LeaderCancel() + if svcCtx != nil { + svcCtx.CallLeaderCancel() } } diff --git a/pkg/servicecontext/servicecontext.go b/pkg/servicecontext/servicecontext.go index 1e63b7f0..f2d1b039 100644 --- a/pkg/servicecontext/servicecontext.go +++ b/pkg/servicecontext/servicecontext.go @@ -12,6 +12,7 @@ type Context struct { IsWatched bool ConfiguredNetworks sync.Map EndpointsReady chan any + mu sync.Mutex epReady sync.Once leaderElection sync.Once Signalled atomic.Bool @@ -51,6 +52,9 @@ func (ctx *Context) StartLeaderElectionOnce(f func()) { } func (ctx *Context) SignalReadiness() { + ctx.mu.Lock() + defer ctx.mu.Unlock() + ctx.epReady.Do(func() { close(ctx.EndpointsReady) ctx.Signalled.Store(true) @@ -58,9 +62,50 @@ func (ctx *Context) SignalReadiness() { } func (ctx *Context) ResetReadiness() { + ctx.mu.Lock() + defer ctx.mu.Unlock() + if ctx.Signalled.Load() { ctx.EndpointsReady = make(chan any) ctx.epReady = sync.Once{} ctx.Signalled.Store(false) } } + +func (ctx *Context) GetEndpointsReady() chan any { + ctx.mu.Lock() + defer ctx.mu.Unlock() + + return ctx.EndpointsReady +} + +func (ctx *Context) SetLeaderCancel(cancel context.CancelFunc) { + ctx.mu.Lock() + defer ctx.mu.Unlock() + + ctx.LeaderCancel = cancel +} + +func (ctx *Context) CallLeaderCancel() { + ctx.mu.Lock() + cancel := ctx.LeaderCancel + ctx.mu.Unlock() + + if cancel != nil { + cancel() + } +} + +func (ctx *Context) SetWatched(watched bool) { + ctx.mu.Lock() + defer ctx.mu.Unlock() + + ctx.IsWatched = watched +} + +func (ctx *Context) IsWatchedLocked() bool { + ctx.mu.Lock() + defer ctx.mu.Unlock() + + return ctx.IsWatched +} diff --git a/pkg/servicecontext/servicecontext_race_test.go b/pkg/servicecontext/servicecontext_race_test.go new file mode 100644 index 00000000..1d264e26 --- /dev/null +++ b/pkg/servicecontext/servicecontext_race_test.go @@ -0,0 +1,56 @@ +package servicecontext + +import ( + "context" + "sync" + "testing" +) + +func TestReadinessResetConcurrentWithSignal(t *testing.T) { + ctx := New(context.Background()) + start := make(chan struct{}) + var wg sync.WaitGroup + + wg.Go(func() { + <-start + for range 1000 { + ctx.SignalReadiness() + ctx.ResetReadiness() + } + }) + wg.Go(func() { + <-start + for range 1000 { + ready := ctx.GetEndpointsReady() + select { + case <-ready: + default: + } + } + }) + + close(start) + wg.Wait() +} + +func TestLeaderCancelConcurrentWithEndpointCleanup(t *testing.T) { + ctx := New(context.Background()) + start := make(chan struct{}) + var wg sync.WaitGroup + + wg.Go(func() { + <-start + for range 1000 { + ctx.SetLeaderCancel(func() {}) + } + }) + wg.Go(func() { + <-start + for range 1000 { + ctx.CallLeaderCancel() + } + }) + + close(start) + wg.Wait() +} diff --git a/pkg/services/leader.go b/pkg/services/leader.go index 404f7025..a11a690d 100644 --- a/pkg/services/leader.go +++ b/pkg/services/leader.go @@ -99,7 +99,7 @@ func (p *Processor) StartServicesLeaderElection(svcCtx *servicecontext.Context, return fmt.Errorf("service context cancelled before election start: %w", svcCtx.Ctx.Err()) case <-svcLease.Ctx.Done(): return fmt.Errorf("lease context cancelled before election start: %w", svcLease.Ctx.Err()) - case <-svcCtx.EndpointsReady: + case <-svcCtx.GetEndpointsReady(): } // this service is sharing lease with another service @@ -135,7 +135,7 @@ func (p *Processor) StartServicesLeaderElection(svcCtx *servicecontext.Context, log.Info("new leader election", "service", service.Name, "namespace", service.Namespace, "lock_name", serviceLease, "host_id", p.config.NodeName) leaderCtx, leaderCancel := context.WithCancel(svcLease.Ctx) - svcCtx.LeaderCancel = leaderCancel + svcCtx.SetLeaderCancel(leaderCancel) run := election.RunConfig{ Config: p.config, diff --git a/pkg/services/processor.go b/pkg/services/processor.go index ba231f26..242ecfd0 100644 --- a/pkg/services/processor.go +++ b/pkg/services/processor.go @@ -223,13 +223,13 @@ func (p *Processor) AddOrModify(ctx context.Context, event watch.Event, serviceF } // this goroutine starts service handling function (with or without leaderelection) - if !svcCtx.IsWatched { + if !svcCtx.IsWatchedLocked() { wg.Go(func() { watchWg := sync.WaitGroup{} defer func() { // wait for the sub-goroutines and tag service as not watched watchWg.Wait() - svcCtx.IsWatched = false + svcCtx.SetWatched(false) }() watchWg.Go(func() { @@ -268,7 +268,7 @@ func (p *Processor) AddOrModify(ctx context.Context, event watch.Event, serviceF }) // tag service as watched - svcCtx.IsWatched = true + svcCtx.SetWatched(true) } if !p.config.EnableServicesElection { diff --git a/pkg/services/processor_race_test.go b/pkg/services/processor_race_test.go new file mode 100644 index 00000000..d497d007 --- /dev/null +++ b/pkg/services/processor_race_test.go @@ -0,0 +1,33 @@ +package services + +import ( + "context" + "sync" + "testing" + + "github.com/kube-vip/kube-vip/pkg/servicecontext" +) + +func TestWatchedFlagConcurrentWithWatcherTeardown(t *testing.T) { + svcCtx := servicecontext.New(context.Background()) + start := make(chan struct{}) + var wg sync.WaitGroup + + wg.Go(func() { + <-start + for range 1000 { + svcCtx.SetWatched(false) + } + }) + wg.Go(func() { + <-start + for range 1000 { + if !svcCtx.IsWatchedLocked() { + svcCtx.SetWatched(true) + } + } + }) + + close(start) + wg.Wait() +} diff --git a/pkg/services/services.go b/pkg/services/services.go index 7dfaa2e2..2ee93531 100644 --- a/pkg/services/services.go +++ b/pkg/services/services.go @@ -65,7 +65,7 @@ func (p *Processor) SyncServices(ctx *servicecontext.Context, svc *v1.Service, w select { case <-ctx.Ctx.Done(): return nil - case <-ctx.EndpointsReady: + case <-ctx.GetEndpointsReady(): } }