feat(config-cache): initial stub for cluster config cache service #3803
Conversation
karol-kokoszka
changed the base branch from
master
to
feature-branch_config_cache_service
karol-kokoszka
force-pushed
the
kk/3767-cache-nodeinfo-outside-healthcheck
branch
from
41e3c5b to
e14498f
Compare
karol-kokoszka
force-pushed
the
kk/3767-cache-nodeinfo-outside-healthcheck
branch
2 times, most recently
from
2154611 to
3247350
Compare
pkg/service/configcache/service.go
Outdated
| const ( | ||
| // CQL defines cql connection type. | ||
| CQL ConnectionType = iota | ||
| // Alternator defines alternator connection type. | ||
| Alternator | ||
|
|
||
| updateFrequency = 30 * time.Minute | ||
| ) |
There was a problem hiding this comment.
I think it would be nicer to separate enums and other consts by placing them in separate blocks.
What do you think?
There was a problem hiding this comment.
And what is the benefit of doing so over keeping them in one block split with empty line ?
There is actually no enum in Go, the ConnectionType is just a work around for this lack of enum.
Compiler would still accept
config.TLSConfig[5] = &tlsConfigWithAddress{}
| for _, p := range []ConnectionType{CQL, Alternator} { | ||
| var tlsEnabled, clientCertAuth bool | ||
| var address string | ||
| if p == CQL { | ||
| address = nodeInfoResp.CQLAddr(host) | ||
| tlsEnabled, clientCertAuth = nodeInfoResp.CQLTLSEnabled() | ||
| tlsEnabled = tlsEnabled && !c.ForceTLSDisabled | ||
| if tlsEnabled && !c.ForceNonSSLSessionPort { | ||
| address = nodeInfoResp.CQLSSLAddr(host) | ||
| } | ||
| } else if p == Alternator { | ||
| tlsEnabled, clientCertAuth = nodeInfoResp.AlternatorTLSEnabled() | ||
| address = nodeInfoResp.AlternatorAddr(host) | ||
| } | ||
| if tlsEnabled { | ||
| tlsConfig, err := svc.tlsConfig(c.ID, clientCertAuth) | ||
| if err != nil && !errors.Is(err, service.ErrNotFound) { | ||
| return config, errors.Wrap(err, "fetch TLS config") | ||
| } | ||
| if clientCertAuth && errors.Is(err, service.ErrNotFound) { | ||
| return config, errors.Wrap(err, "client encryption is enabled, but certificate is missing") | ||
| } | ||
| config.TLSConfig[p] = &tlsConfigWithAddress{ | ||
| Config: tlsConfig, | ||
| Address: address, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I know that's how it was done before, but I don't see the reasons for keeping a map consisting of only two entries. Also this for loop which fills this map is cloudy as CQL and ALTERNATOR are still if-ed in the first half of the loop. Also getting configs from map is less clear (are the configs always there? Should we validate that every time we want to get them?)
Wouldn't it be better to directly store 2 TLS configs in Node config?
There was a problem hiding this comment.
You are right.
Created an issue to not jump into the chaos with the PRs.
It will be addressed as a part of the epic.
pkg/service/configcache/service.go
Outdated
| // Alternator defines alternator connection type. | ||
| Alternator | ||
|
|
||
| updateFrequency = 30 * time.Minute |
There was a problem hiding this comment.
Why do we update so rarely? Shouldn't it be something like 5min?
There was a problem hiding this comment.
It actually can, I have no strong opinion though.
If the cache is not updated, but some configuration changed in meantime, then manager may misbehave for some time (if we spread the cache access to all services, not the healtcheck only).
But this is something what we would met with the current way of caching the nodeinfo as well.
Let me bring the default from previous approach. So the 5min you mentioned.
| go func() { | ||
| client, err := svc.scyllaClient(ctx, c.ID) | ||
| if err != nil { | ||
| fmt.Println(err) | ||
| } | ||
|
|
||
| // Hosts that are going to be asked about the configuration are exactly the same as | ||
| // the ones used by the scylla client. | ||
| hostsWg := sync.WaitGroup{} |
There was a problem hiding this comment.
I think it would be safer to wait for all updates before returning from this function. Otherwise it's possible that a single cluster will be updated in parallel by different calls to update function.
There was a problem hiding this comment.
yes, you are right and it's addressed in the PR fixing the next issue.
Shame on me, of not doing it here, but I had to split the code and realized exactly the same what you pointed here.
Let me merge this PR to feature branch and you can continue with the review of next PR.
| } | ||
|
|
||
| go func() { | ||
| client, err := svc.scyllaClient(ctx, c.ID) |
There was a problem hiding this comment.
Just a note that this opens a client without closing it. We probably can't close it because of the cache, but then maybe it makes sense to create client from scratch so that it can be closed right after?
There was a problem hiding this comment.
It actually doesn't lead to TCP connections leak. Here is why.
Keep-alive is enabled by default on HTTP level in go, what means that the client will reuse TCP connection for multiple requests to the same server. That is good.
Scylla-manager explicitly sets the keep-alive on TCP level too
scylla-manager/pkg/scyllaclient/client.go
Line 52 in 701b8cb
Setting keep-alive on TCP level means that inactive connections are eventually detected and closed by the operating system.
The scylla client cache is OK as is, we don't need to explicitly close the client.
karol-kokoszka
force-pushed
the
kk/3767-cache-nodeinfo-outside-healthcheck
branch
from
3247350 to
b0bc2d1
Compare
|
@Michal-Leszczynski I'm merging it to feature branch. |
karol-kokoszka
merged commit afa59b0
into
feature-branch_config_cache_service
Fixes #3802
This is just an initial stub for new cluster-config-cache service.
General description of the goal is here #3767 (comment)
Please make sure that: