Node metadata #2643
Node metadata #2643
Conversation
kyhavlov
force-pushed
the
f-node-metadata
branch
from
da7a9fc to
03273e4
Compare
| config.Meta = map[string]string{ | ||
| "": "value1", | ||
| } | ||
| if err := agent.loadMetadata(config); err == nil { |
There was a problem hiding this comment.
It would be good in these error cases to make sure it's failing for the reason you expect (check some part of the error string)
| } | ||
|
|
||
| func TestAgent_validateMetaPair(t *testing.T) { | ||
| longKey := fmt.Sprintf(fmt.Sprintf("%%%ds", metaKeyMaxLength+1), "") |
There was a problem hiding this comment.
there's a strings.Repeat() that might be a little simpler here
| Success bool | ||
| }{ | ||
| // valid pair | ||
| {"key", "value", true}, |
There was a problem hiding this comment.
same for these - might be good to have a string that should be in the returned err (empty could mean no error)
| @@ -1081,8 +1081,12 @@ func (c *Command) handleReload(config *Config) (*Config, error) { | |||
| errs = multierror.Append(errs, fmt.Errorf("Failed unloading checks: %s", err)) | |||
| return nil, errs | |||
| } | |||
| if err := c.agent.unloadMetadata(); err != nil { | |||
There was a problem hiding this comment.
Will this properly roll back if an error is encountered?
There was a problem hiding this comment.
Right now this just resets the metadata map so it can't actually get an error; i'll change it to not return anything for simplicity
There was a problem hiding this comment.
I'm not sure that'll work though - there are other error paths in here as well, so we need a way to save the old meta, unload + reload, and restore the old meta if anything goes wrong with any part of the reload.
There was a problem hiding this comment.
Are we doing anything like that with the checks/services during the reload? it seems like they have the same problem at the moment.
There was a problem hiding this comment.
Oh wait - I thought that's what the snapshot was for, but it's just for checks. Reload depends on the config read finding the errors before it starts changing state but it doesn't roll back once it starts to apply the changes. We should have readConfig() validate the metadata so it's caught early.
| @@ -342,6 +342,9 @@ type Config struct { | |||
| // they are configured with TranslateWanAddrs set to true. | |||
| TaggedAddresses map[string]string | |||
|
|
|||
| // Node metadata | |||
There was a problem hiding this comment.
I'd expand this comment to explain why we elide this from the JSON (since it might be stale). This will be confusing later as to why we did this :-)
| // parseMetaFilter is used to parse the ?node-meta=key:value query parameter, used for | ||
| // filtering results to nodes with the given metadata key/value | ||
| func (s *HTTPServer) parseMetaFilter(req *http.Request, key *string, value *string) { | ||
| if filter, ok := req.URL.Query()["node-meta"]; ok && len(filter) > 0 { |
There was a problem hiding this comment.
Hmm - wonder if we should plumb multiple queries down but have the lower-level return an error if there's more than 1 thing in the list. Might be good if we want to expand it later.
There was a problem hiding this comment.
So for the moment, just give an error message back to be clear that only one filter at a time can be used? That seems like a good idea.
There was a problem hiding this comment.
Yeah exactly - I think the error could come from all the way down in the state store so we can support it later with just a server-side change.
| NodeMetaValue: "somevalue", | ||
| } | ||
| var out structs.IndexedNodes | ||
| err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &out) |
There was a problem hiding this comment.
This gets called but you don't look at the results.
| NodeMetaValue: "somevalue", | ||
| } | ||
| var out structs.IndexedServices | ||
| err := msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &args, &out) |
There was a problem hiding this comment.
This gets called but you don't look at the results.
kyhavlov
force-pushed
the
f-node-metadata
branch
from
8f4f8f5 to
c9e430c
Compare
|
|
||
| RaftIndex | ||
| } | ||
| type Nodes []*Node | ||
|
|
||
| // Used for filtering nodes by metadata key/value pairs |
There was a problem hiding this comment.
You can delete this struct.
|
LGTM - just one unused struct to remove and this is good to go (also needs doc updates). |
kyhavlov
force-pushed
the
f-node-metadata
branch
from
bef3f60 to
7f91cd1
Compare
kyhavlov
force-pushed
the
f-node-metadata
branch
2 times, most recently
from
6ff54b6 to
0879b89
Compare
kyhavlov
force-pushed
the
f-node-metadata
branch
from
0879b89 to
87c0283
Compare
Still missing documentation, but this covers the initial stuff with filtering on /catalog/nodes and /catalog/services.
Closes #154.