From 3797e6544c400ba37d95770a29cd221cca94abac Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Mon, 20 Jun 2016 14:07:08 -0700 Subject: [PATCH 01/15] consul: support PreferLocal in PQ's --- consul/prepared_query_endpoint.go | 11 +++++++++++ consul/structs/prepared_query.go | 9 +++++++++ 2 files changed, 20 insertions(+) diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index d30b6c10dd..970b459d56 100644 --- a/consul/prepared_query_endpoint.go +++ b/consul/prepared_query_endpoint.go @@ -372,6 +372,17 @@ func (p *PreparedQuery) Execute(args *structs.PreparedQueryExecuteRequest, return err } + // Prefer the local service if it exists and is in the set. + if query.Service.PreferLocal { + for i, node := range reply.Nodes { + if node.Node.Node == args.Origin { + remote := append(reply.Nodes[:i], reply.Nodes[i+1:]...) + reply.Nodes = append([]structs.CheckServiceNode{node}, remote...) + break + } + } + } + // Apply the limit if given. if args.Limit > 0 && len(reply.Nodes) > args.Limit { reply.Nodes = reply.Nodes[:args.Limit] diff --git a/consul/structs/prepared_query.go b/consul/structs/prepared_query.go index b1b20c9ed3..85439b6e0c 100644 --- a/consul/structs/prepared_query.go +++ b/consul/structs/prepared_query.go @@ -34,6 +34,11 @@ type ServiceQuery struct { // discarded) OnlyPassing bool + // If PreferLocal is true, the local agent will be checked for a local + // copy of the service before continuing to remote machines. This is + // useful to prefer colocated services but fall back when unavailable. + PreferLocal bool + // Tags are a set of required and/or disallowed tags. If a tag is in // this list it must be present. If the tag is preceded with "!" then // it is disallowed. @@ -163,6 +168,10 @@ func (q *PreparedQuerySpecificRequest) RequestDatacenter() string { // PreparedQueryExecuteRequest is used to execute a prepared query. type PreparedQueryExecuteRequest struct { + // Origin is used to carry around a reference to the node which + // is executing the query on behalf of the client. + Origin string + // Datacenter is the target this request is intended for. Datacenter string From 865c264b9c3b2b8f0693c12aded2b03a7e5f1b3f Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Mon, 20 Jun 2016 14:07:36 -0700 Subject: [PATCH 02/15] agent: set origin during PQ execution --- command/agent/dns.go | 1 + command/agent/prepared_query_endpoint.go | 1 + 2 files changed, 2 insertions(+) diff --git a/command/agent/dns.go b/command/agent/dns.go index 2a8d8dd5c3..ea23d088d8 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -592,6 +592,7 @@ RPC: func (d *DNSServer) preparedQueryLookup(network, datacenter, query string, req, resp *dns.Msg) { // Execute the prepared query. args := structs.PreparedQueryExecuteRequest{ + Origin: d.agent.config.NodeName, Datacenter: datacenter, QueryIDOrName: query, QueryOptions: structs.QueryOptions{ diff --git a/command/agent/prepared_query_endpoint.go b/command/agent/prepared_query_endpoint.go index bf643f7c26..ad27933957 100644 --- a/command/agent/prepared_query_endpoint.go +++ b/command/agent/prepared_query_endpoint.go @@ -95,6 +95,7 @@ func parseLimit(req *http.Request, limit *int) error { // preparedQueryExecute executes a prepared query. func (s *HTTPServer) preparedQueryExecute(id string, resp http.ResponseWriter, req *http.Request) (interface{}, error) { args := structs.PreparedQueryExecuteRequest{ + Origin: s.agent.config.NodeName, QueryIDOrName: id, } s.parseSource(req, &args.Source) From 100a46727ff0e5b718046ab6ced1e480c0c0558d Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Mon, 20 Jun 2016 14:53:13 -0700 Subject: [PATCH 03/15] consul: test raw PreferLocal functionality --- consul/prepared_query_endpoint_test.go | 40 +++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index cb10eb8f8c..b4a62b130f 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -1607,6 +1607,45 @@ func TestPreparedQuery_Execute(t *testing.T) { t.Fatalf("unique shuffle ratio too low: %d/100", len(uniques)) } + // Set the query to prefer a colocated service + query.Op = structs.PreparedQueryUpdate + query.Query.Service.PreferLocal = true + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { + t.Fatalf("err: %v", err) + } + + // Now try querying and make sure the local node is preferred. + { + req := structs.PreparedQueryExecuteRequest{ + Origin: "node1", + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, + } + + var reply structs.PreparedQueryExecuteResponse + + // Repeat this a few times to make sure we don't just get lucky. + for i := 0; i < 10; i++ { + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + + if n := len(reply.Nodes); n != 10 { + t.Fatalf("expect 10 nodes, got: %d", n) + } + if node := reply.Nodes[0].Node.Node; node != "node1" { + t.Fatalf("expect node1 first, got: %q", node) + } + } + } + + // Remove local preference. + query.Query.Service.PreferLocal = false + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { + t.Fatalf("err:% v", err) + } + // Update the health of a node to mark it critical. setHealth := func(node string, health string) { req := structs.RegisterRequest{ @@ -1683,7 +1722,6 @@ func TestPreparedQuery_Execute(t *testing.T) { } // Make the query more picky so it excludes warning nodes. - query.Op = structs.PreparedQueryUpdate query.Query.Service.OnlyPassing = true if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { t.Fatalf("err: %v", err) From 4c1afb1bc620a09a827aaa0b06802d9c95f8b238 Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Tue, 21 Jun 2016 12:39:40 -0700 Subject: [PATCH 04/15] consul: use the Near field instead of PreferLocal --- command/agent/http.go | 8 +---- command/agent/http_test.go | 12 -------- command/agent/prepared_query_endpoint_test.go | 2 ++ consul/prepared_query_endpoint.go | 29 +++++++++++++++++-- consul/prepared_query_endpoint_test.go | 24 +++++++++++++-- consul/structs/prepared_query.go | 16 +++++----- 6 files changed, 58 insertions(+), 33 deletions(-) diff --git a/command/agent/http.go b/command/agent/http.go index 92247ef2c8..0cfbafbe14 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -531,13 +531,7 @@ func (s *HTTPServer) parseToken(req *http.Request, token *string) { // DC in the request, if given, or else the agent's DC. func (s *HTTPServer) parseSource(req *http.Request, source *structs.QuerySource) { s.parseDC(req, &source.Datacenter) - if node := req.URL.Query().Get("near"); node != "" { - if node == "_agent" { - source.Node = s.agent.config.NodeName - } else { - source.Node = node - } - } + source.Node = req.URL.Query().Get("near") } // parse is a convenience method for endpoints that need diff --git a/command/agent/http_test.go b/command/agent/http_test.go index b6618977f3..11369f8fda 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -382,18 +382,6 @@ func TestParseSource(t *testing.T) { if source.Datacenter != "foo" || source.Node != "bob" { t.Fatalf("bad: %v", source) } - - // The magic "_agent" node name will use the agent's local node name. - req, err = http.NewRequest("GET", - "/v1/catalog/nodes?near=_agent", nil) - if err != nil { - t.Fatalf("err: %v", err) - } - source = structs.QuerySource{} - srv.parseSource(req, &source) - if source.Datacenter != "dc1" || source.Node != srv.agent.config.NodeName { - t.Fatalf("bad: %v", source) - } } func TestParseWait(t *testing.T) { diff --git a/command/agent/prepared_query_endpoint_test.go b/command/agent/prepared_query_endpoint_test.go index 8997de05f3..138124c69e 100644 --- a/command/agent/prepared_query_endpoint_test.go +++ b/command/agent/prepared_query_endpoint_test.go @@ -279,6 +279,7 @@ func TestPreparedQuery_Execute(t *testing.T) { m.executeFn = func(args *structs.PreparedQueryExecuteRequest, reply *structs.PreparedQueryExecuteResponse) error { expected := &structs.PreparedQueryExecuteRequest{ + Origin: srv.agent.config.NodeName, Datacenter: "dc1", QueryIDOrName: "my-id", Limit: 5, @@ -350,6 +351,7 @@ func TestPreparedQuery_Explain(t *testing.T) { m.explainFn = func(args *structs.PreparedQueryExecuteRequest, reply *structs.PreparedQueryExplainResponse) error { expected := &structs.PreparedQueryExecuteRequest{ + Origin: srv.agent.config.NodeName, Datacenter: "dc1", QueryIDOrName: "my-id", Limit: 5, diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index 970b459d56..6b41c907c0 100644 --- a/consul/prepared_query_endpoint.go +++ b/consul/prepared_query_endpoint.go @@ -368,12 +368,35 @@ func (p *PreparedQuery) Execute(args *structs.PreparedQueryExecuteRequest, // Shuffle the results in case coordinates are not available if they // requested an RTT sort. reply.Nodes.Shuffle() - if err := p.srv.sortNodesByDistanceFrom(args.Source, reply.Nodes); err != nil { + + // Get the source to sort by. This can be passed in by the requestor, or + // pre-defined using the Near parameter in the prepared query. If the + // near parameter was defined, that will be preferred. + sortFrom := args.Source + if query.Service.Near != "" { + sortFrom = structs.QuerySource{ + Datacenter: args.Datacenter, + Node: query.Service.Near, + } + } + + // Respect the magic "_agent" flag. + preferLocal := false + if sortFrom.Node == "_agent" { + preferLocal = true + sortFrom.Node = args.Origin + } + + // Perform the distance sort + if err := p.srv.sortNodesByDistanceFrom(sortFrom, reply.Nodes); err != nil { return err } - // Prefer the local service if it exists and is in the set. - if query.Service.PreferLocal { + // Nodes cannot be any "closer" than localhost, so this special case ensures + // the local node is returned first if it is present in the result. This + // allows the local agent to be preferred even when network coordinates are + // not enabled. + if preferLocal { for i, node := range reply.Nodes { if node.Node.Node == args.Origin { remote := append(reply.Nodes[:i], reply.Nodes[i+1:]...) diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index b4a62b130f..b0a752863b 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -1609,7 +1609,7 @@ func TestPreparedQuery_Execute(t *testing.T) { // Set the query to prefer a colocated service query.Op = structs.PreparedQueryUpdate - query.Query.Service.PreferLocal = true + query.Query.Service.Near = "_agent" if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { t.Fatalf("err: %v", err) } @@ -1630,7 +1630,6 @@ func TestPreparedQuery_Execute(t *testing.T) { if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { t.Fatalf("err: %v", err) } - if n := len(reply.Nodes); n != 10 { t.Fatalf("expect 10 nodes, got: %d", n) } @@ -1640,8 +1639,27 @@ func TestPreparedQuery_Execute(t *testing.T) { } } + // Falls back to remote nodes if service is not available locally + { + req := structs.PreparedQueryExecuteRequest{ + Origin: "not-in-result", + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, + } + + var reply structs.PreparedQueryExecuteResponse + + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if n := len(reply.Nodes); n != 10 { + t.Fatalf("expect 10 nodes, got: %d", n) + } + } + // Remove local preference. - query.Query.Service.PreferLocal = false + query.Query.Service.Near = "" if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { t.Fatalf("err:% v", err) } diff --git a/consul/structs/prepared_query.go b/consul/structs/prepared_query.go index 85439b6e0c..49ad25a980 100644 --- a/consul/structs/prepared_query.go +++ b/consul/structs/prepared_query.go @@ -34,10 +34,10 @@ type ServiceQuery struct { // discarded) OnlyPassing bool - // If PreferLocal is true, the local agent will be checked for a local - // copy of the service before continuing to remote machines. This is - // useful to prefer colocated services but fall back when unavailable. - PreferLocal bool + // Near allows the query to always prefer the node nearest the given + // node. If the node does not exist, results are returned in their + // normal randomly-shuffled order. + Near string // Tags are a set of required and/or disallowed tags. If a tag is in // this list it must be present. If the tag is preceded with "!" then @@ -168,10 +168,6 @@ func (q *PreparedQuerySpecificRequest) RequestDatacenter() string { // PreparedQueryExecuteRequest is used to execute a prepared query. type PreparedQueryExecuteRequest struct { - // Origin is used to carry around a reference to the node which - // is executing the query on behalf of the client. - Origin string - // Datacenter is the target this request is intended for. Datacenter string @@ -182,6 +178,10 @@ type PreparedQueryExecuteRequest struct { // Limit will trim the resulting list down to the given limit. Limit int + // Origin is used to carry around a reference to the node which + // is executing the query on behalf of the client. + Origin string + // Source is used to sort the results relative to a given node using // network coordinates. Source QuerySource From 03fea4b091d518e34c52e9d9cb27473c6c87ba32 Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Tue, 21 Jun 2016 12:54:18 -0700 Subject: [PATCH 05/15] consul: test baked-in distance sort --- consul/prepared_query_endpoint_test.go | 35 ++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index b0a752863b..86b0303896 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -1607,7 +1607,7 @@ func TestPreparedQuery_Execute(t *testing.T) { t.Fatalf("unique shuffle ratio too low: %d/100", len(uniques)) } - // Set the query to prefer a colocated service + // Set the query to prefer a colocated service using the magic _agent token query.Op = structs.PreparedQueryUpdate query.Query.Service.Near = "_agent" if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { @@ -1658,7 +1658,38 @@ func TestPreparedQuery_Execute(t *testing.T) { } } - // Remove local preference. + // Bake a non-local node name into Near parameter of the query. This + // node was seeded with a coordinate above so distance sort works. + query.Query.Service.Near = "node3" + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { + t.Fatalf("err: %v", err) + } + + // Try the distance sort again to ensure the nearest node is returned + { + req := structs.PreparedQueryExecuteRequest{ + Origin: "node1", + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, + } + + var reply structs.PreparedQueryExecuteResponse + + for i := 0; i < 10; i++ { + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if n := len(reply.Nodes); n != 10 { + t.Fatalf("expect 10 nodes, got: %d", n) + } + if node := reply.Nodes[0].Node.Node; node != "node3" { + t.Fatalf("expect node3, got: %q", node) + } + } + } + + // Un-bake the Near parameter. query.Query.Service.Near = "" if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { t.Fatalf("err:% v", err) From d567d6a6d808e927965660ec0b2e43b7a7fbf4cd Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Tue, 21 Jun 2016 15:34:26 -0700 Subject: [PATCH 06/15] consul: send origin node + dc when executing prepared queries --- command/agent/dns.go | 5 +- command/agent/prepared_query_endpoint.go | 5 +- command/agent/prepared_query_endpoint_test.go | 10 +++- consul/prepared_query/walk_test.go | 2 + consul/prepared_query_endpoint.go | 8 +-- consul/prepared_query_endpoint_test.go | 49 +++++++++++++++++-- consul/structs/prepared_query.go | 6 ++- 7 files changed, 72 insertions(+), 13 deletions(-) diff --git a/command/agent/dns.go b/command/agent/dns.go index ea23d088d8..28b8fee905 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -592,7 +592,10 @@ RPC: func (d *DNSServer) preparedQueryLookup(network, datacenter, query string, req, resp *dns.Msg) { // Execute the prepared query. args := structs.PreparedQueryExecuteRequest{ - Origin: d.agent.config.NodeName, + Origin: structs.QuerySource{ + Datacenter: d.agent.config.Datacenter, + Node: d.agent.config.NodeName, + }, Datacenter: datacenter, QueryIDOrName: query, QueryOptions: structs.QueryOptions{ diff --git a/command/agent/prepared_query_endpoint.go b/command/agent/prepared_query_endpoint.go index ad27933957..a70944b8af 100644 --- a/command/agent/prepared_query_endpoint.go +++ b/command/agent/prepared_query_endpoint.go @@ -95,7 +95,10 @@ func parseLimit(req *http.Request, limit *int) error { // preparedQueryExecute executes a prepared query. func (s *HTTPServer) preparedQueryExecute(id string, resp http.ResponseWriter, req *http.Request) (interface{}, error) { args := structs.PreparedQueryExecuteRequest{ - Origin: s.agent.config.NodeName, + Origin: structs.QuerySource{ + Datacenter: s.agent.config.Datacenter, + Node: s.agent.config.NodeName, + }, QueryIDOrName: id, } s.parseSource(req, &args.Source) diff --git a/command/agent/prepared_query_endpoint_test.go b/command/agent/prepared_query_endpoint_test.go index 138124c69e..9c7e540f37 100644 --- a/command/agent/prepared_query_endpoint_test.go +++ b/command/agent/prepared_query_endpoint_test.go @@ -279,7 +279,10 @@ func TestPreparedQuery_Execute(t *testing.T) { m.executeFn = func(args *structs.PreparedQueryExecuteRequest, reply *structs.PreparedQueryExecuteResponse) error { expected := &structs.PreparedQueryExecuteRequest{ - Origin: srv.agent.config.NodeName, + Origin: structs.QuerySource{ + Datacenter: srv.agent.config.Datacenter, + Node: srv.agent.config.NodeName, + }, Datacenter: "dc1", QueryIDOrName: "my-id", Limit: 5, @@ -351,7 +354,10 @@ func TestPreparedQuery_Explain(t *testing.T) { m.explainFn = func(args *structs.PreparedQueryExecuteRequest, reply *structs.PreparedQueryExplainResponse) error { expected := &structs.PreparedQueryExecuteRequest{ - Origin: srv.agent.config.NodeName, + Origin: structs.QuerySource{ + Datacenter: srv.agent.config.Datacenter, + Node: srv.agent.config.NodeName, + }, Datacenter: "dc1", QueryIDOrName: "my-id", Limit: 5, diff --git a/consul/prepared_query/walk_test.go b/consul/prepared_query/walk_test.go index db9a75c1cb..05294e3b65 100644 --- a/consul/prepared_query/walk_test.go +++ b/consul/prepared_query/walk_test.go @@ -20,6 +20,7 @@ func TestWalk_ServiceQuery(t *testing.T) { Failover: structs.QueryDatacenterOptions{ Datacenters: []string{"dc1", "dc2"}, }, + Near: "_agent", Tags: []string{"tag1", "tag2", "tag3"}, } if err := walk(service, fn); err != nil { @@ -30,6 +31,7 @@ func TestWalk_ServiceQuery(t *testing.T) { ".Service:the-service", ".Failover.Datacenters[0]:dc1", ".Failover.Datacenters[1]:dc2", + ".Near:_agent", ".Tags[0]:tag1", ".Tags[1]:tag2", ".Tags[2]:tag3", diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index 6b41c907c0..92f5dff05c 100644 --- a/consul/prepared_query_endpoint.go +++ b/consul/prepared_query_endpoint.go @@ -384,7 +384,7 @@ func (p *PreparedQuery) Execute(args *structs.PreparedQueryExecuteRequest, preferLocal := false if sortFrom.Node == "_agent" { preferLocal = true - sortFrom.Node = args.Origin + sortFrom = args.Origin } // Perform the distance sort @@ -395,10 +395,10 @@ func (p *PreparedQuery) Execute(args *structs.PreparedQueryExecuteRequest, // Nodes cannot be any "closer" than localhost, so this special case ensures // the local node is returned first if it is present in the result. This // allows the local agent to be preferred even when network coordinates are - // not enabled. - if preferLocal { + // not enabled. Only works if the results come from the request origin DC. + if preferLocal && reply.Datacenter == args.Origin.Datacenter { for i, node := range reply.Nodes { - if node.Node.Node == args.Origin { + if node.Node.Node == args.Origin.Node { remote := append(reply.Nodes[:i], reply.Nodes[i+1:]...) reply.Nodes = append([]structs.CheckServiceNode{node}, remote...) break diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index 86b0303896..75d3905bc6 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -1617,7 +1617,10 @@ func TestPreparedQuery_Execute(t *testing.T) { // Now try querying and make sure the local node is preferred. { req := structs.PreparedQueryExecuteRequest{ - Origin: "node1", + Origin: structs.QuerySource{ + Datacenter: "dc1", + Node: "node1", + }, Datacenter: "dc1", QueryIDOrName: query.Query.ID, QueryOptions: structs.QueryOptions{Token: execToken}, @@ -1642,7 +1645,10 @@ func TestPreparedQuery_Execute(t *testing.T) { // Falls back to remote nodes if service is not available locally { req := structs.PreparedQueryExecuteRequest{ - Origin: "not-in-result", + Origin: structs.QuerySource{ + Datacenter: "dc1", + Node: "node1", + }, Datacenter: "dc1", QueryIDOrName: query.Query.ID, QueryOptions: structs.QueryOptions{Token: execToken}, @@ -1658,6 +1664,40 @@ func TestPreparedQuery_Execute(t *testing.T) { } } + // Shuffles if the response comes from a non-local DC. We may + // need to try multiple times if at first we get a match by chance. + { + req := structs.PreparedQueryExecuteRequest{ + Origin: structs.QuerySource{ + Datacenter: "dc2", + Node: "node1", + }, + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, + } + + var reply structs.PreparedQueryExecuteResponse + + shuffled := false + for i := 0; i < 10; i++ { + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if n := len(reply.Nodes); n != 10 { + t.Fatalf("expect 10 nodes, got: %d", n) + } + if reply.Nodes[0].Node.Node != "node1" { + shuffled = true + break + } + } + + if !shuffled { + t.Fatal("expect node shuffle for remote results") + } + } + // Bake a non-local node name into Near parameter of the query. This // node was seeded with a coordinate above so distance sort works. query.Query.Service.Near = "node3" @@ -1668,7 +1708,10 @@ func TestPreparedQuery_Execute(t *testing.T) { // Try the distance sort again to ensure the nearest node is returned { req := structs.PreparedQueryExecuteRequest{ - Origin: "node1", + Origin: structs.QuerySource{ + Datacenter: "dc1", + Node: "node1", + }, Datacenter: "dc1", QueryIDOrName: query.Query.ID, QueryOptions: structs.QueryOptions{Token: execToken}, diff --git a/consul/structs/prepared_query.go b/consul/structs/prepared_query.go index 49ad25a980..ef2896ca38 100644 --- a/consul/structs/prepared_query.go +++ b/consul/structs/prepared_query.go @@ -179,8 +179,10 @@ type PreparedQueryExecuteRequest struct { Limit int // Origin is used to carry around a reference to the node which - // is executing the query on behalf of the client. - Origin string + // is executing the query on behalf of the client. It is taken + // as a QuerySource so that it can be used directly for queries + // relating to the agent servicing the request. + Origin QuerySource // Source is used to sort the results relative to a given node using // network coordinates. From c457ee0075bba7ec78a33ff68fbdfea8f053e09a Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Tue, 21 Jun 2016 16:28:26 -0700 Subject: [PATCH 07/15] agent: fix test --- command/agent/prepared_query_endpoint_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/command/agent/prepared_query_endpoint_test.go b/command/agent/prepared_query_endpoint_test.go index 9c7e540f37..2d51f80ae8 100644 --- a/command/agent/prepared_query_endpoint_test.go +++ b/command/agent/prepared_query_endpoint_test.go @@ -354,10 +354,6 @@ func TestPreparedQuery_Explain(t *testing.T) { m.explainFn = func(args *structs.PreparedQueryExecuteRequest, reply *structs.PreparedQueryExplainResponse) error { expected := &structs.PreparedQueryExecuteRequest{ - Origin: structs.QuerySource{ - Datacenter: srv.agent.config.Datacenter, - Node: srv.agent.config.NodeName, - }, Datacenter: "dc1", QueryIDOrName: "my-id", Limit: 5, From 0c2ad07fa9fac9aecfc76b9e023edc34077f39f5 Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Thu, 30 Jun 2016 12:11:20 -0700 Subject: [PATCH 08/15] consul: use source parameter for near prepared queries --- consul/prepared_query_endpoint.go | 45 ++--- consul/prepared_query_endpoint_test.go | 258 +++++++++++++++---------- consul/structs/prepared_query.go | 6 - consul/structs/structs.go | 5 + 4 files changed, 175 insertions(+), 139 deletions(-) diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index 92f5dff05c..6609ea25c6 100644 --- a/consul/prepared_query_endpoint.go +++ b/consul/prepared_query_endpoint.go @@ -369,40 +369,21 @@ func (p *PreparedQuery) Execute(args *structs.PreparedQueryExecuteRequest, // requested an RTT sort. reply.Nodes.Shuffle() - // Get the source to sort by. This can be passed in by the requestor, or - // pre-defined using the Near parameter in the prepared query. If the - // near parameter was defined, that will be preferred. - sortFrom := args.Source - if query.Service.Near != "" { - sortFrom = structs.QuerySource{ - Datacenter: args.Datacenter, - Node: query.Service.Near, + // Check if the query carries a Near parameter, or if the requestor + // supplied a ?near parameter in the request. We can apply distance + // sorting if either of these cases are true, but we don't want to + // affect the established round-robin default. + if args.Source.NearRequested || query.Service.Near != "" { + // Apply the "near" parameter if it exists on the prepared query and + // was not provided in the request args. + if !args.Source.NearRequested && query.Service.Near != "_agent" { + args.Source.Node = query.Service.Near } - } - // Respect the magic "_agent" flag. - preferLocal := false - if sortFrom.Node == "_agent" { - preferLocal = true - sortFrom = args.Origin - } - - // Perform the distance sort - if err := p.srv.sortNodesByDistanceFrom(sortFrom, reply.Nodes); err != nil { - return err - } - - // Nodes cannot be any "closer" than localhost, so this special case ensures - // the local node is returned first if it is present in the result. This - // allows the local agent to be preferred even when network coordinates are - // not enabled. Only works if the results come from the request origin DC. - if preferLocal && reply.Datacenter == args.Origin.Datacenter { - for i, node := range reply.Nodes { - if node.Node.Node == args.Origin.Node { - remote := append(reply.Nodes[:i], reply.Nodes[i+1:]...) - reply.Nodes = append([]structs.CheckServiceNode{node}, remote...) - break - } + // Perform the distance sort + err := p.srv.sortNodesByDistanceFrom(args.Source, reply.Nodes) + if err != nil { + return err } } diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index 75d3905bc6..f8f6636033 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -1548,8 +1548,9 @@ func TestPreparedQuery_Execute(t *testing.T) { Datacenter: "dc1", QueryIDOrName: query.Query.ID, Source: structs.QuerySource{ - Datacenter: "dc1", - Node: "node3", + Datacenter: "dc1", + Node: "node3", + NearRequested: true, }, QueryOptions: structs.QueryOptions{Token: execToken}, } @@ -1607,110 +1608,22 @@ func TestPreparedQuery_Execute(t *testing.T) { t.Fatalf("unique shuffle ratio too low: %d/100", len(uniques)) } - // Set the query to prefer a colocated service using the magic _agent token + // Set the query to return results nearest to node3. This is the only + // node with coordinates, and it carries the service we are asking for, + // so node3 should always show up first. query.Op = structs.PreparedQueryUpdate - query.Query.Service.Near = "_agent" - if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { - t.Fatalf("err: %v", err) - } - - // Now try querying and make sure the local node is preferred. - { - req := structs.PreparedQueryExecuteRequest{ - Origin: structs.QuerySource{ - Datacenter: "dc1", - Node: "node1", - }, - Datacenter: "dc1", - QueryIDOrName: query.Query.ID, - QueryOptions: structs.QueryOptions{Token: execToken}, - } - - var reply structs.PreparedQueryExecuteResponse - - // Repeat this a few times to make sure we don't just get lucky. - for i := 0; i < 10; i++ { - if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { - t.Fatalf("err: %v", err) - } - if n := len(reply.Nodes); n != 10 { - t.Fatalf("expect 10 nodes, got: %d", n) - } - if node := reply.Nodes[0].Node.Node; node != "node1" { - t.Fatalf("expect node1 first, got: %q", node) - } - } - } - - // Falls back to remote nodes if service is not available locally - { - req := structs.PreparedQueryExecuteRequest{ - Origin: structs.QuerySource{ - Datacenter: "dc1", - Node: "node1", - }, - Datacenter: "dc1", - QueryIDOrName: query.Query.ID, - QueryOptions: structs.QueryOptions{Token: execToken}, - } - - var reply structs.PreparedQueryExecuteResponse - - if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { - t.Fatalf("err: %v", err) - } - if n := len(reply.Nodes); n != 10 { - t.Fatalf("expect 10 nodes, got: %d", n) - } - } - - // Shuffles if the response comes from a non-local DC. We may - // need to try multiple times if at first we get a match by chance. - { - req := structs.PreparedQueryExecuteRequest{ - Origin: structs.QuerySource{ - Datacenter: "dc2", - Node: "node1", - }, - Datacenter: "dc1", - QueryIDOrName: query.Query.ID, - QueryOptions: structs.QueryOptions{Token: execToken}, - } - - var reply structs.PreparedQueryExecuteResponse - - shuffled := false - for i := 0; i < 10; i++ { - if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { - t.Fatalf("err: %v", err) - } - if n := len(reply.Nodes); n != 10 { - t.Fatalf("expect 10 nodes, got: %d", n) - } - if reply.Nodes[0].Node.Node != "node1" { - shuffled = true - break - } - } - - if !shuffled { - t.Fatal("expect node shuffle for remote results") - } - } - - // Bake a non-local node name into Near parameter of the query. This - // node was seeded with a coordinate above so distance sort works. query.Query.Service.Near = "node3" if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { t.Fatalf("err: %v", err) } - // Try the distance sort again to ensure the nearest node is returned + // Now run the query and make sure the baked-in service is returned. { req := structs.PreparedQueryExecuteRequest{ - Origin: structs.QuerySource{ - Datacenter: "dc1", - Node: "node1", + Source: structs.QuerySource{ + Datacenter: "dc1", + Node: "foo", + NearRequested: false, }, Datacenter: "dc1", QueryIDOrName: query.Query.ID, @@ -1727,15 +1640,158 @@ func TestPreparedQuery_Execute(t *testing.T) { t.Fatalf("expect 10 nodes, got: %d", n) } if node := reply.Nodes[0].Node.Node; node != "node3" { - t.Fatalf("expect node3, got: %q", node) + t.Fatalf("expect node3 first, got: %q", node) } } } - // Un-bake the Near parameter. + // Query again, but this time set NearRequested to "true". This should + // prove that we allow overriding the baked-in value with ?near. + { + // Set up the query with a non-existent node. This will cause the + // nodes to be shuffled if the passed node is respected, proving + // that we allow the override to happen. + req := structs.PreparedQueryExecuteRequest{ + Source: structs.QuerySource{ + Datacenter: "dc1", + Node: "foo", + NearRequested: true, + }, + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, + } + + var reply structs.PreparedQueryExecuteResponse + + shuffled := false + for i := 0; i < 10; i++ { + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if n := len(reply.Nodes); n != 10 { + t.Fatalf("expect 10 nodes, got: %d", n) + } + if node := reply.Nodes[0].Node.Node; node != "node3" { + shuffled = true + break + } + } + + if !shuffled { + t.Fatalf("expect nodes to be shuffled") + } + } + + // Check that if NearRequested is passed as true, that we sort based + // on the given node and do not use the one stored in the PQ. + { + req := structs.PreparedQueryExecuteRequest{ + Source: structs.QuerySource{ + Datacenter: "dc1", + Node: "node1", + NearRequested: true, + }, + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, + } + + var reply structs.PreparedQueryExecuteResponse + + // We just want to check that we get a non-local node, because + // the local node is the only one with working coordinates. + shuffled := false + for i := 0; i < 10; i++ { + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if n := len(reply.Nodes); n != 10 { + t.Fatalf("expect 10 nodes, got: %d", n) + } + if node := reply.Nodes[0].Node.Node; node != "node3" { + shuffled = true + break + } + } + if !shuffled { + t.Fatal("expect non-local results") + } + } + + // Set the query to prefer a colocated service using the magic _agent token + query.Query.Service.Near = "_agent" + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { + t.Fatalf("err: %v", err) + } + + // Check that the node returned is the one we asked for in the + // query source. This proves that if the PQ has "_agent" baked + // in, we always use the passed-in node. + { + req := structs.PreparedQueryExecuteRequest{ + Source: structs.QuerySource{ + Datacenter: "dc1", + Node: "node3", + }, + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, + } + + var reply structs.PreparedQueryExecuteResponse + + for i := 0; i < 10; i++ { + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if n := len(reply.Nodes); n != 10 { + t.Fatalf("expect 10 nodes, got: %d", n) + } + if node := reply.Nodes[0].Node.Node; node != "node3" { + t.Fatalf("expect node3 first, got: %q", node) + } + } + } + + // Shuffles if the response comes from a non-local DC. Proves that the + // near parameter does not affect this order. + { + req := structs.PreparedQueryExecuteRequest{ + Source: structs.QuerySource{ + Datacenter: "dc2", + Node: "node3", + }, + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, + } + + var reply structs.PreparedQueryExecuteResponse + + shuffled := false + for i := 0; i < 10; i++ { + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if n := len(reply.Nodes); n != 10 { + t.Fatalf("expect 10 nodes, got: %d", n) + } + if reply.Nodes[0].Node.Node != "node3" { + shuffled = true + break + } + } + + if !shuffled { + t.Fatal("expect node shuffle for remote results") + } + } + + // Un-bake the near parameter. query.Query.Service.Near = "" if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { - t.Fatalf("err:% v", err) + t.Fatalf("err: %v", err) } // Update the health of a node to mark it critical. diff --git a/consul/structs/prepared_query.go b/consul/structs/prepared_query.go index ef2896ca38..e79a178822 100644 --- a/consul/structs/prepared_query.go +++ b/consul/structs/prepared_query.go @@ -178,12 +178,6 @@ type PreparedQueryExecuteRequest struct { // Limit will trim the resulting list down to the given limit. Limit int - // Origin is used to carry around a reference to the node which - // is executing the query on behalf of the client. It is taken - // as a QuerySource so that it can be used directly for queries - // relating to the agent servicing the request. - Origin QuerySource - // Source is used to sort the results relative to a given node using // network coordinates. Source QuerySource diff --git a/consul/structs/structs.go b/consul/structs/structs.go index 4f99399b38..565b1548c3 100644 --- a/consul/structs/structs.go +++ b/consul/structs/structs.go @@ -196,6 +196,11 @@ func (r *DeregisterRequest) RequestDatacenter() string { type QuerySource struct { Datacenter string Node string + + // NearRequested indicates where the values in this QuerySource came + // from. When true, the values were provided by the requestor, + // otherwise they were filled by the agent servicing the request. + NearRequested bool } // DCSpecificRequest is used to query about a specific DC From 104b234ddea5b7b81bc0349b3bc7fc7cdc83fd81 Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Thu, 30 Jun 2016 12:11:48 -0700 Subject: [PATCH 09/15] agent: always pass local agent query source, allow override --- command/agent/dns.go | 13 ++++-- command/agent/http.go | 13 +++++- command/agent/http_test.go | 9 ++-- command/agent/prepared_query_endpoint.go | 4 -- command/agent/prepared_query_endpoint_test.go | 42 +++++++++++++++---- 5 files changed, 60 insertions(+), 21 deletions(-) diff --git a/command/agent/dns.go b/command/agent/dns.go index 28b8fee905..1b7a71a08f 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -592,16 +592,21 @@ RPC: func (d *DNSServer) preparedQueryLookup(network, datacenter, query string, req, resp *dns.Msg) { // Execute the prepared query. args := structs.PreparedQueryExecuteRequest{ - Origin: structs.QuerySource{ - Datacenter: d.agent.config.Datacenter, - Node: d.agent.config.NodeName, - }, Datacenter: datacenter, QueryIDOrName: query, QueryOptions: structs.QueryOptions{ Token: d.agent.config.ACLToken, AllowStale: d.config.AllowStale, }, + + // Always pass the local agent through as the source. In the DNS + // interface, there is no provision for passing additional query + // parameters, so we send the local agent's data through to allow + // distance sorting relative to ourself on the server side. + Source: structs.QuerySource{ + Datacenter: d.agent.config.Datacenter, + Node: d.agent.config.NodeName, + }, } // TODO (slackpad) - What's a safe limit we can set here? It seems like diff --git a/command/agent/http.go b/command/agent/http.go index 0cfbafbe14..18e292ac25 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -531,7 +531,18 @@ func (s *HTTPServer) parseToken(req *http.Request, token *string) { // DC in the request, if given, or else the agent's DC. func (s *HTTPServer) parseSource(req *http.Request, source *structs.QuerySource) { s.parseDC(req, &source.Datacenter) - source.Node = req.URL.Query().Get("near") + + // Always start with the local node as the source. + source.Node = s.agent.config.NodeName + + // If ?near was provided, take the value send it along. We also mark the + // fact that an override was provided with the NearRequested bool. + if node := req.URL.Query().Get("near"); node != "" { + source.NearRequested = true + if node != "_agent" { + source.Node = node + } + } } // parse is a convenience method for endpoints that need diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 11369f8fda..43bbf436fa 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -345,8 +345,9 @@ func TestParseSource(t *testing.T) { defer srv.Shutdown() defer srv.agent.Shutdown() - // Default is agent's DC and no node (since the user didn't care, then - // just give them the cheapest possible query). + // Default is agent's DC and the local node, with the near flag false + // (since the user didn't care, then just give them the cheapest possible + // query). req, err := http.NewRequest("GET", "/v1/catalog/nodes", nil) if err != nil { @@ -354,7 +355,7 @@ func TestParseSource(t *testing.T) { } source := structs.QuerySource{} srv.parseSource(req, &source) - if source.Datacenter != "dc1" || source.Node != "" { + if source.Datacenter != "dc1" || source.Node != srv.agent.config.NodeName { t.Fatalf("bad: %v", source) } @@ -366,7 +367,7 @@ func TestParseSource(t *testing.T) { } source = structs.QuerySource{} srv.parseSource(req, &source) - if source.Datacenter != "dc1" || source.Node != "bob" { + if source.Datacenter != "dc1" || source.Node != "bob" || !source.NearRequested { t.Fatalf("bad: %v", source) } diff --git a/command/agent/prepared_query_endpoint.go b/command/agent/prepared_query_endpoint.go index a70944b8af..bf643f7c26 100644 --- a/command/agent/prepared_query_endpoint.go +++ b/command/agent/prepared_query_endpoint.go @@ -95,10 +95,6 @@ func parseLimit(req *http.Request, limit *int) error { // preparedQueryExecute executes a prepared query. func (s *HTTPServer) preparedQueryExecute(id string, resp http.ResponseWriter, req *http.Request) (interface{}, error) { args := structs.PreparedQueryExecuteRequest{ - Origin: structs.QuerySource{ - Datacenter: s.agent.config.Datacenter, - Node: s.agent.config.NodeName, - }, QueryIDOrName: id, } s.parseSource(req, &args.Source) diff --git a/command/agent/prepared_query_endpoint_test.go b/command/agent/prepared_query_endpoint_test.go index 2d51f80ae8..8fd317142a 100644 --- a/command/agent/prepared_query_endpoint_test.go +++ b/command/agent/prepared_query_endpoint_test.go @@ -279,16 +279,13 @@ func TestPreparedQuery_Execute(t *testing.T) { m.executeFn = func(args *structs.PreparedQueryExecuteRequest, reply *structs.PreparedQueryExecuteResponse) error { expected := &structs.PreparedQueryExecuteRequest{ - Origin: structs.QuerySource{ - Datacenter: srv.agent.config.Datacenter, - Node: srv.agent.config.NodeName, - }, Datacenter: "dc1", QueryIDOrName: "my-id", Limit: 5, Source: structs.QuerySource{ - Datacenter: "dc1", - Node: "my-node", + Datacenter: "dc1", + Node: "my-node", + NearRequested: true, }, QueryOptions: structs.QueryOptions{ Token: "my-token", @@ -327,6 +324,34 @@ func TestPreparedQuery_Execute(t *testing.T) { } }) + // Ensure the proper params are set when no special args are passed + httpTest(t, func(srv *HTTPServer) { + m := MockPreparedQuery{} + if err := srv.agent.InjectEndpoint("PreparedQuery", &m); err != nil { + t.Fatalf("err: %v", err) + } + + m.executeFn = func(args *structs.PreparedQueryExecuteRequest, reply *structs.PreparedQueryExecuteResponse) error { + if args.Source.NearRequested { + t.Fatal("expect NearRequested to be false") + } + if args.Source.Node == "" { + t.Fatalf("expect Source to be %q, got: %q", srv.agent.config.NodeName, args.Source.Node) + } + return nil + } + + req, err := http.NewRequest("GET", "/v1/query/my-id/execute", nil) + if err != nil { + t.Fatalf("err: %v", err) + } + + resp := httptest.NewRecorder() + if _, err := srv.PreparedQuerySpecific(resp, req); err != nil { + t.Fatalf("err: %v", err) + } + }) + httpTest(t, func(srv *HTTPServer) { body := bytes.NewBuffer(nil) req, err := http.NewRequest("GET", "/v1/query/not-there/execute", body) @@ -358,8 +383,9 @@ func TestPreparedQuery_Explain(t *testing.T) { QueryIDOrName: "my-id", Limit: 5, Source: structs.QuerySource{ - Datacenter: "dc1", - Node: "my-node", + Datacenter: "dc1", + Node: "my-node", + NearRequested: true, }, QueryOptions: structs.QueryOptions{ Token: "my-token", From 62884a22d4d7945e585ac802ab9768e4f763487a Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Thu, 30 Jun 2016 16:51:18 -0700 Subject: [PATCH 10/15] consul: send agent source data as separate query source --- command/agent/dns.go | 10 +- command/agent/http.go | 13 +- command/agent/http_test.go | 9 +- command/agent/prepared_query_endpoint.go | 8 ++ command/agent/prepared_query_endpoint_test.go | 30 +++-- consul/prepared_query_endpoint.go | 33 ++--- consul/prepared_query_endpoint_test.go | 114 +++++++++--------- consul/structs/prepared_query.go | 4 + consul/structs/structs.go | 5 - 9 files changed, 119 insertions(+), 107 deletions(-) diff --git a/command/agent/dns.go b/command/agent/dns.go index 1b7a71a08f..9e2357dea4 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -599,11 +599,11 @@ func (d *DNSServer) preparedQueryLookup(network, datacenter, query string, req, AllowStale: d.config.AllowStale, }, - // Always pass the local agent through as the source. In the DNS - // interface, there is no provision for passing additional query - // parameters, so we send the local agent's data through to allow - // distance sorting relative to ourself on the server side. - Source: structs.QuerySource{ + // Always pass the local agent through. In the DNS interface, there + // is no provision for passing additional query parameters, so we + // send the local agent's data through to allow distance sorting + // relative to ourself on the server side. + Agent: structs.QuerySource{ Datacenter: d.agent.config.Datacenter, Node: d.agent.config.NodeName, }, diff --git a/command/agent/http.go b/command/agent/http.go index 18e292ac25..0cfbafbe14 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -531,18 +531,7 @@ func (s *HTTPServer) parseToken(req *http.Request, token *string) { // DC in the request, if given, or else the agent's DC. func (s *HTTPServer) parseSource(req *http.Request, source *structs.QuerySource) { s.parseDC(req, &source.Datacenter) - - // Always start with the local node as the source. - source.Node = s.agent.config.NodeName - - // If ?near was provided, take the value send it along. We also mark the - // fact that an override was provided with the NearRequested bool. - if node := req.URL.Query().Get("near"); node != "" { - source.NearRequested = true - if node != "_agent" { - source.Node = node - } - } + source.Node = req.URL.Query().Get("near") } // parse is a convenience method for endpoints that need diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 43bbf436fa..231fd43285 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -345,9 +345,8 @@ func TestParseSource(t *testing.T) { defer srv.Shutdown() defer srv.agent.Shutdown() - // Default is agent's DC and the local node, with the near flag false - // (since the user didn't care, then just give them the cheapest possible - // query). + // Default is agent's DC and no node (since the user didn't care, + // then just give them the cheapest possible query). req, err := http.NewRequest("GET", "/v1/catalog/nodes", nil) if err != nil { @@ -355,7 +354,7 @@ func TestParseSource(t *testing.T) { } source := structs.QuerySource{} srv.parseSource(req, &source) - if source.Datacenter != "dc1" || source.Node != srv.agent.config.NodeName { + if source.Datacenter != "dc1" || source.Node != "" { t.Fatalf("bad: %v", source) } @@ -367,7 +366,7 @@ func TestParseSource(t *testing.T) { } source = structs.QuerySource{} srv.parseSource(req, &source) - if source.Datacenter != "dc1" || source.Node != "bob" || !source.NearRequested { + if source.Datacenter != "dc1" || source.Node != "bob" { t.Fatalf("bad: %v", source) } diff --git a/command/agent/prepared_query_endpoint.go b/command/agent/prepared_query_endpoint.go index bf643f7c26..1a6ff6d72e 100644 --- a/command/agent/prepared_query_endpoint.go +++ b/command/agent/prepared_query_endpoint.go @@ -96,6 +96,10 @@ func parseLimit(req *http.Request, limit *int) error { func (s *HTTPServer) preparedQueryExecute(id string, resp http.ResponseWriter, req *http.Request) (interface{}, error) { args := structs.PreparedQueryExecuteRequest{ QueryIDOrName: id, + Agent: structs.QuerySource{ + Node: s.agent.config.NodeName, + Datacenter: s.agent.config.Datacenter, + }, } s.parseSource(req, &args.Source) if done := s.parse(resp, req, &args.Datacenter, &args.QueryOptions); done { @@ -131,6 +135,10 @@ func (s *HTTPServer) preparedQueryExecute(id string, resp http.ResponseWriter, r func (s *HTTPServer) preparedQueryExplain(id string, resp http.ResponseWriter, req *http.Request) (interface{}, error) { args := structs.PreparedQueryExecuteRequest{ QueryIDOrName: id, + Agent: structs.QuerySource{ + Node: s.agent.config.NodeName, + Datacenter: s.agent.config.Datacenter, + }, } s.parseSource(req, &args.Source) if done := s.parse(resp, req, &args.Datacenter, &args.QueryOptions); done { diff --git a/command/agent/prepared_query_endpoint_test.go b/command/agent/prepared_query_endpoint_test.go index 8fd317142a..ff757e0acf 100644 --- a/command/agent/prepared_query_endpoint_test.go +++ b/command/agent/prepared_query_endpoint_test.go @@ -283,9 +283,12 @@ func TestPreparedQuery_Execute(t *testing.T) { QueryIDOrName: "my-id", Limit: 5, Source: structs.QuerySource{ - Datacenter: "dc1", - Node: "my-node", - NearRequested: true, + Datacenter: "dc1", + Node: "my-node", + }, + Agent: structs.QuerySource{ + Datacenter: srv.agent.config.Datacenter, + Node: srv.agent.config.NodeName, }, QueryOptions: structs.QueryOptions{ Token: "my-token", @@ -332,11 +335,15 @@ func TestPreparedQuery_Execute(t *testing.T) { } m.executeFn = func(args *structs.PreparedQueryExecuteRequest, reply *structs.PreparedQueryExecuteResponse) error { - if args.Source.NearRequested { - t.Fatal("expect NearRequested to be false") + if args.Source.Node != "" { + t.Fatalf("expect node to be empty, got %q", args.Source.Node) } - if args.Source.Node == "" { - t.Fatalf("expect Source to be %q, got: %q", srv.agent.config.NodeName, args.Source.Node) + expect := structs.QuerySource{ + Datacenter: srv.agent.config.Datacenter, + Node: srv.agent.config.NodeName, + } + if !reflect.DeepEqual(args.Agent, expect) { + t.Fatalf("expect: %#v\nactual: %#v", expect, args.Agent) } return nil } @@ -383,9 +390,12 @@ func TestPreparedQuery_Explain(t *testing.T) { QueryIDOrName: "my-id", Limit: 5, Source: structs.QuerySource{ - Datacenter: "dc1", - Node: "my-node", - NearRequested: true, + Datacenter: "dc1", + Node: "my-node", + }, + Agent: structs.QuerySource{ + Datacenter: srv.agent.config.Datacenter, + Node: srv.agent.config.NodeName, }, QueryOptions: structs.QueryOptions{ Token: "my-token", diff --git a/consul/prepared_query_endpoint.go b/consul/prepared_query_endpoint.go index 6609ea25c6..5f7d788943 100644 --- a/consul/prepared_query_endpoint.go +++ b/consul/prepared_query_endpoint.go @@ -369,22 +369,25 @@ func (p *PreparedQuery) Execute(args *structs.PreparedQueryExecuteRequest, // requested an RTT sort. reply.Nodes.Shuffle() - // Check if the query carries a Near parameter, or if the requestor - // supplied a ?near parameter in the request. We can apply distance - // sorting if either of these cases are true, but we don't want to - // affect the established round-robin default. - if args.Source.NearRequested || query.Service.Near != "" { - // Apply the "near" parameter if it exists on the prepared query and - // was not provided in the request args. - if !args.Source.NearRequested && query.Service.Near != "_agent" { - args.Source.Node = query.Service.Near - } + // Build the query source. This can be provided by the client, or by + // the prepared query. Client-specified takes priority. + qs := args.Source + if qs.Datacenter == "" { + qs.Datacenter = args.Agent.Datacenter + } + if query.Service.Near != "" && qs.Node == "" { + qs.Node = query.Service.Near + } - // Perform the distance sort - err := p.srv.sortNodesByDistanceFrom(args.Source, reply.Nodes) - if err != nil { - return err - } + // Respect the magic "_agent" flag. + if qs.Node == "_agent" { + qs.Node = args.Agent.Node + } + + // Perform the distance sort + err = p.srv.sortNodesByDistanceFrom(qs, reply.Nodes) + if err != nil { + return err } // Apply the limit if given. diff --git a/consul/prepared_query_endpoint_test.go b/consul/prepared_query_endpoint_test.go index f8f6636033..5630f26c61 100644 --- a/consul/prepared_query_endpoint_test.go +++ b/consul/prepared_query_endpoint_test.go @@ -1548,9 +1548,8 @@ func TestPreparedQuery_Execute(t *testing.T) { Datacenter: "dc1", QueryIDOrName: query.Query.ID, Source: structs.QuerySource{ - Datacenter: "dc1", - Node: "node3", - NearRequested: true, + Datacenter: "dc1", + Node: "node3", }, QueryOptions: structs.QueryOptions{Token: execToken}, } @@ -1617,13 +1616,12 @@ func TestPreparedQuery_Execute(t *testing.T) { t.Fatalf("err: %v", err) } - // Now run the query and make sure the baked-in service is returned. + // Now run the query and make sure the sort looks right. { req := structs.PreparedQueryExecuteRequest{ - Source: structs.QuerySource{ - Datacenter: "dc1", - Node: "foo", - NearRequested: false, + Agent: structs.QuerySource{ + Datacenter: "dc1", + Node: "node3", }, Datacenter: "dc1", QueryIDOrName: query.Query.ID, @@ -1645,17 +1643,20 @@ func TestPreparedQuery_Execute(t *testing.T) { } } - // Query again, but this time set NearRequested to "true". This should - // prove that we allow overriding the baked-in value with ?near. + // Query again, but this time set a client-supplied query source. This + // proves that we allow overriding the baked-in value with ?near. { // Set up the query with a non-existent node. This will cause the // nodes to be shuffled if the passed node is respected, proving // that we allow the override to happen. req := structs.PreparedQueryExecuteRequest{ Source: structs.QuerySource{ - Datacenter: "dc1", - Node: "foo", - NearRequested: true, + Datacenter: "dc1", + Node: "foo", + }, + Agent: structs.QuerySource{ + Datacenter: "dc1", + Node: "node3", }, Datacenter: "dc1", QueryIDOrName: query.Query.ID, @@ -1683,54 +1684,16 @@ func TestPreparedQuery_Execute(t *testing.T) { } } - // Check that if NearRequested is passed as true, that we sort based - // on the given node and do not use the one stored in the PQ. - { - req := structs.PreparedQueryExecuteRequest{ - Source: structs.QuerySource{ - Datacenter: "dc1", - Node: "node1", - NearRequested: true, - }, - Datacenter: "dc1", - QueryIDOrName: query.Query.ID, - QueryOptions: structs.QueryOptions{Token: execToken}, - } - - var reply structs.PreparedQueryExecuteResponse - - // We just want to check that we get a non-local node, because - // the local node is the only one with working coordinates. - shuffled := false - for i := 0; i < 10; i++ { - if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { - t.Fatalf("err: %v", err) - } - if n := len(reply.Nodes); n != 10 { - t.Fatalf("expect 10 nodes, got: %d", n) - } - if node := reply.Nodes[0].Node.Node; node != "node3" { - shuffled = true - break - } - } - if !shuffled { - t.Fatal("expect non-local results") - } - } - - // Set the query to prefer a colocated service using the magic _agent token + // Bake the magic "_agent" flag into the query. query.Query.Service.Near = "_agent" if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Apply", &query, &query.Query.ID); err != nil { t.Fatalf("err: %v", err) } - // Check that the node returned is the one we asked for in the - // query source. This proves that if the PQ has "_agent" baked - // in, we always use the passed-in node. + // Check that we sort the local agent first when the magic flag is set. { req := structs.PreparedQueryExecuteRequest{ - Source: structs.QuerySource{ + Agent: structs.QuerySource{ Datacenter: "dc1", Node: "node3", }, @@ -1754,14 +1717,55 @@ func TestPreparedQuery_Execute(t *testing.T) { } } + // Check that the query isn't just sorting "node3" first because we + // provided it in the Agent query source. Proves that we use the + // Agent source when the magic "_agent" flag is passed. + { + req := structs.PreparedQueryExecuteRequest{ + Agent: structs.QuerySource{ + Datacenter: "dc1", + Node: "foo", + }, + Datacenter: "dc1", + QueryIDOrName: query.Query.ID, + QueryOptions: structs.QueryOptions{Token: execToken}, + } + + var reply structs.PreparedQueryExecuteResponse + + // Expect the set to be shuffled since we have no coordinates + // on the "foo" node. + shuffled := false + for i := 0; i < 10; i++ { + if err := msgpackrpc.CallWithCodec(codec1, "PreparedQuery.Execute", &req, &reply); err != nil { + t.Fatalf("err: %v", err) + } + if n := len(reply.Nodes); n != 10 { + t.Fatalf("expect 10 nodes, got: %d", n) + } + if node := reply.Nodes[0].Node.Node; node != "node3" { + shuffled = true + break + } + } + + if !shuffled { + t.Fatal("expect nodes to be shuffled") + } + } + // Shuffles if the response comes from a non-local DC. Proves that the - // near parameter does not affect this order. + // agent query source does not interfere with the order. { req := structs.PreparedQueryExecuteRequest{ Source: structs.QuerySource{ Datacenter: "dc2", Node: "node3", }, + Agent: structs.QuerySource{ + Datacenter: "dc1", + Node: "node3", + }, Datacenter: "dc1", QueryIDOrName: query.Query.ID, QueryOptions: structs.QueryOptions{Token: execToken}, diff --git a/consul/structs/prepared_query.go b/consul/structs/prepared_query.go index e79a178822..6677544934 100644 --- a/consul/structs/prepared_query.go +++ b/consul/structs/prepared_query.go @@ -182,6 +182,10 @@ type PreparedQueryExecuteRequest struct { // network coordinates. Source QuerySource + // Agent is used to carry around a reference to the agent which initiated + // the execute request. Used to distance-sort relative to the local node. + Agent QuerySource + // QueryOptions (unfortunately named here) controls the consistency // settings for the query lookup itself, as well as the service lookups. QueryOptions diff --git a/consul/structs/structs.go b/consul/structs/structs.go index 565b1548c3..4f99399b38 100644 --- a/consul/structs/structs.go +++ b/consul/structs/structs.go @@ -196,11 +196,6 @@ func (r *DeregisterRequest) RequestDatacenter() string { type QuerySource struct { Datacenter string Node string - - // NearRequested indicates where the values in this QuerySource came - // from. When true, the values were provided by the requestor, - // otherwise they were filled by the agent servicing the request. - NearRequested bool } // DCSpecificRequest is used to query about a specific DC From 00819e89de00262d9f16a622f2235a7987192a70 Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Fri, 1 Jul 2016 09:46:26 -0700 Subject: [PATCH 11/15] agent: test that DNS passes the agent data through --- command/agent/dns_test.go | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/command/agent/dns_test.go b/command/agent/dns_test.go index 863a0bfefb..1182d9172e 100644 --- a/command/agent/dns_test.go +++ b/command/agent/dns_test.go @@ -3166,3 +3166,37 @@ func TestDNS_InvalidQueries(t *testing.T) { } } } + +func TestDNS_PreparedQuery_AgentSource(t *testing.T) { + dir, srv := makeDNSServer(t) + defer os.RemoveAll(dir) + defer srv.agent.Shutdown() + + testutil.WaitForLeader(t, srv.agent.RPC, "dc1") + + m := MockPreparedQuery{} + if err := srv.agent.InjectEndpoint("PreparedQuery", &m); err != nil { + t.Fatalf("err: %v", err) + } + + m.executeFn = func(args *structs.PreparedQueryExecuteRequest, reply *structs.PreparedQueryExecuteResponse) error { + // Check that the agent inserted its self-name and datacenter to + // the RPC request body. + if args.Agent.Datacenter != srv.agent.config.Datacenter || + args.Agent.Node != srv.agent.config.NodeName { + t.Fatalf("bad: %#v", args.Agent) + } + return nil + } + + { + m := new(dns.Msg) + m.SetQuestion("foo.query.consul.", dns.TypeSRV) + + c := new(dns.Client) + addr, _ := srv.agent.config.ClientListener("", srv.agent.config.Ports.DNS) + if _, _, err := c.Exchange(m, addr.String()); err != nil { + t.Fatalf("err: %v", err) + } + } +} From 7fd0c3ce70d97840041e4f39585a8918fc056642 Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Fri, 1 Jul 2016 10:04:58 -0700 Subject: [PATCH 12/15] agent: parseSource still subs for _agent --- command/agent/http.go | 8 +++++++- command/agent/http_test.go | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/command/agent/http.go b/command/agent/http.go index 0cfbafbe14..92247ef2c8 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -531,7 +531,13 @@ func (s *HTTPServer) parseToken(req *http.Request, token *string) { // DC in the request, if given, or else the agent's DC. func (s *HTTPServer) parseSource(req *http.Request, source *structs.QuerySource) { s.parseDC(req, &source.Datacenter) - source.Node = req.URL.Query().Get("near") + if node := req.URL.Query().Get("near"); node != "" { + if node == "_agent" { + source.Node = s.agent.config.NodeName + } else { + source.Node = node + } + } } // parse is a convenience method for endpoints that need diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 231fd43285..b6618977f3 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -345,8 +345,8 @@ func TestParseSource(t *testing.T) { defer srv.Shutdown() defer srv.agent.Shutdown() - // Default is agent's DC and no node (since the user didn't care, - // then just give them the cheapest possible query). + // Default is agent's DC and no node (since the user didn't care, then + // just give them the cheapest possible query). req, err := http.NewRequest("GET", "/v1/catalog/nodes", nil) if err != nil { @@ -382,6 +382,18 @@ func TestParseSource(t *testing.T) { if source.Datacenter != "foo" || source.Node != "bob" { t.Fatalf("bad: %v", source) } + + // The magic "_agent" node name will use the agent's local node name. + req, err = http.NewRequest("GET", + "/v1/catalog/nodes?near=_agent", nil) + if err != nil { + t.Fatalf("err: %v", err) + } + source = structs.QuerySource{} + srv.parseSource(req, &source) + if source.Datacenter != "dc1" || source.Node != srv.agent.config.NodeName { + t.Fatalf("bad: %v", source) + } } func TestParseWait(t *testing.T) { From 01b28b9581c8950a694daeeef638ac1cbcd0b4a2 Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Fri, 1 Jul 2016 11:50:09 -0700 Subject: [PATCH 13/15] website: document near parameter of prepared queries --- .../source/docs/agent/http/query.html.markdown | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/website/source/docs/agent/http/query.html.markdown b/website/source/docs/agent/http/query.html.markdown index bad3ae1288..d2737384d9 100644 --- a/website/source/docs/agent/http/query.html.markdown +++ b/website/source/docs/agent/http/query.html.markdown @@ -70,6 +70,7 @@ query, like this example: "Name": "my-query", "Session": "adf4238a-882b-9ddc-4a9d-5b6758e4159e", "Token": "", + "Near": "node1", "Service": { "Service": "redis", "Failover": { @@ -114,6 +115,16 @@ attribute which can be set on functions. This change in effect moves Consul from using `SECURITY DEFINER` by default to `SECURITY INVOKER` by default for new Prepared Queries. + +`Near` allows specifying a particular node to sort near based on distance +sorting using [Network Coordinates](/docs/internals/coordinates.html). The +nearest instance to the specified node will be returned first, and subsequent +nodes in the response will be sorted in ascending order of estimated round-trip +times. If the node given does not exist, the nodes in the response will +be shuffled. Using the magic `_agent` value is supported, and will automatically +return results nearest the agent servicing the request. If unspecified, the +response will be shuffled by default. + The set of fields inside the `Service` structure define the query's behavior. `Service` is the name of the service to query. This is required. @@ -365,8 +376,9 @@ blocking queries, but it does support all consistency modes. Adding the optional "?near=" parameter with a node name will sort the resulting list in ascending order based on the estimated round trip time from that node. Passing "?near=_agent" will use the agent's node for the sort. If this is not -present, then the nodes will be shuffled randomly and will be in a different -order each time the query is executed. +present, the default behavior will shuffle the nodes randomly each time the +query is executed. Passing this option will override the built-in +near parameter of a prepared query, if present. An optional "?limit=" parameter can be used to limit the size of the list to the given number of nodes. This is applied after any sorting or shuffling. From 46b6726d1c64cd619a4e3cb9ca2ff8f05a2cd816 Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Fri, 1 Jul 2016 11:50:30 -0700 Subject: [PATCH 14/15] consul: mention magic _agent token in struct comments --- consul/structs/prepared_query.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/consul/structs/prepared_query.go b/consul/structs/prepared_query.go index 6677544934..5e9c31847b 100644 --- a/consul/structs/prepared_query.go +++ b/consul/structs/prepared_query.go @@ -36,7 +36,8 @@ type ServiceQuery struct { // Near allows the query to always prefer the node nearest the given // node. If the node does not exist, results are returned in their - // normal randomly-shuffled order. + // normal randomly-shuffled order. Supplying the magic "_agent" value + // is supported to sort near the agent which initiated the request. Near string // Tags are a set of required and/or disallowed tags. If a tag is in From 2b2464403f93134a05eb5946e0b223199d364aa8 Mon Sep 17 00:00:00 2001 From: Ryan Uber Date: Fri, 1 Jul 2016 12:26:14 -0700 Subject: [PATCH 15/15] website: add upgrading note for Near param in PQ's --- .../source/docs/upgrade-specific.html.markdown | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/website/source/docs/upgrade-specific.html.markdown b/website/source/docs/upgrade-specific.html.markdown index d55e397b47..1a97794a2a 100644 --- a/website/source/docs/upgrade-specific.html.markdown +++ b/website/source/docs/upgrade-specific.html.markdown @@ -14,6 +14,21 @@ details provided for their upgrades as a result of new features or changed behavior. This page is used to document those details separately from the standard upgrade flow. +## Consul 0.7 + +Consul version 0.7 adds a feature which allows prepared queries to store a +["Near" parameter](/docs/agent/http/query.html#near) in the query definition +itself. This feature enables using the distance sorting features of prepared +queries without explicitly providing the node to sort near in requests, but +requires the agent servicing a request to send additional information about +itself to the Consul servers when executing the prepared query. Agents prior +to 0.7.0 do not send this information, which means they are unable to properly +execute prepared queries configured with a `Near` parameter. Similarly, any +server nodes prior to version 0.7.0 are unable to store the `Near` parameter, +making them unable to properly serve requests for prepared queries using the +feature. It is recommended that all agents be running version 0.7.0 prior to +using this feature. + ## Consul 0.6.4 Consul 0.6.4 made some substantial changes to how ACLs work with prepared