Add support for retries #2038

adleong · 2019-01-03T23:22:16Z

This change adds the isRetryable property to routes in a service profile. This causes the proxy to retry failed requests to retryable routes whenever it is able to. A retry budget may also be added to the service profile which can limit the number of retries. The linkerd profile --template command has been updated to describe these new properties.

The linkerd routes command has been updated to differentiate between actual and effective RPS and success rate. This data is only available for outgoing queries i.e. when the --to flag is used. Effective metrics are the metrics as observed by the client whereas actual metrics are the metrics which describe the requests and responses which are actually sent on the wire. Adding retries will usually increase the actual RPS and increase the effective success rate while leaving the actual success rate and effective RPS unchanged.

$linkerd routes deploy/books --to svc/authors
ROUTE                     SERVICE   EFFECTIVE_SUCCESS   EFFECTIVE_RPS   ACTUAL_SUCCESS   ACTUAL_RPS   LATENCY_P50   LATENCY_P95   LATENCY_P99
HEAD /authors/{id}.json   authors              68.28%          2.4rps           45.21%       3.6rps          14ms          40ms          48ms
[UNKNOWN]                 authors               0.00%          0.0rps            0.00%       0.0rps           0ms           0ms           0ms

adleong · 2019-01-03T23:30:57Z

How I tested

Install Linkerd control plane and booksapp
Run linkerd routes deploy/webapp --to svc/books, notice: no useful data
Apply service profiles from swagger:

curl https://run.linkerd.io/booksapp/webapp.swagger | linkerd profile --open-api - webapp | kubectl apply -f -
curl https://run.linkerd.io/booksapp/authors.swagger | linkerd profile --open-api - authors | kubectl apply -f -
curl https://run.linkerd.io/booksapp/books.swagger | linkerd profile --open-api - books | kubectl apply -f -

Run linkerd routes deploy/webapp --to svc/books: Notice SR on the POST and PUT routes is ~50%
Run linkerd routes deploy/books --to svc/authors: Notice SR on the HEAD route is ~50%
Download the authors profile: kubectl -n linkerd get sp/authors.default.svc.cluster.local -o yaml > authors-profile.yaml
Edit the HEAD route to be retryable:

  - condition:
      method: HEAD
      pathRegex: /authors/[^/]*\.json
    name: HEAD /authors/{id}.json
    isRetryable: true

Apply the new profile kubectl apply -f authors-profile.yaml
Run linkerd routes deploy/books --to svc/authors notice effective SR go to 100%
Run linkerd routes deploy/webapp --to svc/books notice SR go to 100%

seanmonstar · 2019-01-05T04:30:37Z

pkg/profiles/profiles.go

