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.

Sign up for GitHub

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

Jump to bottom

Wait for the prebuild workload. #3255

Merged
merged 3 commits into from
Oct 21, 2024
Merged

Wait for the prebuild workload. #3255

merged 3 commits into from
Oct 21, 2024

Conversation

mbobrovskyi
Copy link
Contributor

@mbobrovskyi mbobrovskyi commented Oct 17, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Wait for the prebuilt Workload if a Job is running before it.

Which issue(s) this PR fixes:

Fixes #3051

Special notes for your reviewer:

Follow up for #3131 (comment).

Does this PR introduce a user-facing change?

Best effort support for scenarios when the Job is created at the same time as prebuilt workload or momentarily before the workload. In that case an error is logged to indicate that creating a Job before prebuilt-workload is outside of the intended use.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Oct 17, 2024
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 17, 2024
@netlify Netlify
Copy link

netlify bot commented Oct 17, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name Link
🔨 Latest commit f05c9fc
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/671620fadc4c840008315fd2
😎 Deploy Preview https://deploy-preview-3255--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 17, 2024
@mbobrovskyi mbobrovskyi mentioned this pull request Oct 17, 2024
Closed
@mbobrovskyi mbobrovskyi force-pushed the fix/wait-for-prebuild-workload branch from a59b9af to f9e4321 Compare October 17, 2024 13:24
@mimowo
Copy link
Contributor

mimowo commented Oct 17, 2024

Please also add "Fixes #3051" to the description.

@mbobrovskyi
Copy link
Contributor Author

Please also add "Fixes #3051" to the description.

IIUC correct #3131 (comment) it's not fixing #3051.

@mimowo
Copy link
Contributor

mimowo commented Oct 17, 2024

Hm, I thought it does, because the integration test passes. I would prefer to better understand the issue in the #3131 (comment) comment then before merging any of the solutions.

@mbobrovskyi
Copy link
Contributor Author

/hold for #3131 (comment)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 17, 2024
@mbobrovskyi
Copy link
Contributor Author

/unhold due to #3131 (comment).

@mimowo
Copy link
Contributor

mimowo commented Oct 21, 2024

I would suggest, for simplicity, move forward with this PR, and:

  1. Return an error (to make the delays exponential)
  2. In the release note say: 'Best effort support for scenarios when the Job is created at the same time as prebuilt workload or momentarily before the workload. In that case an error is logged to indicate that creating a Job before prebuilt-workload is outside of the intended use.
  3. Update the docs for the prebuild workload annotation to say that the intended use of prebuilt workload is to create the Job once the workload is created. In other scenarios the behavior is undefined

@mbobrovskyi mbobrovskyi force-pushed the fix/wait-for-prebuild-workload branch 2 times, most recently from 9ade4b2 to d73282a Compare October 21, 2024 08:53
@mbobrovskyi
Copy link
Contributor Author

I would suggest, for simplicity, move forward with this PR, and:

  1. Return an error (to make the delays exponential)
  2. In the release note say: 'Best effort support for scenarios when the Job is created at the same time as prebuilt workload or momentarily before the workload. In that case an error is logged to indicate that creating a Job before prebuilt-workload is outside of the intended use.
  3. Update the docs for the prebuild workload annotation to say that the intended use of prebuilt workload is to create the Job once the workload is created. In other scenarios the behavior is undefined

Done

@mbobrovskyi @IrvingMg
Wait for the prebuild workload.
881a31f 
Co-authored-by: Irving Mondragón <irvingmg@users.noreply.github.com>
@mimowo
Copy link
Contributor

mimowo commented Oct 21, 2024

please check why the unit tests fail and adjust if ok

@mbobrovskyi mbobrovskyi force-pushed the fix/wait-for-prebuild-workload branch from d73282a to f05c9fc Compare October 21, 2024 09:38
@mbobrovskyi
Copy link
Contributor Author

please check why the unit tests fail and adjust if ok

Done

mimowo
mimowo reviewed Oct 21, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
Thanks, please follow up with the docs update suggestion for the label

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 76100cd2904ec7df2d994714563305f622b745a1

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbobrovskyi, mimowo

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2024
@mimowo
Copy link
Contributor

mimowo commented Oct 21, 2024

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2024
@mimowo
Copy link
Contributor

mimowo commented Oct 21, 2024

Thanks, please follow up with the docs update suggestion for the label

Oh, I see it is already added here. No need for follow up here.

@k8s-ci-robot k8s-ci-robot merged commit 28a8058 into kubernetes-sigs:main Oct 21, 2024
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.9 milestone Oct 21, 2024
@mbobrovskyi mbobrovskyi deleted the fix/wait-for-prebuild-workload branch October 21, 2024 11:34
PBundyra pushed a commit to PBundyra/kueue that referenced this pull request Nov 5, 2024
@mbobrovskyi @IrvingMg @PBundyra
Wait for the prebuild workload. (kubernetes-sigs#3255)
b5fe343 
* Wait for the prebuild workload.

Co-authored-by: Irving Mondragón <irvingmg@users.noreply.github.com>

* Return an error on "prebuild workload not found" error.

* Update docs.

---------

Co-authored-by: Irving Mondragón <irvingmg@users.noreply.github.com>
kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024
@mbobrovskyi @IrvingMg @kannon92
Wait for the prebuild workload. (kubernetes-sigs#3255)
cf96e31 
* Wait for the prebuild workload.

Co-authored-by: Irving Mondragón <irvingmg@users.noreply.github.com>

* Return an error on "prebuild workload not found" error.

* Update docs.

---------

Co-authored-by: Irving Mondragón <irvingmg@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mimowo mimowo mimowo left review comments

@denkensk denkensk Awaiting requested review from denkensk

Assignees

@mimowo mimowo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v0.9
Development

Successfully merging this pull request may close these issues.

[Flaking test] Kueue when Creating a Job With Queueing Should run with prebuilt workload
3 participants
@mbobrovskyi @mimowo @k8s-ci-robot