AppWrapper integration #3953

dgrove-oss · 2025-01-09T22:51:07Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds an AppWrapper integration to Kueue.

During the initial development of a Kueue-compatible AppWrapper, providing Kueue support via the external framework mechanism provided maximal flexibility. AppWrappers have reached a stable 1.0 release and we are interested in pursuing multi-Kueue support for AppWrappers (which is not supported for external frameworks). Additionally, the fault tolerance capabilities of AppWrapper and its ability to (a) group multiple resources into a single workload and (b) swiftly Kueue-enable other Kinds that are built on PodSpecTemplate may be of interest to the Kueue community. Therefore, we would like to contribute and maintain an AppWrapper integration for Kueue as a full-fledged built-in integration.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

I think TAS support is straightforward; will look at adding that (and related integration tests) next.

I think it makes sense to add AppWrapper to the e2e tests, especially as it gives us a way to test multi-level parent/child relationships. I didn't do that in this PR to keep it from getting too large. I would plan to do that in a subsequent PR.

Multi-Kueue support would also be a follow up PR. I think it is doable before 0.11, but we don't have much direct experience with multi-kueue since it hasn't been possible to do as an external framework.

Does this PR introduce a user-facing change?

Add an integration for AppWrappers to Kueue.

netlify · 2025-01-09T22:51:25Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`a9a1d51`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6786c0ab88ef1c00085dd562

kannon92 · 2025-01-10T16:26:52Z

I think it makes sense to add AppWrapper to the e2e tests, especially as it gives us a way to test multi-level parent/child relationships. I didn't do that in this PR to keep it from getting too large. I would plan to do that in a subsequent PR.

I lean towards adding at least a simple e2e test as part of this PR to make sure it works. But I'll leave that to the maintainers if it is required.

mimowo · 2025-01-13T08:47:57Z

Thank you for the PR! I'm ok with including the integration as a built-in for Kueue as it nicely fits into our needs to support mixing training and inference.

