From f0a81e642e390a7a6da3725b7ab7e0baa70b376a Mon Sep 17 00:00:00 2001
From: "Alex Ellis (OpenFaaS Ltd)" <alexellis2@gmail.com>
Date: Thu, 4 Nov 2021 14:56:04 +0000
Subject: [PATCH] Add 2s jitter to termination grace period

The additional 2s should prevent an issue where the grace
period is exactly the same as the timeout, and kills the Pod
before the remaining requests have completed.

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
---
 pkg/controller/deployment.go      |  8 ++++++--
 pkg/controller/deployment_test.go | 12 ++++++------
 pkg/handlers/deploy.go            |  7 +++++--
 pkg/handlers/deploy_test.go       | 12 ++++++------
 4 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/pkg/controller/deployment.go b/pkg/controller/deployment.go
index 2ae9b99b2..4d829522f 100644
--- a/pkg/controller/deployment.go
+++ b/pkg/controller/deployment.go
@@ -57,7 +57,9 @@ func newDeployment(
 		}
 	}
 
-	terminationGracePeriod := time.Second * 30
+	// add 2s jitter to avoid a race condition between write_timeout and grace period
+	jitter := time.Second * 2
+	terminationGracePeriod := time.Second*30 + jitter
 
 	if function.Spec.Environment != nil {
 		e := *function.Spec.Environment
@@ -67,9 +69,11 @@ func newDeployment(
 				glog.Warningf("Function %s failed to parse write_timeout: %s",
 					function.Spec.Name, err.Error())
 			}
-			terminationGracePeriod = period
+
+			terminationGracePeriod = period + jitter
 		}
 	}
+
 	terminationGracePeriodSeconds := int64(terminationGracePeriod.Seconds())
 
 	allowPrivilegeEscalation := false
diff --git a/pkg/controller/deployment_test.go b/pkg/controller/deployment_test.go
index 35ba97fc2..12ca6f554 100644
--- a/pkg/controller/deployment_test.go
+++ b/pkg/controller/deployment_test.go
@@ -13,18 +13,18 @@ import (
 func Test_GracePeriodFromWriteTimeout(t *testing.T) {
 
 	scenarios := []struct {
-		name    string
-		seconds int64
-		envs    map[string]string
+		name        string
+		wantSeconds int64
+		envs        map[string]string
 	}{
-		{"grace period is the default", 30, map[string]string{}},
-		{"grace period is set from write_timeout", 60, map[string]string{"write_timeout": "60s"}},
+		{"grace period is the default", 32, map[string]string{}},
+		{"grace period is set from write_timeout", 62, map[string]string{"write_timeout": "60s"}},
 	}
 
 	for _, s := range scenarios {
 		t.Run(s.name, func(t *testing.T) {
 
-			want := int64(s.seconds)
+			want := int64(s.wantSeconds)
 			function := &faasv1.Function{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "alpine",
diff --git a/pkg/handlers/deploy.go b/pkg/handlers/deploy.go
index 4a2a906a6..62494844a 100644
--- a/pkg/handlers/deploy.go
+++ b/pkg/handlers/deploy.go
@@ -174,7 +174,9 @@ func makeDeploymentSpec(request types.FunctionDeployment, existingSecrets map[st
 		return nil, err
 	}
 
-	terminationGracePeriod := time.Second * 30
+	// add 2s jitter to avoid a race condition between write_timeout and grace period
+	jitter := time.Second * 2
+	terminationGracePeriod := time.Second*30 + jitter
 
 	if request.EnvVars != nil {
 		if v, ok := request.EnvVars["write_timeout"]; ok && len(v) > 0 {
@@ -183,7 +185,8 @@ func makeDeploymentSpec(request types.FunctionDeployment, existingSecrets map[st
 				glog.Warningf("Function %s failed to parse write_timeout: %s",
 					request.Service, err.Error())
 			}
-			terminationGracePeriod = period
+
+			terminationGracePeriod = period + jitter
 		}
 	}
 
diff --git a/pkg/handlers/deploy_test.go b/pkg/handlers/deploy_test.go
index a8c56183f..e8609b075 100644
--- a/pkg/handlers/deploy_test.go
+++ b/pkg/handlers/deploy_test.go
@@ -16,12 +16,12 @@ import (
 func Test_GracePeriodFromWriteTimeout(t *testing.T) {
 
 	scenarios := []struct {
-		name    string
-		seconds int64
-		envs    map[string]string
+		name        string
+		wantSeconds int64
+		envs        map[string]string
 	}{
-		{"grace period is the default", 30, map[string]string{}},
-		{"grace period is set from write_timeout", 60, map[string]string{"write_timeout": "60s"}},
+		{"grace period is the default", 32, map[string]string{}},
+		{"grace period is set from write_timeout", 62, map[string]string{"write_timeout": "60s"}},
 	}
 
 	for _, s := range scenarios {
@@ -38,7 +38,7 @@ func Test_GracePeriodFromWriteTimeout(t *testing.T) {
 			if err != nil {
 				t.Errorf("unexpected makeDeploymentSpec error: %s", err.Error())
 			}
-			want := s.seconds
+			want := s.wantSeconds
 			got := deployment.Spec.Template.Spec.TerminationGracePeriodSeconds
 
 			if got == nil {