Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: ImagePullJob timeout setting is not effective when it is greater than 1800 #1874

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

zmberg
Copy link
Member

@zmberg zmberg commented Jan 2, 2025

… than 1800

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

Copy link

codecov bot commented Jan 2, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 50.93%. Comparing base (0d0031a) to head (92c8627).
Report is 141 commits behind head on master.

Files with missing lines Patch % Lines
pkg/controller/imagepulljob/imagepulljob_utils.go 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1874      +/-   ##
==========================================
+ Coverage   47.91%   50.93%   +3.01%     
==========================================
  Files         162      192      +30     
  Lines       23491    24953    +1462     
==========================================
+ Hits        11256    12710    +1454     
+ Misses      11014    10939      -75     
- Partials     1221     1304      +83     
Flag Coverage Δ
unittests 50.93% <88.88%> (+3.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zmberg zmberg force-pushed the imagepulljob-timeout branch from 89e42b9 to 2231201 Compare January 3, 2025 01:40
@zmberg zmberg force-pushed the imagepulljob-timeout branch from 2231201 to 2c621f3 Compare January 3, 2025 02:12
if job.Spec.PullPolicy != nil && job.Spec.PullPolicy.TimeoutSeconds != nil &&
int64(*job.Spec.PullPolicy.TimeoutSeconds) > defaultActiveDeadlineSecondsForNever {

var ret = int64(*job.Spec.PullPolicy.TimeoutSeconds)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use the package 'k8s.io/utils/pointer'

if job.Spec.PullPolicy != nil && job.Spec.PullPolicy.TimeoutSeconds != nil &&
int64(*job.Spec.PullPolicy.TimeoutSeconds) > defaultActiveDeadlineSecondsForNever {

var ret = int64(*job.Spec.PullPolicy.TimeoutSeconds)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why int64, it seems that TimeoutSeconds is of *int32 type

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because activeDeadlineSeconds is *int64.

… than 1800

Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com>
@zmberg zmberg force-pushed the imagepulljob-timeout branch from 2c621f3 to 92c8627 Compare January 7, 2025 09:47
Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@furykerry
Copy link
Member

/approve

@furykerry furykerry merged commit 58c1ecb into openkruise:master Jan 7, 2025
44 of 45 checks passed
@zmberg zmberg deleted the imagepulljob-timeout branch January 7, 2025 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants