Add TG garbage collector #580
Conversation
| defer func() { | ||
| tgGc.lock.Unlock() | ||
| }() | ||
| tgGc.lock.Lock() |
There was a problem hiding this comment.
Can you add some comments here for the reason we need to do Lock and Unlock?
pkg/deploy/stack_deployer.go
Outdated
| tgGcFn := NewTgGcFn(tgGcSynth) | ||
| tgGc = &TgGc{ | ||
| lock: sync.Mutex{}, | ||
| log: log.Named("gc"), |
There was a problem hiding this comment.
usually log names are lower case, can do "tg-gc" or "tg.gc"
There was a problem hiding this comment.
Would we ever garbage collect services? I think this would be the only GC
There was a problem hiding this comment.
I think we should not use GC at all. It will be a problem in future - global lock will cause contention and need carefully lock/unlock otherwise we will deadlock route reconciliations. This solution is a little pain relief from doing full TG clean up on each reconciliation loop.
We should have a better mechanism to identify TGs when we create brand new TG or when we replace old one and deletion is required. In that case we dont need locks, since problem will be properly partitioned by single-threaded owners - reconcilers.
| } | ||
| succ := 0 | ||
| for _, res := range results { | ||
| if res.Err == nil { |
There was a problem hiding this comment.
Do we need to print res.Err or do errors.Join(res.Err) and return it with TgGcResult{}?
There was a problem hiding this comment.
Not really, mostly errors happen when TG is in state that cannot be deleted yet. It pollutes logs.
There was a problem hiding this comment.
I think critical errors needs to be logged in SynthesizeUnusedDelete, or need to wrap them into some "NonOk" types and do type check here.
| svcBuilder := gateway.NewLatticeServiceBuilder(log, k8sClient, brTgBuilder) | ||
|
|
||
| tgGcOnce.Do(func() { | ||
| // TODO: need to refactor TG synthesizer. Remove stack from constructor |
There was a problem hiding this comment.
We should revisit synthesizers, the concept of abstract stack that we build and how it's actually implemented is questionable.
|
Overall LGTM, love the use of goroutines here. |
Note:
Address performance issue for unused target group cleanup. Implemented background garbage collector, that runs every X seconds. Might need to tune timers after.
e2e tests pass:
close #552