Adding Concurrency in Pool Manager (#1698)

Concurrency in the pool manager allows specializing pods concurrently based on a specified limit. 

Co-authored-by: Vishal <vishal-biyani@users.noreply.github.com>

pkg/router/functionHandler.go

@@ -35,7 +35,6 @@ import (
 	k8stypes "k8s.io/apimachinery/pkg/types"
 
 	fv1 "github.com/fission/fission/pkg/apis/core/v1"
-	"github.com/fission/fission/pkg/crd"
 	ferror "github.com/fission/fission/pkg/error"
 	"github.com/fission/fission/pkg/error/network"
 	executorClient "github.com/fission/fission/pkg/executor/client"
@@ -76,8 +75,8 @@ type (
 		// svcAddrRetryCount is the max times for RetryingRoundTripper to retry with a specific service address
 		// Router sends requests to a specific service address for each function.
 		// A service address is considered as an invalid one if amount of non-network
-		// errors router received is higher than svcAddrRetryCount. In this situation,
-		// remove it from cache and try to get a new one from executor.
+		// errors router received is higher than svcAddrRetryCount.
+		// Try to get a new one from executor.
 		// Default svcAddrRetryCount is 5.
 		svcAddrRetryCount int
 	}
@@ -98,11 +97,6 @@ type (
 	fakeCloseReadCloser struct {
 		io.ReadCloser
 	}
-
-	svcEntryRecord struct {
-		svcUrl    *url.URL
-		fromCache bool
-	}
 )
 
 func init() {
@@ -148,10 +142,9 @@ func (w *fakeCloseReadCloser) RealClose() error {
 // Earlier, GetServiceForFunction was called inside handler function and fission explicitly set http status code to 500
 // if it returned an error.
 func (roundTripper *RetryingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
-	// Set forwarded host header if not exists
+	// set the timeout for transport context
 	roundTripper.addForwardedHostHeader(req)
 
-	// set the timeout for transport context
 	transport := roundTripper.getDefaultTransport()
 	ocRoundTripper := &ochttp.Transport{Base: transport}
 
@@ -190,11 +183,16 @@ func (roundTripper *RetryingRoundTripper) RoundTrip(req *http.Request) (*http.Re
 		// trying to get new service url from cache/executor.
 		if retryCounter == 0 {
 			// get function service url from cache or executor
-			roundTripper.serviceUrl, roundTripper.urlFromCache, err = roundTripper.funcHandler.getServiceEntry()
+			roundTripper.serviceUrl, err = roundTripper.funcHandler.getServiceEntryFromExecutor()
 			if err != nil {
 				// We might want a specific error code or header for fission failures as opposed to
 				// user function bugs.
 				statusCode, errMsg := ferror.GetHTTPError(err)
+				// if statusCode == http.StatusTooManyRequests {
+				// 	time.Sleep(executingTimeout)
+				// 	executingTimeout = executingTimeout * time.Duration(roundTripper.funcHandler.tsRoundTripperParams.timeoutExponent)
+				// 	continue
+				// } else {
 				if roundTripper.funcHandler.isDebugEnv {
 					return &http.Response{
 						StatusCode:    statusCode,
@@ -208,18 +206,14 @@ func (roundTripper *RetryingRoundTripper) RoundTrip(req *http.Request) (*http.Re
 					}, nil
 				}
 				return nil, ferror.MakeError(http.StatusInternalServerError, err.Error())
+				// }
 			}
-
-			// service url maybe nil if router cannot find one in cache,
-			// so here we retry to get service url again
-			if roundTripper.serviceUrl == nil {
-				time.Sleep(executingTimeout)
-				executingTimeout = executingTimeout * time.Duration(roundTripper.funcHandler.tsRoundTripperParams.timeoutExponent)
-				continue
+			if roundTripper.funcHandler.function.Spec.InvokeStrategy.ExecutionStrategy.ExecutorType == fv1.ExecutorTypePoolmgr {
+				defer roundTripper.funcHandler.unTapService(roundTripper.funcHandler.function, roundTripper.serviceUrl)
 			}
 
 			// modify the request to reflect the service url
-			// this service url may have come from the cache lookup or from executor response
+			// this service url comes from executor response
 			req.URL.Scheme = roundTripper.serviceUrl.Scheme
 			req.URL.Host = roundTripper.serviceUrl.Host
 
@@ -254,7 +248,7 @@ func (roundTripper *RetryingRoundTripper) RoundTrip(req *http.Request) (*http.Re
 			return resp, nil
 		}
 
-		roundTripper.totalRetry += 1
+		roundTripper.totalRetry++
 
 		if i >= roundTripper.funcHandler.tsRoundTripperParams.maxRetries-1 {
 			// return here if we are in the last round
@@ -276,6 +270,7 @@ func (roundTripper *RetryingRoundTripper) RoundTrip(req *http.Request) (*http.Re
 
 		// if transport.RoundTrip returns a non-network dial error (e.g. "context canceled"), then relay it back to user
 		if !isNetDialErr {
+			roundTripper.logger.Error("encountered non-network dial error", zap.Error(err))
 			return resp, err
 		}
 
@@ -285,21 +280,7 @@ func (roundTripper *RetryingRoundTripper) RoundTrip(req *http.Request) (*http.Re
 		}
 
 		// Check whether an error is an timeout error ("dial tcp i/o timeout").
-		// If it's not a timeout error or retryCounter exceeded pre-defined threshold,
-		// we assume the entry in router cache is stale, invalidate it.
-		if !isNetTimeoutErr || retryCounter >= roundTripper.funcHandler.tsRoundTripperParams.svcAddrRetryCount {
-			if roundTripper.urlFromCache {
-				// if transport.RoundTrip returns a network dial error and serviceUrl was from cache,
-				// it means, the entry in router cache is stale, so invalidate it.
-				roundTripper.logger.Debug("request errored out - removing function from router's cache and requesting a new service for function",
-					zap.String("url", req.URL.Host),
-					zap.String("function_name", fnMeta.Name),
-					zap.Error(err))
-
-				roundTripper.funcHandler.fmap.remove(fnMeta)
-			}
-			retryCounter = 0
-		} else {
+		if isNetTimeoutErr {
 			roundTripper.logger.Debug("request errored out - backing off before retrying",
 				zap.String("url", req.URL.Host),
 				zap.String("function_name", fnMeta.Name),
@@ -307,6 +288,14 @@ func (roundTripper *RetryingRoundTripper) RoundTrip(req *http.Request) (*http.Re
 			retryCounter++
 		}
 
+		// If it's not a timeout error or retryCounter exceeded pre-defined threshold,
+		if retryCounter >= roundTripper.funcHandler.tsRoundTripperParams.svcAddrRetryCount {
+			roundTripper.logger.Debug(fmt.Sprintf(
+				"retry counter exceeded pre-defined threshold of %v",
+				roundTripper.funcHandler.tsRoundTripperParams.svcAddrRetryCount))
+			retryCounter = 0
+		}
+
 		roundTripper.logger.Debug("Backing off before retrying", zap.Any("backoff_time", executingTimeout), zap.Error(err))
 		time.Sleep(executingTimeout)
 		executingTimeout = executingTimeout * time.Duration(roundTripper.funcHandler.tsRoundTripperParams.timeoutExponent)
@@ -478,12 +467,12 @@ func (roundTripper RetryingRoundTripper) addForwardedHostHeader(req *http.Reques
 	// Format of req.Host is <host>:<port>
 	// We need to extract hostname from it, than
 	// check whether a host is ipv4 or ipv6 or FQDN
-	reqUrl := fmt.Sprintf("%s://%s", req.Proto, req.Host)
-	u, err := url.Parse(reqUrl)
+	reqURL := fmt.Sprintf("%s://%s", req.Proto, req.Host)
+	u, err := url.Parse(reqURL)
 	if err != nil {
 		roundTripper.logger.Error("error parsing request url while adding forwarded host headers",
 			zap.Error(err),
-			zap.String("url", reqUrl))
+			zap.String("url", reqURL))
 		return
 	}
 
@@ -508,100 +497,35 @@ func (roundTripper RetryingRoundTripper) addForwardedHostHeader(req *http.Reques
 	req.Header.Set(X_FORWARDED_HOST, req.Host)
 }
 
-// getServiceEntry is a short-hand for developers to get service url entry that may returns from executor or cache
-func (fh *functionHandler) getServiceEntry() (serviceUrl *url.URL, serviceUrlFromCache bool, err error) {
-	// try to find service url from cache first
-	serviceUrl, err = fh.getServiceEntryFromCache()
-	if err == nil && serviceUrl != nil {
-		return serviceUrl, true, nil
-	} else if err != nil {
-		return nil, false, err
-	}
-
-	// cache miss or nil entry in cache
+// unTapservice marks the serviceURL in executor's cache as inactive, so that it can be reused
+func (fh functionHandler) unTapService(fn *fv1.Function, serviceUrl *url.URL) error {
+	fh.logger.Info("UnTapService Called")
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 	defer cancel()
-
-	fnMeta := &fh.function.ObjectMeta
-
-	// Use throttle to limit the total amount of requests sent
-	// to the executor to prevent it from overloaded.
-	recordObj, err := fh.svcAddrUpdateThrottler.RunOnce(
-		crd.CacheKey(fnMeta),
-		func(firstToTheLock bool) (interface{}, error) {
-			var u *url.URL
-			// Get service entry from executor and update cache if its the first goroutine
-			if firstToTheLock { // first to the service url
-				fh.logger.Debug("calling getServiceForFunction",
-					zap.String("function_name", fnMeta.Name))
-				u, err = fh.getServiceEntryFromExecutor(ctx)
-				if err != nil {
-					fh.logger.Error("error getting service url from executor",
-						zap.Error(err),
-						zap.String("function_name", fnMeta.Name))
-					return nil, err
-				}
-				// add the address in router's cache
-				fh.logger.Info("assigning service url for function",
-					zap.String("url", u.String()),
-					zap.String("function_name", fnMeta.Name))
-				fh.fmap.assign(fnMeta, u)
-			} else {
-				u, err = fh.getServiceEntryFromCache()
-				if err != nil {
-					return nil, err
-				}
-			}
-
-			return svcEntryRecord{
-				svcUrl:    u,
-				fromCache: firstToTheLock,
-			}, err
-		},
-	)
+	err := fh.executor.UnTapService(ctx, fn.ObjectMeta, fn.Spec.InvokeStrategy.ExecutionStrategy.ExecutorType, serviceUrl)
 	if err != nil {
-		e := "error updating service address entry for function"
-		fh.logger.Error(e,
+		statusCode, errMsg := ferror.GetHTTPError(err)
+		fh.logger.Error("error from UnTapService",
 			zap.Error(err),
-			zap.String("function_name", fnMeta.Name),
-			zap.String("function_namespace", fnMeta.Namespace))
-		return nil, false, errors.Wrapf(err, "%s %s_%s", e, fnMeta.Name, fnMeta.Namespace)
+			zap.String("error_message", errMsg),
+			zap.Any("function", fh.function),
+			zap.Int("status_code", statusCode))
+		return err
 	}
-
-	record, ok := recordObj.(svcEntryRecord)
-	if !ok {
-		return nil, false, errors.Errorf("Received unknown service record type")
-	}
-
-	return record.svcUrl, record.fromCache, nil
-}
-
-// getServiceEntryFromCache returns service url entry returns from cache
-func (fh functionHandler) getServiceEntryFromCache() (serviceUrl *url.URL, err error) {
-	// cache lookup to get serviceUrl
-	serviceUrl, err = fh.fmap.lookup(&fh.function.ObjectMeta)
-	if err != nil {
-		var errMsg string
-
-		e, ok := err.(ferror.Error)
-		if !ok {
-			errMsg = fmt.Sprintf("Unknown error when looking up service entry: %v", err)
-		} else {
-			// Ignore ErrorNotFound error here, it's an expected error,
-			// roundTripper will try to get service url later.
-			if e.Code == ferror.ErrorNotFound {
-				return nil, nil
-			}
-			errMsg = fmt.Sprintf("Error getting function %v;s service entry from cache: %v", fh.function.ObjectMeta.Name, err)
-		}
-		return nil, ferror.MakeError(http.StatusInternalServerError, errMsg)
-	}
-	return serviceUrl, nil
+	return nil
 }
 
 // getServiceEntryFromExecutor returns service url entry returns from executor
-func (fh functionHandler) getServiceEntryFromExecutor(ctx context.Context) (*url.URL, error) {
+func (fh functionHandler) getServiceEntryFromExecutor() (*url.URL, error) {
 	// send a request to executor to specialize a new pod
+	fh.logger.Debug("function timeout specified", zap.Int("timeout", fh.function.Spec.FunctionTimeout))
+	timeout := 30 * time.Second
+	if fh.function.Spec.FunctionTimeout > 0 {
+		timeout = time.Second * time.Duration(fh.function.Spec.FunctionTimeout)
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
+	defer cancel()
 	service, err := fh.executor.GetServiceForFunction(ctx, &fh.function.ObjectMeta)
 	if err != nil {
 		statusCode, errMsg := ferror.GetHTTPError(err)

pkg/router/functionHandler_test.go

@@ -19,10 +19,8 @@ package router
 import (
 	"context"
 	"errors"
-	"log"
 	"net/http"
 	"net/http/httptest"
-	"net/url"
 	"testing"
 	"time"
 
@@ -34,72 +32,6 @@ import (
 	fv1 "github.com/fission/fission/pkg/apis/core/v1"
 )
 
-func createBackendService(testResponseString string) *url.URL {
-	backendServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.Write([]byte(testResponseString))
-	}))
-
-	backendURL, err := url.Parse(backendServer.URL)
-	if err != nil {
-		panic("error parsing url")
-	}
-	return backendURL
-}
-
-/*
-   1. Create a service at some URL
-   2. Add it to the function service map
-   3. Create a http server with some trigger url pointed at function handler
-   4. Send a request to that server, ensure it reaches the first service.
-*/
-func TestFunctionProxying(t *testing.T) {
-	testResponseString := "hi"
-	backendURL := createBackendService(testResponseString)
-	log.Printf("Created backend svc at %v", backendURL)
-
-	fnMeta := metav1.ObjectMeta{Name: "foo", Namespace: metav1.NamespaceDefault}
-
-	config := zap.NewDevelopmentConfig()
-	config.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder
-	logger, err := config.Build()
-
-	panicIf(err)
-
-	fmap := makeFunctionServiceMap(logger, 0)
-	fmap.assign(&fnMeta, backendURL)
-
-	httpTrigger := &fv1.HTTPTrigger{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:            "xxx",
-			Namespace:       metav1.NamespaceDefault,
-			ResourceVersion: "1234",
-		},
-		Spec: fv1.HTTPTriggerSpec{
-			FunctionReference: fv1.FunctionReference{
-				Type: fv1.FunctionReferenceTypeFunctionName,
-			},
-		},
-	}
-
-	fh := &functionHandler{
-		logger: logger,
-		fmap:   fmap,
-		function: &fv1.Function{
-			ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: metav1.NamespaceDefault},
-		},
-		tsRoundTripperParams: &tsRoundTripperParams{
-			timeout:         50 * time.Millisecond,
-			timeoutExponent: 2,
-			maxRetries:      10,
-		},
-		httpTrigger: httpTrigger,
-	}
-	functionHandlerServer := httptest.NewServer(http.HandlerFunc(fh.handler))
-	fhURL := functionHandlerServer.URL
-
-	testRequest(fhURL, testResponseString)
-}
-
 func TestProxyErrorHandler(t *testing.T) {
 	config := zap.NewDevelopmentConfig()
 	config.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder

pkg/router/functionServiceMap.go

@@ -77,8 +77,3 @@ func (fmap *functionServiceMap) assign(f *metav1.ObjectMeta, serviceUrl *url.URL
 		// ignore error
 	}
 }
-
-func (fmap *functionServiceMap) remove(f *metav1.ObjectMeta) error {
-	mk := keyFromMetadata(f)
-	return fmap.cache.Delete(*mk)
-}

pkg/router/router_test.go

@@ -1,108 +0,0 @@
-/*
-Copyright 2016 The Fission Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package router
-
-import (
-	"context"
-	"fmt"
-	"testing"
-	"time"
-
-	"go.uber.org/zap"
-	"go.uber.org/zap/zapcore"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
-	fv1 "github.com/fission/fission/pkg/apis/core/v1"
-	"github.com/fission/fission/pkg/throttler"
-)
-
-func TestRouter(t *testing.T) {
-	// metadata for a fake function
-	fnMeta := metav1.ObjectMeta{Name: "foo", Namespace: metav1.NamespaceDefault}
-
-	// and a reference to it
-	fr := fv1.FunctionReference{
-		Type: fv1.FunctionReferenceTypeFunctionName,
-		Name: fnMeta.Name,
-	}
-
-	// start a fake service
-	testResponseString := "hi"
-	testServiceUrl := createBackendService(testResponseString)
-
-	config := zap.NewDevelopmentConfig()
-	config.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder
-	logger, err := config.Build()
-	panicIf(err)
-
-	// set up the cache with this fake service
-	fmap := makeFunctionServiceMap(logger, 0)
-	fmap.assign(&fnMeta, testServiceUrl)
-
-	// HTTP trigger set with a trigger for this function
-	triggers, _, _ := makeHTTPTriggerSet(logger, fmap, nil, nil, nil, nil,
-		&tsRoundTripperParams{
-			timeout:         50 * time.Millisecond,
-			timeoutExponent: 2,
-			maxRetries:      10,
-		}, false, throttler.MakeThrottler(30*time.Second))
-	triggerUrl := "/foo"
-	triggers.triggers = append(triggers.triggers,
-		fv1.HTTPTrigger{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:            "xxx",
-				Namespace:       metav1.NamespaceDefault,
-				ResourceVersion: "1234",
-			},
-			Spec: fv1.HTTPTriggerSpec{
-				RelativeURL:       triggerUrl,
-				FunctionReference: fr,
-				Method:            "GET",
-			},
-		})
-
-	// set up the resolver's cache for this function
-	frr := makeFunctionReferenceResolver(nil)
-	nfr := namespacedTriggerReference{
-		namespace:              metav1.NamespaceDefault,
-		triggerName:            "xxx",
-		triggerResourceVersion: "1234",
-	}
-
-	fnMetaMap := make(map[string]*fv1.Function, 1)
-	fnMetaMap[fnMeta.Name] = &fv1.Function{
-		ObjectMeta: fnMeta,
-	}
-
-	rr := resolveResult{
-		resolveResultType: resolveResultSingleFunction,
-		functionMap:       fnMetaMap,
-	}
-	frr.refCache.Set(nfr, rr)
-
-	// run the router
-	port := 4242
-	tracingSamplingRate := .5
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-	go serve(ctx, logger, port, tracingSamplingRate, triggers, frr, false)
-	time.Sleep(100 * time.Millisecond)
-
-	// hit the router
-	testUrl := fmt.Sprintf("http://localhost:%v%v", port, triggerUrl)
-	testRequest(testUrl, testResponseString)
-}