query, rule: make endpoint discovery dynamically reloadable #7890
Conversation
MichaHoffmann
force-pushed
the
mhoffmann/make-endpoint-config-dynamically-reloadable
branch
3 times, most recently
from
7b8c118
to
249620c
Compare
MichaHoffmann
force-pushed
the
mhoffmann/make-endpoint-config-dynamically-reloadable
branch
8 times, most recently
from
e57969b
to
963126d
Compare
MichaHoffmann
force-pushed
the
mhoffmann/make-endpoint-config-dynamically-reloadable
branch
5 times, most recently
from
d5cfc49
to
e9f1ff4
Compare
MichaHoffmann
force-pushed
the
mhoffmann/make-endpoint-config-dynamically-reloadable
branch
5 times, most recently
from
12a9008
to
529954c
Compare
MichaHoffmann
force-pushed
the
mhoffmann/make-endpoint-config-dynamically-reloadable
branch
3 times, most recently
from
bb7e84d
to
d3efbc2
Compare
MichaHoffmann
force-pushed
the
mhoffmann/make-endpoint-config-dynamically-reloadable
branch
2 times, most recently
from
cb05e05
to
3284e0d
Compare
cmd/thanos/endpointset.go
Outdated
| return "" | ||
| } | ||
|
|
||
| type EndpointSpec struct { |
There was a problem hiding this comment.
I think this should be closer to the endpointset in the query package.
| { | ||
| // Setup Legacy File SD | ||
| var fileSD *file.Discovery | ||
| if len(legacyFileSDFiles) > 0 { |
There was a problem hiding this comment.
Let's return early:
if len(legacyFIleSDFiles) == 0 {
return
}
...
There was a problem hiding this comment.
We cannot, we would return from the whole method early
| return nil | ||
|
|
||
| }, func(err error) { | ||
| cancelLegacyFileSD() |
There was a problem hiding this comment.
Maybe we can just close here fileSDUpdates to make ctxUpdateLegacyFileSDUpdate unnecessary?
There was a problem hiding this comment.
That feels dangerous, we dont own the channel; what if there is a race and filesd wants to send on the channel
There was a problem hiding this comment.
What if we close the channel after Run()? That way we ensure that no one will send anything on it anymore
There was a problem hiding this comment.
If only drawback of having context is namespace pollution; I would prefer keeping the context here; its somewhat established pattern with the g.Add() functions and outside of the naming i dont find it too offensive
| return removeDuplicateEndpointSpecs(specs) | ||
| }, unhealthyTimeout, endpointTimeout, queryConnMetricLabels...) | ||
|
|
||
| ctxEndpointUpdate, cancelEndpointUpdate := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
Isn't it possible to just create one ctx, cancel := context.WithCancel(context.Background()) and use it in all branches (expect when there's a deadline)?
There was a problem hiding this comment.
Ah well; It might, i thought its nicer to have one per group ~ It might be just matter of taste
| type EndpointSpec struct { | ||
| Strict bool `yaml:"strict"` | ||
| Group bool `yaml:"group"` | ||
| Address string `yaml:"address"` |
There was a problem hiding this comment.
We should document the conent of this file. As a person who is not too familiar with this I feel confused about what strict and group is doing so better to have docs
There was a problem hiding this comment.
Essentially same as for flags "--strict-endpoint" and "--endpoint-group" ~ Can I add docs in followup?
MichaHoffmann
force-pushed
the
mhoffmann/make-endpoint-config-dynamically-reloadable
branch
from
3284e0d
to
a45ffad
Compare
* Removed previously deprecated and hidden flags to configure endpoints ( --rule, --target, ...) * Added new flags --endpoint.sd-config, --endpoint-sd-config-reload-interval to configure a dynamic SD file * Moved endpoint set construction into cmd/thanos/endpointset.go for a little cleanup The new config makes it possible to also set "strict" and "group" flags on the endpoint instead of only their addresses, making it possible to have file based service discovery for endpoint groups too. Signed-off-by: Michael Hoffmann <[email protected]>
MichaHoffmann
dismissed stale reviews from fpetkovski and saswatamcode
via
c7dd843
MichaHoffmann
force-pushed
the
mhoffmann/make-endpoint-config-dynamically-reloadable
branch
from
a45ffad
to
c7dd843
Compare
Changes
--store.sd-fileand--store.sd-intervalflags--endpoint.sd-config,--endpoint-sd-config-reload-intervalto configure a dynamic SD fileThe new config makes it possible to also set "strict" and "group" flags on the endpoint instead of only their addresses, making it possible to have file based service discovery for endpoint groups too.
Verification
Existing E2E test suite, some manual testing.