cache event fanout & reversetunnel improvements #3536
Conversation
fspmarshall
requested review from
awly,
klizhentas,
russjones and
webvictim
as code owners
fspmarshall
force-pushed
the
fspmarshall/iot-debug-master
branch
from
1d99284 to
9ecf9a5
Compare
|
https://github.com/golang/go/wiki/CodeReviewComments#interfaces |
| if err != nil && err != io.EOF { | ||
| p.log.Warnf("Proxy transport failed: %v %T.", trace.DebugReport(err), err) | ||
| } | ||
| case <-errorCh: |
There was a problem hiding this comment.
Looks like before we were reporting errors other than io.EOF. What errors were you seeing here other than io.EOF that were not relevant?
There was a problem hiding this comment.
Removed because this was just filling the logs with errors related to "use of closed network connection".
| if count < 1 { | ||
| count = 1 | ||
| } | ||
| p.wp.Set(key, uint64(count)) |
There was a problem hiding this comment.
Won't the count be picked up in the tick function and won't it be more accurate because it will have reduce count by the number of proxies that are expired?
There was a problem hiding this comment.
Expiry is checked on a pretty long interval (30 seconds, though it is configurable). If we only updated counts at that point, the AgentPool would be very slow to react to new proxies being available (also, tests would take significantly longer).
fspmarshall
force-pushed
the
fspmarshall/iot-debug-master
branch
from
d0b0082 to
0e80232
Compare
There was a problem hiding this comment.
@fspmarshall approved with minor nitpicks left here and there, please proceed with backport and ping me so I can load test.
fspmarshall
force-pushed
the
fspmarshall/iot-debug-master
branch
3 times, most recently
from
ecf227c to
5384883
Compare
| "github.com/gravitational/teleport/lib/utils/workpool" | ||
| ) | ||
|
|
||
| type Lease = workpool.Lease |
There was a problem hiding this comment.
why use a type alias here?
if it's just to shorten the code below, remove it
There was a problem hiding this comment.
The Lease type is the only part of the workpool API which isn't wrapped. Alias unifies the API and eliminates the need for consumer to import the workpool package, which is otherwise an implementation detail.
| // SetDefaults set default values for Config. | ||
| func (c *Config) SetDefaults() { | ||
| if c.ProxyExpiry < 1 { | ||
| c.ProxyExpiry = 3 * time.Minute | ||
| } | ||
| if c.TickRate < 1 { | ||
| c.TickRate = 30 * time.Second | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement this as a constructor func NewConfig
There was a problem hiding this comment.
Usable zero value with defaults set at time of usage is more consistent with how configuration structs are handled elsewhere in Teleport. Not 100% certain how the convention got started, but it helps with deserialization and with this handy pattern:
thing := NewThing(ThingConfig {
Foo: "bar",
// ...
})| if strings.HasSuffix(name, p.key.Cluster) { | ||
| t := strings.TrimSuffix(name, p.key.Cluster) | ||
| if strings.HasSuffix(t, ".") { | ||
| name = strings.TrimSuffix(t, ".") | ||
| } | ||
| } |
There was a problem hiding this comment.
How about:
name = strings.TrimSuffix(name, fmt.Sprintf(".%s", p.key.Cluster))it will be a noop if there's no such suffix
There was a problem hiding this comment.
Trimming Cluster and "." separately avoids unnecessary string allocation.
fspmarshall
force-pushed
the
fspmarshall/iot-debug-master
branch
from
6d78612 to
188e22c
Compare
- cache now perforams in-memory fanout of events, eliminating spurious event generation due to cache init/reset. - removed old unused logic from reversetunnel agents. - replaced seekpool with simpler ttl-cache and semaphore-like lease system. - add jittered backoff to agent connection attempts to reduce "thundering herd" effect. - improved reversetunnel logging. - improved LB usage in tests.
fspmarshall
force-pushed
the
fspmarshall/iot-debug-master
branch
from
188e22c to
de246cf
Compare
The PR includes multiple fixes intended to improve reliability of the caching and reversetunnel systems:
cache now perforams in-memory fanout of events, eliminating
spurious event generation due to cache init/reset.
removed old unused logic from reversetunnel agents.
seekpool now directly controls active agent count via newsemaphore-like lease system.
seekpool functionality simplified and split into two separate components; a "workpool" construct which functions like a per-cluster semaphore with variable targets, and a "tracker" which manages a record of which proxies might exist and which proxies are currently held by active agents.
add per-cluster jittered backoff to agent connection attempts toreduce "thundering herd" effect.
removed per-proxy and per-cluster backoff constructs in favor of a single jittered backoff at the AgentPool level (easier to maintain and test).
improved reversetunnel logging.
improved LB usage in tests.
Fixes #3468