+		routes = append(routes, pbRoute)
+	}
+	budget := DefaultRetryBudget
+	if profile.RetryBudget != nil {


Sorry if this is addressed elsewhere and I couldn't find it: would this be an appropriate place to validate the budget arguments? Would an error here be more visible to a user?

The proxy will already disregard invalid budgets and log at WARN level, which should be visible, but only if the user is looking at the proxy logs...

Good point. Probably a good idea to validate here too. Unfortunately, the suffers from the same problem that validation errors will only be visible if you're looking at the logs.

We may be able to some limited validation with CRD schema validation too...

Is it possible for linkerd inject or other related command to do the validation then?

It'd be a great addition to the inject checks.

grampelberg · 2019-01-09T18:21:35Z

@adleong I can't get stat to work on this branch. Installed booksapp, looked at stat and there's nothing there.

adleong · 2019-01-09T19:17:24Z

@grampelberg uh oh. can you share a screenshot of the stat command and output?

grampelberg · 2019-01-09T20:44:22Z

$ bin/go-run cli -n booksapp stat deploy
NAME      MESHED   SUCCESS   RPS   LATENCY_P50   LATENCY_P95   LATENCY_P99   TLS
authors      1/1         -     -             -             -             -     -
books        1/1         -     -             -             -             -     -
traffic      1/1         -     -             -             -             -     -
webapp       3/3         -     -             -             -             -     -

grampelberg · 2019-01-09T22:16:19Z

I've replicated my stat bug on stable, so it isn't your code! I'm going to go figure out what the deal is there now .....

jon-walton · 2019-01-10T09:33:16Z

is it feasible to read the IsRetryable from an openapi spec? The spec supports extensions (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#specificationExtensions) as part of the operation object (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#operationObject).

We could use something like x-linkerd-is-retryable

The reason I ask this is that our openapi spec is auto generated based on the routes in the controllers. It would be annoying to maintain a separate list of routes and their linkerd settings to then patch the json before passing into the linkerd cli

grampelberg · 2019-01-11T23:09:19Z

What do you think about reporting effective by default and adding a -o wide option?

bin/go-run cli -n booksapp routes deploy/books --to svc/authors
ROUTE                     SERVICE   SUCCESS   RPS   LATENCY_P50   LATENCY_P95   LATENCY_P99
HEAD /authors/{id}.json   authors   100.00%  2.2rps         5ms          16ms          19ms
[UNKNOWN]                 authors     0.00%  0.0rps         0ms           0ms           0ms

bin/go-run cli -n booksapp routes deploy/books --to svc/authors -o wide
ROUTE                     SERVICE   SUCCESS   RPS   ACTUAL_SUCCESS  ACTUAL_RPS  LATENCY_P50   LATENCY_P95   LATENCY_P99
HEAD /authors/{id}.json   authors   100.00%  2.2rps         55.14%      4.0rps          5ms          16ms          19ms
[UNKNOWN]                 authors     0.00%  0.0rps          0.00%      0.0rps          0ms           0ms           0ms

adleong · 2019-01-11T23:19:31Z

@jon-walton I love this idea! It should be pretty simple to add this to the linkerd profile --open-api command. Would you mind opening an issue?

adleong · 2019-01-11T23:20:30Z

@grampelberg I dig it. ⛏

grampelberg · 2019-01-11T23:26:43Z

@adleong also see #2072 if you wanted to tackle that as a separate PR.

Signed-off-by: Alex Leong <alex@buoyant.io>

adleong · 2019-01-14T22:02:48Z

@grampelberg I implemented your -o wide suggestion. PTAL

grampelberg · 2019-01-15T00:06:22Z

Oh yeah, I like that!

siggy

looks great. thanks for the test setup instructions.

siggy · 2019-01-15T21:05:36Z

cli/cmd/root.go

@@ -162,17 +162,17 @@ func newStatOptionsBase() *statOptionsBase {

 func (o *statOptionsBase) validateOutputFormat() error {
 	switch o.outputFormat {
-	case "table", "json", "":
+	case "table", "json", "wide", "":


is it possible to implement the EFFECTIVE/ACTUAL columns for the linkerd stat command? if not, it's a bit confusing that stat -o wide works but is a no-op.

related, if routes -o wide only works with --to, consider validating that, or maybe documenting it in the help.

siggy · 2019-01-15T21:07:09Z

cli/cmd/root.go

+		proxyOutboundCapacity: map[string]uint{},
+		proxyCPURequest:       "",
+		proxyMemoryRequest:    "",
+		tls:                   "",
 		disableExternalProfiles: false,


i think this is a go-fmt issue? when i save this file in vscode it re-aligns the field assignments.

siggy · 2019-01-15T21:17:54Z

cli/cmd/routes.go

-		"SUCCESS",
-		"RPS",
+	}
+	if options.toResource == "" || options.outputFormat != "wide" {


consider:

outputActual := options.toResource != "" && options.outputFormat == "wide"

... and then use outputActual in the 3 subsequent if statements.

siggy · 2019-01-15T22:09:16Z

controller/api/public/top_routes.go

@@ -13,6 +13,7 @@ import (

 const (
 	routeReqQuery             = "sum(increase(route_response_total%s[%s])) by (%s, dst, classification)"
+	actualRouteReqQuery       = "sum(increase(route_actual_response_total%s[%s])) by (%s, dst, classification)"


at the prometheus metrics level and BasicStats level, we're re-using the existing response keys for "effective" stats, and adding new keys for "actual". higher up in the abstractions, around jsonRouteStats and cli table headers, we're instead adding both new "effective" and "actual" keys, and leaving the existing keys empty. what do you think about modifying the higher-level abstractions to match these lower-level ones, such that we'd remove "effective" altogether?

I think the only place that we have "effective" keys and leave the existing keys empty is in the struct that will be serialized to json. I do this so that we can have different field names: effective_rps in the outbound case and just plain rps in the inbound case.

Signed-off-by: Alex Leong <alex@buoyant.io>

siggy

👍 🚢

klingerf

⭐️ 🚀 Awesome! So cool to see these stats wired up.

$ bin/linkerd routes deploy/books --to svc/authors -owide
ROUTE                     SERVICE   EFFECTIVE_SUCCESS   EFFECTIVE_RPS   ACTUAL_SUCCESS   ACTUAL_RPS   LATENCY_P50   LATENCY_P95   LATENCY_P99
HEAD /authors/{id}.json   authors             100.00%          2.2rps           51.16%       4.3rps          12ms          30ms          38ms

klingerf · 2019-01-16T19:54:50Z

cli/cmd/routes.go

 func buildTopRoutesRequest(resource string, options *routesOptions) (*pb.TopRoutesRequest, error) {
-	err := options.validateOutputFormat()
+	err := validateOutputFormat(options)


TIOLI -- you could still use *routesOptions as the receiver for this method, even though it embeds statOptionsBase, which has its own definition of validateOutputFormat. Something like this would work:

diff --git a/cli/cmd/routes.go b/cli/cmd/routes.go index bab9fd36..9c3dceb4 100644 --- a/cli/cmd/routes.go +++ b/cli/cmd/routes.go @@ -268,12 +268,12 @@ func printRouteJSON(stats []*routeRowStats, w *tabwriter.Writer, options *routes fmt.Fprintf(w, "%s\n", b) } -func validateOutputFormat(options *routesOptions) error { - switch options.outputFormat { +func (o *routesOptions) validateOutputFormat() error { + switch o.outputFormat { case "table", "json", "": return nil case "wide": - if options.toResource == "" { + if o.toResource == "" { return errors.New("wide output is only available when --to is specified") } return nil @@ -283,7 +283,7 @@ func validateOutputFormat(options *routesOptions) error { } func buildTopRoutesRequest(resource string, options *routesOptions) (*pb.TopRoutesRequest, error) { - err := validateOutputFormat(options) + err := options.validateOutputFormat() if err != nil { return nil, err }

Depends on #2037, #2038, #2063, #2066, #2086, #2087, #2089 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

Signed-off-by: Alex Leong <alex@buoyant.io>

Depends on #2037, #2038, #2063, #2066, #2087, #2089, #2105 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

adleong self-assigned this Jan 3, 2019

adleong requested review from grampelberg and olix0r January 3, 2019 23:22

seanmonstar reviewed Jan 5, 2019

View reviewed changes

jon-walton mentioned this pull request Jan 14, 2019

add support for configuring service profile retries via openapi spec #2073

Closed

adleong added 2 commits January 14, 2019 10:36

Retries integration

a81f82e

Signed-off-by: Alex Leong <alex@buoyant.io>

Add -o wide output format

d87b6cc

Signed-off-by: Alex Leong <alex@buoyant.io>

adleong force-pushed the alex/retry-two branch from 534aeda to d87b6cc Compare January 14, 2019 21:51

siggy self-requested a review January 15, 2019 21:02

siggy reviewed Jan 15, 2019

View reviewed changes

adleong added 3 commits January 15, 2019 16:47

Review feedback

593482e

Signed-off-by: Alex Leong <alex@buoyant.io>

Fix indentation

6308155

Signed-off-by: Alex Leong <alex@buoyant.io>

Fix typo

3f67360

Signed-off-by: Alex Leong <alex@buoyant.io>

siggy approved these changes Jan 16, 2019

View reviewed changes

klingerf approved these changes Jan 16, 2019

View reviewed changes

siggy added a commit that referenced this pull request Jan 16, 2019

Add release notes for edge-19.1.2 release

8e1be57

Depends on #2037, #2038, #2063, #2066, #2086, #2087, #2089 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy mentioned this pull request Jan 16, 2019

Add release notes for edge-19.1.2 release #2096

Merged

siggy added a commit that referenced this pull request Jan 16, 2019

Add release notes for edge-19.1.2 release

dbf546f

Depends on #2037, #2038, #2063, #2066, #2086, #2087, #2089 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

Review feedback

2ef5c0b

Signed-off-by: Alex Leong <alex@buoyant.io>

adleong merged commit 771542d into master Jan 16, 2019

adleong mentioned this pull request Jan 16, 2019

Introduce -o wide for routes command #2072

Closed

siggy added a commit that referenced this pull request Jan 17, 2019

Add release notes for edge-19.1.2 release (#2096)

50d181c

Depends on #2037, #2038, #2063, #2066, #2087, #2089, #2105 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

adleong deleted the alex/retry-two branch December 13, 2019 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for retries #2038

Add support for retries #2038

adleong commented Jan 3, 2019

adleong commented Jan 3, 2019

seanmonstar Jan 5, 2019

adleong Jan 7, 2019

seanmonstar Jan 7, 2019

grampelberg Jan 9, 2019

grampelberg commented Jan 9, 2019

adleong commented Jan 9, 2019

grampelberg commented Jan 9, 2019

grampelberg commented Jan 9, 2019

jon-walton commented Jan 10, 2019

grampelberg commented Jan 11, 2019

adleong commented Jan 11, 2019

adleong commented Jan 11, 2019

grampelberg commented Jan 11, 2019

adleong commented Jan 14, 2019

grampelberg commented Jan 15, 2019

siggy left a comment

siggy Jan 15, 2019

siggy Jan 15, 2019

siggy Jan 15, 2019

siggy Jan 15, 2019

adleong Jan 16, 2019

siggy left a comment

klingerf left a comment

klingerf Jan 16, 2019

Add support for retries #2038

Add support for retries #2038

Conversation

adleong commented Jan 3, 2019

adleong commented Jan 3, 2019

How I tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grampelberg commented Jan 9, 2019

adleong commented Jan 9, 2019

grampelberg commented Jan 9, 2019

grampelberg commented Jan 9, 2019

jon-walton commented Jan 10, 2019

grampelberg commented Jan 11, 2019

adleong commented Jan 11, 2019

adleong commented Jan 11, 2019

grampelberg commented Jan 11, 2019

adleong commented Jan 14, 2019

grampelberg commented Jan 15, 2019

siggy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siggy left a comment

Choose a reason for hiding this comment

klingerf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment