-
Notifications
You must be signed in to change notification settings - Fork 962
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 job controller reports duplicate warnings #3746
Conversation
var latestConditionMsg string | ||
|
||
// Get the latest condition by timestamp | ||
for _, condition := range podGroup.Status.Conditions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little doubt here, the unschedulable condition will append to unschedulable condition?What if just override the unschedulable condition because pg is already schedulable and no need to concern the unschedulable condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just to solve the problem that the existing podgroup will repeatedly log warning events when the controller restarts. The condition update issue needs to be fixed separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add ut here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UT testcase has been added
Seems that the main cause is that there are duplicated condition type status:
conditions:
- lastTransitionTime: "2024-09-25T02:15:18Z"
message: '1/0 tasks in gang unschedulable: pod group is not ready, 1 minAvailable'
reason: NotEnoughResources
status: "True"
transitionID: b2adeea6-9812-4315-a3a4-6411c4b364c9
type: Unschedulable
- lastTransitionTime: "2024-09-25T02:15:20Z"
reason: tasks in gang are ready to be scheduled
status: "True"
transitionID: f9b8c487-41ad-49a6-9474-3e732b5ae29e
type: Scheduled
phase: Running
running: 1 So maybe we can add a new condition type like |
cc @lowang-bh |
e751093
to
3bd1708
Compare
I've reworked the function logic a bit, and now it can support the subsequent extension of the new unscheduled condition |
We should record another issue to track the following problem:
|
Yes I think we are ok with pr to not record warning events if the last condition is not scheduled, but we should keep resolving these problems. |
I submitted an issue for podgroup unschedulable condition. #3749 |
3bd1708
to
5ea28b6
Compare
Signed-off-by: liuyuanchun <superpig13@hotmail.com>
5ea28b6
to
f731d36
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Monokaix The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
When the syncJob is processed in the job_controller, the condition timestamp of the podGroup is determined, and if the latest status is Unschedulable, a warning event is recorded. If the latest status is Scheduled, no warning event will be logged even if there is Unschedulable in the condtition.
fix issue #3745