Another good thing is that it provides us a way of deploying Deployments and StatefulSets via MultiKueue before the Pod Integration is supported for MultiKueue (#3802). Otherwise the support would be unlikely for 0.11.

My only concern is the potentially growing pool of integrations moved into Kueue due to the lack of support for external Jobs in MultiKueue (#2349). To prevent that I would suggest to prioritize it @mwielgus @mwysokin. In any case, it is unlikely to happen for 0.11.

I lean towards adding at least a simple e2e test as part of this PR to make sure it works. But I'll leave that to the maintainers if it is required.

I would also like to have an e2e test - could be this PR or a follow up if this one was manually tested.

BTW, it TAS supported? I believe not, because I see that SetPodSetInfos does not copy the SchedulingGates which needed. Do you plan to fill in the gap?

mimowo · 2025-01-13T12:55:09Z

pkg/controller/jobs/appwrapper/appwrapper_controller.go

+func (aw *AppWrapper) PodSets() []kueue.PodSet {
+	podSets, err := awutils.GetPodSets((*awv1beta2.AppWrapper)(aw))
+	if err != nil {
+		// Kueue will raise an error on zero length PodSet; the Kueue GenericJob API prevents propagating the actual error.


Can you indicate where the actual error will be raised?

I would prefer to avoid silencing errors, can we update the PodSets interface, could be a follow up?

I can definitely do a followup to add an error return to PodSets().

The theoretical possibility of an error return from awutils.GetPodSets() arises because if there isn't a cached version of the PodSets in the AppWrapper's status field, GetPodSets has to call unstructured.UnstructuredJSONScheme.Decode to process theruntime.RawExtension that represents the wrapped objects to let it build the PodSets. The validating web hook does this exact same decoding during AppWrapper creation and the update web hook enforces that the field holding the runtime.RawExtension is immutable. So an error happening here seems extremely unlikely, but totally agree that if it does happen that obscuring it makes it even harder to figure out.

Cool, in that case I would suggest: to add a log in this PR and open a TODO(#....) issue for the follow up

Done. Opened #3969 and added a TODO and log message for now.

mimowo · 2025-01-13T13:02:48Z

go.mod

@@ -52,6 +53,7 @@ require (
 	github.com/coreos/go-semver v0.3.1 // indirect
 	github.com/coreos/go-systemd/v22 v22.5.0 // indirect
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
+	github.com/distribution/reference v0.5.0 // indirect


Could you briefly summarize what the dependecy is used for (same for the one below). I would suggest you to consider using dependabot so that the dependencies are not getting outdated.

When it constructs the PodSetInfos from the runtime.RawExtension the AppWrapper controller has to replicate the processing of default values that the apiServer would apply to PodSpecsTemplates if they were created "normally.". The github.com/distribution/reference is used here to figure out if the image has a latest tag, which in turn is used to default the imagePullPolicy.

github.com/opencontainers/go-digest is pulled in as dependency by github.com/distribution/reference.

If we changed the equality comparison for PodSpecs to be less stringent (#3103), then we probably could avoid the need to do this defaulting, which would then eliminate the dependencies.

Thanks for explaining. I'm ok with the dependency for now.

mimowo · 2025-01-13T13:03:01Z

I went through the code and lgtm. Comments which can be addressed in follow ups:

e2e test for the integration
avoid silencing errors
consider using dependabot for the project to make sure the transitive dependencies are not outdated

dgrove-oss · 2025-01-13T15:31:54Z

consider using dependabot for the project to make sure the transitive dependencies are not outdated

Thanks for pointing this out; I'd missed that dependbot hadn't been fully enabled by the settings for https://github.com/project-codeflare/appwrapper. It is now.

dgrove-oss · 2025-01-13T16:25:33Z

For the e2e tests, I'm missing something that's going to be obvious in retrospect..

where is the logic that installs the desired version of the external controllers (training-operator, jobset controller, etc) on the test kind clusters? I'd need to extend to install the appwrapper controller.

dgrove-oss · 2025-01-13T16:44:53Z

BTW, it TAS supported? I believe not, because I see that SetPodSetInfos does not copy the SchedulingGates which needed. Do you plan to fill in the gap?

AppWrapper had been sitting at Kueue 0.8 (which didn't have the SchedulingGates) due to our need to run on OpenShift. But #3908 enables Kueue 0.10 to run on OpenShift, so we were able to move AppWrapper forward. I'll be looking at adding in SchedulingGate support this week, with a plan of making an AppWrapper point release once we have it. Then we can update the AppWrapper version dependency for Kueue and enable TAS support in the AppWrapper integration.

dgrove-oss · 2025-01-13T19:10:11Z

For the e2e tests, I'm missing something that's going to be obvious in retrospect..

where is the logic that installs the desired version of the external controllers (training-operator, jobset controller, etc) on the test kind clusters? I'd need to extend to install the appwrapper controller.

Found it.... and it was obvious in retrospect (makefile-test + e2e-common.sh)

dgrove-oss · 2025-01-14T13:37:46Z

I added a simple e2e test of an AppWrapper around a batch.Job that checks the basic functionality of the integration (AppWrapepr is admitted, Job is created and runs successfully, AppWrapper enters a completed state). Test runs in 15ish seconds, so that seems like a reasonable start.

mimowo

LGTM

mimowo · 2025-01-14T16:14:54Z

test/e2e/singlecluster/appwrapper_test.go

+				Parallelism(int32(numPods)).
+				Completions(int32(numPods)).
+				Suspend(false).
+				Image(util.E2eTestSleepImage, []string{"10s"}).


I prefer to make it end immediately like after 10ms. Is the only issue that you want to assert that Appswrapper is running? I think from the integration PoV he most important thing is that it gets scheduled and completes successfully.

I would expect the creation of pods etc to be tested outside of Kueue.

Maybe we can add that asserts when proven to be important based on specific issues.

Also, if we want to assert on the running Appswrapper and the pods we could have another test with longer timeout but after the asserts just kill the pods.

Makes sense. I'll revise as suggested later today.

Test simplified as requested. Runs a 10ms batch.Job. Only checks for AppWrapper (a) being unsuspended and (b) completing successfully.

dgrove-oss · 2025-01-14T20:08:17Z

/test pull-kueue-verify-main

mimowo · 2025-01-15T10:18:58Z

/lgtm
/approve

Awesome, thanks for the contribution! I think it will de-facto enable running custom Jobs on MultiKueue wrapped with the AppWrapper. As a follow up I would like yet to see the integration of AppWrapper with MultiKueue, based on managedBy with e2e tests for that.

k8s-ci-robot · 2025-01-15T10:19:05Z

LGTM label has been added.

Git tree hash: 444f0c64bd562541dc5542f0fb25175de1d2c0cc

k8s-ci-robot · 2025-01-15T10:19:06Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~OWNERS~~ [mimowo]

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 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 Jan 9, 2025

k8s-ci-robot requested review from mbobrovskyi and tenzen-y January 9, 2025 22:51

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 9, 2025

mimowo reviewed Jan 13, 2025

View reviewed changes

dgrove-oss added 6 commits January 13, 2025 11:07

bare bones appwrapper integration (no tests)

c281788

basic appwrapper_controller tests

9cf4329

appwrapper integration tests

7738abc

appwrapper documentation

ca8957d

linter fixes

24eb983

update appwrapper rbacs

25f12bd

dgrove-oss mentioned this pull request Jan 13, 2025

Extend GenericJob.GetPodSets() to allow error to be returned #3969

Open

review comment: log error and add TODO with issue link

88bb3a2

dgrove-oss force-pushed the appwrapper branch from b0e7fae to 88bb3a2 Compare January 13, 2025 17:48

dgrove-oss force-pushed the appwrapper branch from 977f758 to fd21974 Compare January 13, 2025 22:09

simple e2e test for appwrapper integration

8a7e500

dgrove-oss force-pushed the appwrapper branch from fd21974 to 8a7e500 Compare January 13, 2025 22:14

dgrove-oss added 2 commits January 13, 2025 17:36

fix typo in addition to run-test-e2e target

ad8e215

enable appwrapper framework in e2e tests

0c66206

dgrove-oss added 5 commits January 13, 2025 17:59

add appwrapper to scheme for e2e tests

2cbe5dd

improve assertions in appwrapper e2e test

f2a0a92

work around creationTimestamp serialization issue

bb708e4

add assert that appwrapper reaches running state

88630e1

wrapped job needs suspend to be false

60c7ed2

mimowo reviewed Jan 14, 2025

View reviewed changes

review comment: simplify/shorten appwrapper e2e test

a9a1d51

k8s-ci-robot assigned mimowo Jan 15, 2025

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2025

k8s-ci-robot merged commit d71331b into kubernetes-sigs:main Jan 15, 2025
17 checks passed

k8s-ci-robot added this to the v0.11 milestone Jan 15, 2025

dgrove-oss deleted the appwrapper branch January 15, 2025 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AppWrapper integration #3953

AppWrapper integration #3953

dgrove-oss commented Jan 9, 2025

netlify bot commented Jan 9, 2025 •

edited

Loading

kannon92 commented Jan 10, 2025

mimowo commented Jan 13, 2025

mimowo Jan 13, 2025

dgrove-oss Jan 13, 2025

mimowo Jan 13, 2025

dgrove-oss Jan 13, 2025

mimowo Jan 13, 2025

dgrove-oss Jan 13, 2025

mimowo Jan 13, 2025

mimowo commented Jan 13, 2025

dgrove-oss commented Jan 13, 2025

dgrove-oss commented Jan 13, 2025

dgrove-oss commented Jan 13, 2025 •

edited

Loading

dgrove-oss commented Jan 13, 2025 •

edited

Loading

dgrove-oss commented Jan 14, 2025

mimowo left a comment

mimowo Jan 14, 2025

dgrove-oss Jan 14, 2025

dgrove-oss Jan 14, 2025

mimowo Jan 15, 2025

dgrove-oss commented Jan 14, 2025

mimowo commented Jan 15, 2025

k8s-ci-robot commented Jan 15, 2025

k8s-ci-robot commented Jan 15, 2025

AppWrapper integration #3953

AppWrapper integration #3953

Conversation

dgrove-oss commented Jan 9, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Jan 9, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

kannon92 commented Jan 10, 2025

mimowo commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo commented Jan 13, 2025

dgrove-oss commented Jan 13, 2025

dgrove-oss commented Jan 13, 2025

dgrove-oss commented Jan 13, 2025 • edited Loading

dgrove-oss commented Jan 13, 2025 • edited Loading

dgrove-oss commented Jan 14, 2025

mimowo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrove-oss commented Jan 14, 2025

mimowo commented Jan 15, 2025

k8s-ci-robot commented Jan 15, 2025

k8s-ci-robot commented Jan 15, 2025

netlify bot commented Jan 9, 2025 •

edited

Loading

dgrove-oss commented Jan 13, 2025 •

edited

Loading

dgrove-oss commented Jan 13, 2025 •

edited

Loading