feat: convert k8s submissions from pods to jobs #9296

stoksc · 2024-05-02T20:43:55Z

Ticket

[RM-203,RM-204,RM-205,RM-206,RM-208,RM-213]

Description

Update our Kubernetes resource manager to submit one job per Determined task instead of many pods. This is a complicated change but we think it is worth it because:

Jobs play nice with resource quotas and other Kubernetes features out of the box.
Eventually we can delegate restarts, TTL, pause/resume (using suspend), and more to jobs.
They allow us to integrate with kueue immediately.
If we want to support VolcanoJobs we are much closer (and it is easier to maintain Job+VolcanoJob than Pods+VolcanoJob).

Test Plan

Covered by automated tests.

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.
File ticket for rp.namespace weirdness, graceful shutdown, using indexed completions

netlify · 2024-05-02T20:44:12Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`8806bff`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6656522457a5fe00081f0f51
😎 Deploy Preview	https://deploy-preview-9296--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

codecov · 2024-05-02T20:46:38Z

Codecov Report

Attention: Patch coverage is 75.71014% with 419 lines in your changes are missing coverage. Please review.

Project coverage is 49.03%. Comparing base (515c135) to head (8806bff).

Additional details and impacted files

@@                      Coverage Diff                       @@
##           stoksc/feat/kubernetesjobs    #9296      +/-   ##
==============================================================
+ Coverage                       48.60%   49.03%   +0.43%     
==============================================================
  Files                            1233     1233              
  Lines                          158981   159205     +224     
  Branches                         2778     2777       -1     
==============================================================
+ Hits                            77271    78074     +803     
+ Misses                          81536    80957     -579     
  Partials                          174      174

Flag	Coverage Δ
backend	`43.56% <77.00%> (+1.41%)`	⬆️
harness	`63.97% <9.09%> (-0.05%)`	⬇️
web	`44.38% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/internal/rm/kubernetesrm/informer.go	`86.36% <100.00%> (+5.41%)`	⬆️
master/internal/rm/kubernetesrm/log.go	`90.00% <100.00%> (+0.52%)`	⬆️
master/internal/sproto/task_actor.go	`57.14% <ø> (+9.52%)`	⬆️
master/pkg/cproto/state.go	`31.70% <ø> (+14.63%)`	⬆️
master/internal/rm/agentrm/resource_pool.go	`34.13% <0.00%> (ø)`
master/internal/rm/kubernetesrm/request_queue.go	`89.89% <94.11%> (+5.28%)`	⬆️
master/internal/sproto/task.go	`40.90% <0.00%> (ø)`
master/internal/webhooks/postgres_webhook.go	`57.10% <0.00%> (ø)`
master/internal/db/postgres_test_utils.go	`82.00% <60.00%> (-0.16%)`	⬇️
master/internal/task/allocation.go	`76.35% <80.00%> (-0.05%)`	⬇️
... and 11 more

... and 4 files with indirect coverage changes

carolinaecalderon

notes from my first skim through

master/internal/rm/kubernetesrm/job.go

helm/charts/determined/values.yaml

helm/charts/determined/templates/master-permissions.yaml

helm/charts/determined/templates/master-deployment.yaml

master/internal/rm/kubernetesrm/spec.go

stoksc · 2024-05-17T23:24:00Z

All the failing tests are also failing on main, but I'm going to make an attempt to fix the relevant ones before landing at least.

carolinaecalderon

Approved, but before you merge I pointed out 2 changes that I think should either be called out explicitly in the PR description or given their own PR because I think they're unrelated to this feat
Also, I think you should reword the log messages into something a little more clear by putting the action verb first.
Besides that, just style comments, which I assume will make it into their own PR

carolinaecalderon · 2024-05-21T16:05:59Z

master/internal/rm/kubernetesrm/job.go

+		j.syslog.Infof("saw pod %s in state %s", podName, cproto.Pulling)
+		j.container.State = cproto.Pulling
+		j.informTaskResourcesState()
+
+		j.syslog.Infof("saw pod %s in state %s", podName, cproto.Starting)


nit: I don't love the wording of these "saw pod X in state Y", maybe try "pulling/starting/ pod %s" --> strings.ToLower(cproto.Pulling) + " pod " + stringName

tara-det-ai

LGTM

NicholasBlaskey · 2024-05-23T20:06:58Z

master/Makefile

@@ -99,11 +99,13 @@ build/mock_gen.stamp: $(MOCK_INPUTS)
 	mockery --quiet --name=PodInterface --srcpkg=k8s.io/client-go/kubernetes/typed/core/v1 --output internal/mocks --filename pod_iface.go
 	mockery --quiet --name=EventInterface --srcpkg=k8s.io/client-go/kubernetes/typed/core/v1 --output internal/mocks --filename event_iface.go
 	mockery --quiet --name=NodeInterface --srcpkg=k8s.io/client-go/kubernetes/typed/core/v1 --output internal/mocks --filename node_iface.go
+	mockery --quiet --name=JobInterface --srcpkg=k8s.io/client-go/kubernetes/typed/batch/v1 --output internal/mocks --filename job_iface.go


we need to switch to a config file ASAP

https://hpe-aiatscale.atlassian.net/browse/RM-277

NicholasBlaskey · 2024-05-23T20:20:24Z

master/internal/rm/kubernetesrm/resource_pool.go

 			req.State = msg.State
 			if sproto.ScheduledStates[req.State] {
-				k.allocationIDToRunningPods[id]++
+				k.allocationIDToRunningPods[msg.AllocationID] += msg.NumPods


does the job know how many pods it has running, it is a little tragic that we need to keep track of this map

feel free to just make a follow up ticket or ignore any of these

I agree, tragic but no it doesn't know how many are "running" where our definition of "running" is post scheduling and bound to a node.

various ci fixes add consts fix import style revert unneeded helm changes last bit of review feedback lint fixes bring in carolina's config changes tmp stuff that i'm definitely keeping amends lints debug logging for weird failure only in CI debug logging test fixes test fixes fixes for reattach tests self review more self review fix annoyance pass numPods to recreateJobHandler final self review fix: job queue state not recovered on reattach various fixes

stoksc · 2024-05-28T21:54:03Z

landing this into the feature branch (as soon as I get green tests) where it will get a release note, docs, extra tests, and maybe some more adjustments (nothing major) before it lands later this week.

This change updates the Kubernetes resource manager to submit one Kubernetes job per Determined allocation instead of many pods. This is complicated but we think it is worth it because: - Jobs play nice with resource quotas and other Kubernetes features out of the box. - Eventually we can delegate restarts, TTL, pause/resume (using suspend), and more to jobs. - They allow us to better integrate with Kueue and other tools in the ml ecosystem. - Supporting VolcanoJobs (or similar alternatives) alongside Jobs is realistic. - The refactor is net positive w.r.t. test coverage (20% to 80%) and code quality. This commit is the result of several PRs, enumerated here for easier discovery. - #9296 contains most of the code changes. - #9443 - #9447 - #9450 - #9451 Co-authored-by: Carolina Calderon <carolina.calderon@hpe.com>

cla-bot bot added the cla-signed label May 2, 2024

stoksc force-pushed the stoksc/feat/pods2jobs branch 2 times, most recently from 07bfc25 to e99300c Compare May 6, 2024 21:36

carolinaecalderon reviewed May 7, 2024

View reviewed changes

stoksc force-pushed the stoksc/feat/pods2jobs branch from a49c29b to dd9f56d Compare May 9, 2024 19:17

stoksc changed the title ~~Stoksc/feat/pods2jobs~~ feat: convert k8s submissions from pods to jobs May 9, 2024

stoksc force-pushed the stoksc/feat/pods2jobs branch 5 times, most recently from ae8e8e0 to 101af3d Compare May 16, 2024 18:27

stoksc force-pushed the stoksc/feat/pods2jobs branch 4 times, most recently from e8a4aab to aad1242 Compare May 17, 2024 21:17

stoksc marked this pull request as ready for review May 17, 2024 23:01

stoksc requested review from a team as code owners May 17, 2024 23:01

stoksc requested review from carolinaecalderon, MikhailKardash and JComins000 May 17, 2024 23:01

stoksc changed the base branch from main to stoksc/feat/kubernetesjobs May 17, 2024 23:25

stoksc requested review from a team as code owners May 17, 2024 23:25

stoksc requested a review from ashtonG May 17, 2024 23:25

carolinaecalderon approved these changes May 21, 2024

View reviewed changes

stoksc force-pushed the stoksc/feat/pods2jobs branch from 6fa894e to 6d8e486 Compare May 21, 2024 23:29

determined-ci requested a review from a team May 21, 2024 23:29

determined-ci added the documentation Improvements or additions to documentation label May 21, 2024

stoksc changed the base branch from stoksc/feat/kubernetesjobs to main May 22, 2024 00:40

determined-ci removed the documentation Improvements or additions to documentation label May 22, 2024

tara-det-ai approved these changes May 22, 2024

View reviewed changes

stoksc force-pushed the stoksc/feat/pods2jobs branch from ed5620b to 96f1ce4 Compare May 22, 2024 22:11

NicholasBlaskey approved these changes May 23, 2024

View reviewed changes

stoksc force-pushed the stoksc/feat/pods2jobs branch from 2274790 to 10af218 Compare May 28, 2024 18:52

stoksc added 13 commits May 28, 2024 17:51

fix 'Kill a pod in pod status phase pending results in a success'

5b85ebf

various logging fixes

f3b4b0a

various review feedback

8e1fffa

bring back user-readable pod names

604e37a

rearrange jobsService

f6660be

various renames and refactor

dc01f0f

get rid of job submission info

d7a2304

add isSuccessfulExit

bd5ad8a

various pr feedback

f940aa3

various pr feedback

8eb4527

various fixes

0e52fed

various test and lint fixes

8806bff

stoksc force-pushed the stoksc/feat/pods2jobs branch from 10af218 to 8806bff Compare May 28, 2024 21:52

stoksc changed the base branch from main to stoksc/feat/kubernetesjobs May 28, 2024 21:53

stoksc merged commit 66f9d1e into stoksc/feat/kubernetesjobs May 29, 2024
91 of 102 checks passed

stoksc deleted the stoksc/feat/pods2jobs branch May 29, 2024 00:52

stoksc mentioned this pull request May 31, 2024

feat: convert k8s submissions from pods to jobs (#9296) #9438

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: convert k8s submissions from pods to jobs #9296

feat: convert k8s submissions from pods to jobs #9296

stoksc commented May 2, 2024 •

edited

Loading

netlify bot commented May 2, 2024 •

edited

Loading

codecov bot commented May 2, 2024 •

edited

Loading

carolinaecalderon left a comment

stoksc commented May 17, 2024

carolinaecalderon left a comment

carolinaecalderon May 21, 2024

tara-det-ai left a comment

NicholasBlaskey May 23, 2024

NicholasBlaskey May 23, 2024

stoksc May 23, 2024

stoksc commented May 28, 2024

feat: convert k8s submissions from pods to jobs #9296

feat: convert k8s submissions from pods to jobs #9296

Conversation

stoksc commented May 2, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented May 2, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented May 2, 2024 • edited Loading

Codecov Report

carolinaecalderon left a comment

Choose a reason for hiding this comment

stoksc commented May 17, 2024

carolinaecalderon left a comment

Choose a reason for hiding this comment

carolinaecalderon May 21, 2024

Choose a reason for hiding this comment

tara-det-ai left a comment

Choose a reason for hiding this comment

NicholasBlaskey May 23, 2024

Choose a reason for hiding this comment

NicholasBlaskey May 23, 2024

Choose a reason for hiding this comment

stoksc May 23, 2024

Choose a reason for hiding this comment

stoksc commented May 28, 2024

stoksc commented May 2, 2024 •

edited

Loading

netlify bot commented May 2, 2024 •

edited

Loading

codecov bot commented May 2, 2024 •

edited

Loading