-
Notifications
You must be signed in to change notification settings - Fork 228
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
Add condition WaitingForReplacementPods for pod groups integration #2234
Add condition WaitingForReplacementPods for pod groups integration #2234
Conversation
Skipping CI for Draft Pull Request. |
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
b3e4aec
to
a2dc2ad
Compare
/ok-to-test |
/assign @trasc |
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.
Overall LGTM
a2dc2ad
to
840ab23
Compare
/test all |
17a5f5c
to
a517928
Compare
a517928
to
a91aa41
Compare
// when custom workload conditions should be updated after ensure that the workload exists. | ||
type JobWithCustomWorkloadConditions interface { | ||
// CustomWorkloadConditions return custom workload conditions and status changed or not. | ||
CustomWorkloadConditions(wl *kueue.Workload) ([]metav1.Condition, bool) |
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.
Can we instead put it somewhere inside the implementation of FindMatchingWorkloads
for the pod integration? We are already removing excess pods and doing patches to the Workload object there.
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.
I think that FindMatchingWorkloads
already doing so much. The function name is Find...
that is shouldn't update workload. So, if we can move this logic to another place, that's better in my opinion.
} | ||
|
||
if replCond.Status != metav1.ConditionTrue { | ||
replCond.Status = metav1.ConditionTrue |
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.
in which case do we reach this?
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.
After replacement finished we are setting status to false. And when replacement happening again we need to set to true and update. In this case we need to know that status updated. But also we don't need to update workload if it's no changes.
a91aa41
to
6749db5
Compare
224492a
to
ddc0f4a
Compare
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.
I forgot to send my review yesterday :(
ddc0f4a
to
1c2075e
Compare
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.
/lgtm
/approve
LGTM label has been added. Git tree hash: d22055cf144807ed663f8aa495662b4f33171f8c
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, mbobrovskyi 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 |
/release-note-edit
|
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add condition WaitingForReplacementPods for pod groups integration.
Which issue(s) this PR fixes:
Fixes #2207
Special notes for your reviewer:
Does this PR introduce a user-facing change?