Fix: ImagePullJob timeout setting is not effective when it is greater…

… than 1800 Signed-off-by: liheng.zms <[email protected]>
openkruise · Jan 7, 2025 · 58c1ecb · 58c1ecb
1 parent 79b64c1
commit 58c1ecb
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 11 deletions.
diff --git a/pkg/controller/imagepulljob/imagepulljob_utils.go b/pkg/controller/imagepulljob/imagepulljob_utils.go
@@ -29,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
+	utilpointer "k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -94,7 +95,7 @@ func getImagePullPolicy(job *appsv1alpha1.ImagePullJob) *appsv1alpha1.ImageTagPu
 	}
 	if job.Spec.CompletionPolicy.Type == appsv1alpha1.Never {
 		pullPolicy.TTLSecondsAfterFinished = getTTLSecondsForNever()
-		pullPolicy.ActiveDeadlineSeconds = getActiveDeadlineSecondsForNever()
+		pullPolicy.ActiveDeadlineSeconds = getActiveDeadlineSecondsForNever(job)
 	} else {
 		pullPolicy.TTLSecondsAfterFinished = getTTLSecondsForAlways(job)
 		pullPolicy.ActiveDeadlineSeconds = job.Spec.CompletionPolicy.ActiveDeadlineSeconds
@@ -108,9 +109,15 @@ func getTTLSecondsForNever() *int32 {
 	return &ret
 }
 
-func getActiveDeadlineSecondsForNever() *int64 {
+func getActiveDeadlineSecondsForNever(job *appsv1alpha1.ImagePullJob) *int64 {
+	if job.Spec.PullPolicy != nil && job.Spec.PullPolicy.TimeoutSeconds != nil &&
+		int64(*job.Spec.PullPolicy.TimeoutSeconds) > defaultActiveDeadlineSecondsForNever {
+
+		ret := int64(*job.Spec.PullPolicy.TimeoutSeconds)
+		return utilpointer.Int64(ret)
+	}
 	var ret = defaultActiveDeadlineSecondsForNever
-	return &ret
+	return utilpointer.Int64(ret)
 }
 
 func containsObject(slice []appsv1alpha1.ReferenceObject, obj appsv1alpha1.ReferenceObject) bool {

diff --git a/pkg/controller/imagepulljob/imagepulljob_utils_test.go b/pkg/controller/imagepulljob/imagepulljob_utils_test.go
@@ -20,10 +20,12 @@ import (
 	"reflect"
 	"testing"
 
+	appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
 	"github.com/openkruise/kruise/pkg/util"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	utilpointer "k8s.io/utils/pointer"
 )
 
 func TestTargetFromSource(t *testing.T) {
@@ -123,3 +125,58 @@ func TestTargetFromSource(t *testing.T) {
 		})
 	}
 }
+
+func TestGetActiveDeadlineSecondsForNever(t *testing.T) {
+	cases := []struct {
+		name        string
+		getImageJob func() *appsv1alpha1.ImagePullJob
+		expected    int64
+	}{
+		{
+			name: "not set timeout",
+			getImageJob: func() *appsv1alpha1.ImagePullJob {
+				return &appsv1alpha1.ImagePullJob{}
+			},
+			expected: 1800,
+		},
+		{
+			name: "timeout < 1800",
+			getImageJob: func() *appsv1alpha1.ImagePullJob {
+				return &appsv1alpha1.ImagePullJob{
+					Spec: appsv1alpha1.ImagePullJobSpec{
+						ImagePullJobTemplate: appsv1alpha1.ImagePullJobTemplate{
+							PullPolicy: &appsv1alpha1.PullPolicy{
+								TimeoutSeconds: utilpointer.Int32Ptr(1799),
+							},
+						},
+					},
+				}
+			},
+			expected: 1800,
+		},
+		{
+			name: "timeout > 1800",
+			getImageJob: func() *appsv1alpha1.ImagePullJob {
+				return &appsv1alpha1.ImagePullJob{
+					Spec: appsv1alpha1.ImagePullJobSpec{
+						ImagePullJobTemplate: appsv1alpha1.ImagePullJobTemplate{
+							PullPolicy: &appsv1alpha1.PullPolicy{
+								TimeoutSeconds: utilpointer.Int32Ptr(7200),
+							},
+						},
+					},
+				}
+			},
+			expected: 7200,
+		},
+	}
+
+	for _, cs := range cases {
+		t.Run(cs.name, func(t *testing.T) {
+			ret := getActiveDeadlineSecondsForNever(cs.getImageJob())
+			if *ret != cs.expected {
+				t.Fatalf("expect(%d), but get(%d)", cs.expected, *ret)
+			}
+		})
+	}
+}
diff --git a/pkg/webhook/imagepulljob/validating/imagepulljob_create_update_handler.go b/pkg/webhook/imagepulljob/validating/imagepulljob_create_update_handler.go
@@ -21,16 +21,15 @@ import (
 	"fmt"
 	"net/http"
 
-	"k8s.io/apimachinery/pkg/util/sets"
-
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/klog/v2"
-	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
-
 	appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
 	daemonutil "github.com/openkruise/kruise/pkg/daemon/util"
 	"github.com/openkruise/kruise/pkg/features"
 	utilfeature "github.com/openkruise/kruise/pkg/util/feature"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/klog/v2"
+	utilpointer "k8s.io/utils/pointer"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
 // ImagePullJobCreateUpdateHandler handles ImagePullJob
@@ -97,10 +96,17 @@ func validate(obj *appsv1alpha1.ImagePullJob) error {
 	if _, err := daemonutil.NormalizeImageRef(obj.Spec.Image); err != nil {
 		return fmt.Errorf("invalid image %s: %v", obj.Spec.Image, err)
 	}
-
+	if obj.Spec.PullPolicy == nil {
+		obj.Spec.PullPolicy = &appsv1alpha1.PullPolicy{}
+	}
+	if obj.Spec.PullPolicy.TimeoutSeconds == nil {
+		obj.Spec.PullPolicy.TimeoutSeconds = utilpointer.Int32Ptr(600)
+	}
 	switch obj.Spec.CompletionPolicy.Type {
 	case appsv1alpha1.Always:
-
+		if obj.Spec.CompletionPolicy.ActiveDeadlineSeconds != nil && int64(*obj.Spec.PullPolicy.TimeoutSeconds) > *obj.Spec.CompletionPolicy.ActiveDeadlineSeconds {
+			return fmt.Errorf("completionPolicy.activeDeadlineSeconds must be greater than pullPolicy.timeoutSeconds(default 600)")
+		}
 	case appsv1alpha1.Never:
 		if obj.Spec.CompletionPolicy.ActiveDeadlineSeconds != nil || obj.Spec.CompletionPolicy.TTLSecondsAfterFinished != nil {
 			return fmt.Errorf("activeDeadlineSeconds and ttlSecondsAfterFinished can only work with Always CompletionPolicyType")