Add retries in activator and envoy timeout to avoid 503's (#1226)

cmd/activator/main.go

@@ -19,6 +19,7 @@ import (
 	"net/http"
 	"net/http/httputil"
 	"net/url"
+	"time"
 
 	"github.com/golang/glog"
 	"github.com/knative/serving/pkg/activator"
@@ -29,10 +30,39 @@ import (
 	"k8s.io/client-go/rest"
 )
 
+const (
+	maxRetry      = 60
+	retryInterval = 1 * time.Second
+)
+
 type activationHandler struct {
 	act activator.Activator
 }
 
+// retryRoundTripper retries on 503's for up to 60 seconds. The reason is there is
+// a small delay for k8s to include the ready IP in service.
+// https://github.com/knative/serving/issues/660#issuecomment-384062553
+type retryRoundTripper struct{}
+
+func (rrt retryRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
+	transport := http.DefaultTransport
+	resp, err := transport.RoundTrip(r)
+	// TODO: Activator should retry with backoff.
+	// https://github.com/knative/serving/issues/1229
+	i := 1
+	for ; i < maxRetry; i++ {
+		if err == nil && resp != nil && resp.StatusCode != 503 {
+			break
+		}
+		resp.Body.Close()
+		time.Sleep(retryInterval)
+		resp, err = transport.RoundTrip(r)
+	}
+	// TODO: add metrics for number of tries and the response code.
+	glog.Infof("It took %d tries to get response code %d", i, resp.StatusCode)
+	return resp, nil
+}
+
 func (a *activationHandler) handler(w http.ResponseWriter, r *http.Request) {
 	namespace := r.Header.Get(controller.GetRevisionHeaderNamespace())
 	name := r.Header.Get(controller.GetRevisionHeaderName())
@@ -48,6 +78,7 @@ func (a *activationHandler) handler(w http.ResponseWriter, r *http.Request) {
 		Host:   fmt.Sprintf("%s:%d", endpoint.FQDN, endpoint.Port),
 	}
 	proxy := httputil.NewSingleHostReverseProxy(target)
+	proxy.Transport = retryRoundTripper{}
 	// TODO: Clear the host to avoid 404's.
 	// https://github.com/elafros/elafros/issues/964
 	r.Host = ""

pkg/activator/revision.go

@@ -105,13 +105,6 @@ func (r *revisionActivator) ActiveEndpoint(namespace, name string) (end Endpoint
 					} else {
 						log.Printf("Revision %s/%s is ready", rev.namespace, rev.name)
 					}
-					// After a pod goes ready, there is a poll loop to publish that fact, then there are
-					// controllers that wake up to propagate the info to each node which configures iptables on
-					// a max frequency loop, so it's always possible that there's a small delay.
-					// The delay should be O(seconds) max, most of the time.
-					// TODO: rely on readinessProbe instead of a hard-coded sleep.
-					// https://github.com/elafros/elafros/issues/974
-					time.Sleep(2 * time.Second)
 					break RevisionReady
 				} else {
 					return internalError("Unexpected result type for revision %s/%s: %v", rev.namespace, rev.name, event)

pkg/apis/istio/v1alpha2/routerule_types.go

@@ -87,7 +87,7 @@ type RouteRuleSpec struct {
 	Destination   IstioService        `json:"destination"`
 	Match         Match               `json:"match,omitempty"`
 	Route         []DestinationWeight `json:"route"`
-	AppendHeaders map[string]string   `json:"appendHeaders"`
+	AppendHeaders map[string]string   `json:"appendHeaders,omitempty"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

pkg/controller/route/istio_route.go

@@ -20,12 +20,14 @@ import (
 	"fmt"
 	"regexp"
 
-	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	istiov1alpha2 "github.com/knative/serving/pkg/apis/istio/v1alpha2"
+	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/knative/serving/pkg/controller"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const requestTimeoutMs = "60000"
+
 // makeIstioRouteSpec creates an Istio route
 func makeIstioRouteSpec(u *v1alpha1.Route, tt *v1alpha1.TrafficTarget, ns string, routes []RevisionRoute, domain string, inactiveRev string) istiov1alpha2.RouteRuleSpec {
 	destinationWeights := calculateDestinationWeights(u, tt, routes)
@@ -60,6 +62,9 @@ func makeIstioRouteSpec(u *v1alpha1.Route, tt *v1alpha1.TrafficTarget, ns string
 		appendHeaders := make(map[string]string)
 		appendHeaders[controller.GetRevisionHeaderName()] = inactiveRev
 		appendHeaders[controller.GetRevisionHeaderNamespace()] = u.Namespace
+		// Set the Envoy upstream timeout to be 60 seconds, in case the revision needs longer time to come up
+		// https://www.envoyproxy.io/docs/envoy/v1.5.0/configuration/http_filters/router_filter#x-envoy-upstream-rq-timeout-ms
+		appendHeaders["x-envoy-upstream-rq-timeout-ms"] = requestTimeoutMs
 		spec.AppendHeaders = appendHeaders
 	}
 	return spec

pkg/controller/route/istio_route_test.go

@@ -17,10 +17,10 @@ import (
 	"regexp"
 	"testing"
 
-	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
+	"github.com/google/go-cmp/cmp"
 	istiov1alpha2 "github.com/knative/serving/pkg/apis/istio/v1alpha2"
+	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/knative/serving/pkg/controller"
-	"github.com/google/go-cmp/cmp"
 )
 
 const (
@@ -77,6 +77,7 @@ func TestMakeIstioRouteSpecRevisionInactive(t *testing.T) {
 	appendHeaders := make(map[string]string)
 	appendHeaders[controller.GetRevisionHeaderName()] = testInactiveRev
 	appendHeaders[controller.GetRevisionHeaderNamespace()] = testNamespace
+	appendHeaders["x-envoy-upstream-rq-timeout-ms"] = requestTimeoutMs
 	expectedIstioRouteSpec := istiov1alpha2.RouteRuleSpec{
 		Destination: istiov1alpha2.IstioService{
 			Name:      "test-route-service",

pkg/controller/route/route_test.go

@@ -412,6 +412,7 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) {
 	appendHeaders := make(map[string]string)
 	appendHeaders[ctrl.GetRevisionHeaderName()] = "test-rev"
 	appendHeaders[ctrl.GetRevisionHeaderNamespace()] = testNamespace
+	appendHeaders["x-envoy-upstream-rq-timeout-ms"] = requestTimeoutMs
 	expectedRouteSpec := v1alpha2.RouteRuleSpec{
 		Destination: v1alpha2.IstioService{
 			Name:      "test-route-service",
@@ -509,14 +510,13 @@ func TestCreateRouteFromConfigsWithMultipleRevs(t *testing.T) {
 				Namespace: testNamespace,
 			},
 			Weight: 100,
-		}, getActivatorDestinationWeight(0),
-			{
-				Destination: v1alpha2.IstioService{
-					Name:      fmt.Sprintf("%s-service", otherRev.Name),
-					Namespace: testNamespace,
-				},
-				Weight: 0,
-			}},
+		}, getActivatorDestinationWeight(0), {
+			Destination: v1alpha2.IstioService{
+				Name:      fmt.Sprintf("%s-service", otherRev.Name),
+				Namespace: testNamespace,
+			},
+			Weight: 0,
+		}},
 	}
 
 	if diff := cmp.Diff(expectedRouteSpec, routerule.Spec); diff != "" {
@@ -646,6 +646,7 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) {
 	appendHeaders := make(map[string]string)
 	appendHeaders[ctrl.GetRevisionHeaderName()] = "test-rev"
 	appendHeaders[ctrl.GetRevisionHeaderNamespace()] = testNamespace
+	appendHeaders["x-envoy-upstream-rq-timeout-ms"] = requestTimeoutMs
 	expectedRouteSpec := v1alpha2.RouteRuleSpec{
 		Destination: v1alpha2.IstioService{
 			Name:      fmt.Sprintf("%s-service", route.Name),