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

add argo integration based on pod integration #3897

Merged
merged 4 commits into from
Jan 29, 2025
Merged

add argo integration based on pod integration #3897

merged 4 commits into from
Jan 29, 2025

Conversation

kannon92
Copy link
Contributor

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Add an integration with Kueue and Argo Workflows

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Credit goes to sam-leitch-oxb.
Nice work!

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 20, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 20, 2024
@netlify Netlify
Copy link

netlify bot commented Dec 20, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name Link
🔨 Latest commit 30558fc
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/679931db4dc18000087cbc54
😎 Deploy Preview https://deploy-preview-3897--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.

@kannon92 kannon92 mentioned this pull request Dec 20, 2024
Closed
@trasc
Copy link
Contributor

trasc commented Jan 14, 2025

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from trasc January 14, 2025 11:55
mimowo
mimowo reviewed Jan 15, 2025
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.

Thanks for the docs!

site/content/en/docs/tasks/run/argo_workload.md Outdated Show resolved Hide resolved
site/content/en/docs/tasks/run/argo_workload.md Outdated Show resolved Hide resolved
site/content/en/docs/tasks/run/argo_workload.md Outdated Show resolved Hide resolved
site/content/en/docs/tasks/run/argo_workload.md Outdated Show resolved Hide resolved
site/content/en/docs/tasks/run/argo_workload.md Outdated Show resolved Hide resolved
site/content/en/docs/tasks/_index.md Outdated Show resolved Hide resolved
@kannon92 kannon92 requested a review from mimowo January 17, 2025 18:30
@kannon92
Copy link
Contributor Author

@mimowo PTAL

@mimowo
Copy link
Contributor

mimowo commented Jan 23, 2025

/approve
/hold
LGTM overall, but would like to get a pass from @PBundyra who is recently looking at the ways to improve our documentation.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 23, 2025
@PBundyra
Copy link
Contributor

PBundyra commented Jan 23, 2025 •
edited
Loading

Other than the one comment I left, LGTM! Feel free to unhold your PR after addressing it.
Thanks!
/lgtm

@PBundyra
Copy link
Contributor

/lgtm

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

LGTM label has been added.

Git tree hash: c034e48c52ccc8eb568493ca2b67a9b23abdb2fe

mimowo
mimowo reviewed Jan 23, 2025
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 overall, please additionally extend the list of options in https://kueue.sigs.k8s.io/docs/tasks/dev/integrate_a_custom_job/ by adding a point like "Leverage the Plain Pods integration", linking to the new tasks page.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2025
@k8s-ci-robot k8s-ci-robot requested a review from PBundyra January 23, 2025 15:20
@kannon92
Copy link
Contributor Author

/hold

Addressing #3897 (comment).

Need to confirm that this worked.

@kannon92
Copy link
Contributor Author

@PBundyra @mimowo PTAL when you get a chance.

I moved around the files as you suggested.

mimowo
mimowo reviewed Jan 28, 2025
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.

Actually, I would like to consider the following sturcture:

Run Workloads:
- Kubernetes Jobs
- Kubernetes CronJobs
- AppWrappers
- RayClusters
- RayJobs
- Deployment
- StatefulSet
- Plain Pods
- Kubeflow Jobs
- Python
- Jobsets
- External Frameworks
  * Using AppWrapper (renamed from Custom Workload)
  * Flux MiniClusters
  * Argo Workflows 
- Multi-Cluster

I don't think we need more structure under "External frameworks" at this point, because there are only three items underneath, and extra layers mean more clicking to expand.

site/content/en/docs/tasks/_index.md Outdated Show resolved Hide resolved
site/content/en/docs/tasks/_index.md Outdated Show resolved Hide resolved
site/content/en/docs/tasks/_index.md Outdated
- [Submit Kueue jobs from Python](run/python_jobs).
- [Run a Kueue managed plain Pod](run/plain_pods).
- [Run a Kueue managed JobSet](run/jobsets).
- [Submit jobs to MultiKueue](run/multikueue).
- [Support external workloads](run/external_workloads).
Kueue allows one to use existing integrations to support external workloads.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Kueue allows one to use existing integrations to support external workloads.
Kueue allows one to use built-in integrations (such as Pods or Jobs) to run external workloads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

site/content/en/docs/tasks/run/external_workloads/_index.md Outdated Show resolved Hide resolved
site/content/en/docs/tasks/run/external_workloads/appwrappers.md Outdated Show resolved Hide resolved
mimowo
mimowo reviewed Jan 29, 2025
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
Feel free to address the nit comment in a follow up.

site/content/en/docs/tasks/run/external_workloads/argo_workflow.md
@@ -0,0 +1,51 @@
---
title: "Run An Argo Workflow"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add linkTitle: Argo Workflow to make the display consistent.

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

LGTM label has been added.

Git tree hash: be39cfda7dec117ccaf20956e1f0ece28e98896f

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kannon92, 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

@kannon92
Copy link
Contributor Author

/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 Jan 29, 2025
@k8s-ci-robot k8s-ci-robot merged commit 4a6c3fa into kubernetes-sigs:main Jan 29, 2025
7 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.11 milestone Jan 29, 2025
@kannon92
Copy link
Contributor Author

Should this be cherry-picked to website also?

@mimowo
Copy link
Contributor

mimowo commented Jan 30, 2025

We could cherry pick, but i assume it would have conflicts due to introducing AppWrapper in the meanwhile.

I guess it depends on you. if you want to prepare the cherry pick and resolve the conflicts, then i will welcome the PR, but waiting another 1.5month for the docs update along with 0.11 is also fine for me.

@dgrove-oss
Copy link
Contributor

I would suggest waiting until 0.11 so we get a unified update of the website.

@kannon92
Copy link
Contributor Author

SGTM.

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

@dgrove-oss dgrove-oss dgrove-oss left review comments

@PBundyra PBundyra Awaiting requested review from PBundyra

Assignees

@mimowo mimowo

@PBundyra PBundyra

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/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.11
Development

Successfully merging this pull request may close these issues.
6 participants
@kannon92 @trasc @mimowo @PBundyra @k8s-ci-robot @dgrove-oss