chore: rework Reconciler to use proper http.Transport
#929
Conversation
DmitriyMV
added
the
integration/e2e-workload-proxy
| logger: logger, | ||
| lbLogger: logger.WithOptions(zap.IncreaseLevel(zapcore.ErrorLevel)), | ||
| logLevel: logLevel, | ||
| trans := &http.Transport{ |
There was a problem hiding this comment.
let's use https://github.com/hashicorp/go-cleanhttp instead of hand-rolling it
| ExpectContinueTimeout: 1 * time.Second, | ||
| } | ||
|
|
||
| if err := http2.ConfigureTransport(trans); err != nil { |
There was a problem hiding this comment.
as we dial without TLS, I'm not sure it has much value
| trans := &http.Transport{ | ||
| Proxy: http.ProxyFromEnvironment, | ||
| ForceAttemptHTTP2: true, | ||
| MaxIdleConns: 100, |
There was a problem hiding this comment.
we need to consider idle connections in a specific way, as the host would be service name
There was a problem hiding this comment.
there's also stuff like MaxConnsPerHost
| return proxy, clusterID, nil | ||
| // hostPortForAlias returns a unique IP:port for the given cluster and alias. | ||
| func hostPortForAlias(clusterID resource.ID, alias string) string { | ||
| return fmt.Sprintf("%s_%s:4242", clusterID, alias) |
There was a problem hiding this comment.
do we have some kind of guarantee that _ can't be in the clusterID ?
There was a problem hiding this comment.
That's a good question. Maybe pick some other (perhaps unicode) symbol? But there is no guarantee that it will not be picked in the future either.There is one in ClusterDiscoveryConfig.Validate and one in Config.Validate checks in Talos, but both only ensure that ClusterID is not empty. As for Omni, I'm currently try to dig usages.
There was a problem hiding this comment.
if we pick a very obscure character, we can also disallow it in the validations.
| } | ||
|
|
||
| func (b *backend) HealthCheck(context.Context) (upstream.Tier, error) { return 0, nil } |
There was a problem hiding this comment.
that should be a Dial check actually ?
|
|
||
| return startErr | ||
| } | ||
| func (registry *Reconciler) removeLB(cluster resource.ID, alias string) { |
There was a problem hiding this comment.
we should call this on some "idle" timeout - that is if we don't have DialContext for the cluster for some timeout, we can shut it down
DmitriyMV
force-pushed
the
reconciler
branch
6 times, most recently
from
06b6aae to
754b393
Compare
DmitriyMV
force-pushed
the
reconciler
branch
4 times, most recently
from
75c07ff to
e752ce3
Compare
- [x] Ensure `Reconciler` is internally consistent on all variations of `Reconcile` call (including parallel). Track aliases and clusters side by side. - [x] Add tests for the above. - [x] Replace HealthCheck logic with the actual tcp probing. - [ ] Replace probing port on alias removal. That is - if we lost an alias to the probing port, find a new one and use it. - [ ] Find a way to test the dialer logic. Pass the cache as DI? Or add probes factory? - [ ] Ensure that active probes are actually deleted on time expiration. - [?] On setting upstream.List[T] ensure that "down" clusters are added with the negative score so we don't see those on dial attempt. (Looks like it's already the case). - [ ] Expose metrics. Specifically for `idleConnections` and `inuse` connections. Register those in prometheus. - [ ] Use latency checker from standard prometheus exporter. GotConn and WriteReq are important. GotResponse maybe interesting for their workload. Count can be used for idleConnection because in-flight requests and in-flight connections. For siderolabs#886 Signed-off-by: Dmitriy Matrenichev <[email protected]>
DmitriyMV
force-pushed
the
reconciler
branch
from
e752ce3 to
ee5db60
Compare
chore: rework
Reconcilerto use properhttp.TransportReconcileris internally consistent on all variations ofReconcilecall (including parallel). Track aliases and clustersside by side.
idleConnectionsandinuseconnections. Register those in prometheus.For #886
Signed-off-by: Dmitriy Matrenichev [email protected]