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] BroadcastJob activeDeadlineSeconds did not take effect #1410

Closed
wants to merge 1 commit into from

Conversation

ls-2018
Copy link
Member

@ls-2018 ls-2018 commented Sep 9, 2023

A description of the specific problem is in issue

@kruise-bot kruise-bot added the size/XL size/XL: 500-999 label Sep 9, 2023
@ls-2018 ls-2018 changed the title [FIX] issue 1409 [FIX] BroadcastJob activeDeadlineSeconds did not take effect Sep 9, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2023

Codecov Report

Attention: Patch coverage is 41.66667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 47.88%. Comparing base (dad39bc) to head (3183a64).

Files Patch % Lines
...controller/broadcastjob/broadcastjob_controller.go 41.66% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1410      +/-   ##
==========================================
- Coverage   47.90%   47.88%   -0.02%     
==========================================
  Files         161      161              
  Lines       23425    23435      +10     
==========================================
+ Hits        11222    11223       +1     
- Misses      10989    10995       +6     
- Partials     1214     1217       +3     
Flag Coverage Δ
unittests 47.88% <41.66%> (-0.02%) ⬇️

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.

"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

type empty struct{}
Copy link
Member

@zmberg zmberg Sep 12, 2023

Choose a reason for hiding this comment

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

Why must this official function be redefine, and official functions are best left unmodified.

@zmberg
Copy link
Member

zmberg commented Sep 12, 2023

@ls-2018 My local validation is working and this code place jobFailed is true and then it will delete all Pods.

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from zmberg by writing /assign @zmberg in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kruise-bot kruise-bot added size/M size/M: 30-99 and removed size/XL size/XL: 500-999 labels Sep 12, 2023
@ls-2018
Copy link
Member Author

ls-2018 commented Sep 12, 2023

I simplified the code a little bit.

Reconcile all of the Reconcile function can be handled because AdvancedCronJob, BroadcastJob and Job events occur. However, in the case I submitted, I watched three kinds of resources respectively and found that no incident happened. Of course, this probability may be low, but it is not impossible

@zmberg
Copy link
Member

zmberg commented Sep 12, 2023

What I'm more interested in understanding is how this patch related to the issue? Bug?

@ls-2018
Copy link
Member Author

ls-2018 commented Sep 12, 2023

I think it was a bug.
Pulling the large image exceeded the expected death time, but the pod could not be killed because no event was watched.No event changes at this time

@zmberg
Copy link
Member

zmberg commented Sep 13, 2023

@kruise-bot kruise-bot added size/S size/S 10-29 and removed size/M size/M: 30-99 labels Sep 13, 2023
@ls-2018
Copy link
Member Author

ls-2018 commented Sep 13, 2023

I think I have found the root cause. It is not that the ActiveDeadlineSeconds have not been reconciled after expiration, but that there is no judgment on whether the pod should be removed or not.
@zmberg

@ls-2018
Copy link
Member Author

ls-2018 commented Oct 9, 2023

@zmberg Can you look at this request again please? Thanks.

Copy link

stale bot commented Jan 8, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jan 8, 2024
@stale stale bot closed this Jan 16, 2024
@ls-2018 ls-2018 reopened this Jan 16, 2024
@stale stale bot removed wontfix This will not be worked on labels Jan 16, 2024
@ls-2018 ls-2018 marked this pull request as draft January 16, 2024 02:13
@ls-2018 ls-2018 marked this pull request as ready for review January 16, 2024 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S size/S 10-29
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants