kv: Ignore backend servers with no url #1196
Conversation
provider/kv.go
Outdated
| @@ -162,6 +162,9 @@ func (provider *Kv) list(keys ...string) []string { | |||
| func (provider *Kv) listServers(backend string) []string { | |||
| serverNames := provider.list(backend, "/servers/") | |||
| return fun.Filter(func(serverName string) bool { | |||
| if _, err := provider.kvclient.Get(fmt.Sprint(serverName, "/url")); err != nil { | |||
There was a problem hiding this comment.
First time getting in touch with our k/v store interfaces and libkv in general, so behold. :-)
Looking at https://godoc.org/github.com/docker/libkv/store#pkg-variables, it seems like we should be able to distinguish between ErrKeyNotFound and other errors. If that's feasible, I think we should at least log unexpected errors.
WDYT?
There was a problem hiding this comment.
If that's feasible, I think we should at least log unexpected errors.
Yes, that is properly best. Something like log.Debugf("Unexpected error %s", err)? or what do you think the error sentence should be? Naming stuff and so on is hard :)
Could we have a quick test please? :)
Of course, should be pretty straight forward. :)
There was a problem hiding this comment.
Re: error message: I'd use something like:
log.Errorf("Failed to retrieve value for key %s: %s", key, err)
key can be an extracted variable because we might need it up to two times (once for the Get and then again for the error message).
|
@timoreimann could you take a look now? |
|
Rebased |
| { | ||
| Key: "traefik/backends/backend.with.dot.too/servers/server.with.dot.without.url/weight", | ||
| Value: []byte("0"), | ||
| }, |
There was a problem hiding this comment.
Shouldn't we also have added a test where we encounter a "true" error and make sure we filter?
There was a problem hiding this comment.
Can you elaborate a bit? I'm not sure what you mean.
There was a problem hiding this comment.
In the new production code, we have added two paths: One where we encounter a "not found" error and another one where we encounter a critical, "true" error. Both paths should do the same, which is to return false.
You have changed the mock to return the "not found" error if Get cannot find a given key among the configured ones. That's great because there's now also a test case to execute that path. However, for the "true" error path to happen, we'd need to configure the mock to return an error by another test case. However, I don't see any new test case with the error switch being set.
So I'm thinking that we might be missing a test case here.
There was a problem hiding this comment.
Hmm, I'm not sure how it can/should be implemented. We can't rely on the Mock Error bool, as that cause List to error out.
I think we need to crate our own KVPair type with a error "field", but that seems kind of messy. Do you have a simpler idea?
There was a problem hiding this comment.
I'd change the Mock struct to separate the currently single error into one for Get and one for List calls. We'll then have to adjust all tests that test error cases, but I think it's worth it: We have to improve our test coverage in the future anyway, and more fine-grained error distinctions are most likely going to be needed.
There was a problem hiding this comment.
I'd change the Mock struct to separate the currently single error into one for Get and one for List calls.
Done, see last commit. I will look on a listServers test when I get some more time.
Currently with a kv tree like: /traefik/backends/b1/servers/ẁeb1 /traefik/backends/b1/servers/web2 /traefik/backends/b1/servers/web2/url Traefik would try to forward traffic to web1, which is impossible as traefik doesn't know the url of web1. This commit solve that, by ignoring backend server with no url "key" when generating the config. This is very useful, for people who use etcd TTL feature. They can then just "renew" the url key every X second, and if the server goes down, it is automatic removed from traefik after the TTL.
Currently with a kv tree like:
/traefik/backends/b1/servers/ẁeb1
/traefik/backends/b1/servers/web2
/traefik/backends/b1/servers/web2/url
Traefik would try to forward traffic to web1, which is impossible as
traefik doesn't know the url of web1.
This commit solve that, by ignoring backend server with no url "key"
when generating the config.
This is very useful, for people who use etcd TTL feature. They can then
just "renew" the url key every X second, and if the server goes down, it
is automatic removed from traefik after the TTL.