From 2c621f31bce082b78730fd67149671d72ed04594 Mon Sep 17 00:00:00 2001 From: "liheng.zms" Date: Thu, 2 Jan 2025 21:01:17 +0800 Subject: [PATCH] Fix: ImagePullJob timeout setting is not effective when it is greater than 1800 Signed-off-by: liheng.zms --- .../imagepulljob/imagepulljob_utils.go | 10 +++- .../imagepulljob/imagepulljob_utils_test.go | 57 +++++++++++++++++++ .../imagepulljob_create_update_handler.go | 22 ++++--- 3 files changed, 79 insertions(+), 10 deletions(-) diff --git a/pkg/controller/imagepulljob/imagepulljob_utils.go b/pkg/controller/imagepulljob/imagepulljob_utils.go index 63afc2c26e..4a7f01b5f3 100644 --- a/pkg/controller/imagepulljob/imagepulljob_utils.go +++ b/pkg/controller/imagepulljob/imagepulljob_utils.go @@ -94,7 +94,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,7 +108,13 @@ 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 { + + var ret = int64(*job.Spec.PullPolicy.TimeoutSeconds) + return &ret + } var ret = defaultActiveDeadlineSecondsForNever return &ret } diff --git a/pkg/controller/imagepulljob/imagepulljob_utils_test.go b/pkg/controller/imagepulljob/imagepulljob_utils_test.go index f2dd0f223a..c88471672f 100644 --- 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 index 28d0e41e5f..c68c7e3ad4 100644 --- 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")