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	`96f1ce4`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/664e6d94bdb6a6000850cfea
😎 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

All modified and coverable lines are covered by tests ✅

Project coverage is 40.60%. Comparing base (ca45198) to head (96f1ce4).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9296      +/-   ##
==========================================
- Coverage   48.57%   40.60%   -7.98%     
==========================================
  Files        1234      662     -572     
  Lines      158841    77260   -81581     
  Branches     2778        0    -2778     
==========================================
- Hits        77155    31368   -45787     
+ Misses      81511    45892   -35619     
+ Partials      175        0     -175

Flag	Coverage Δ
harness	`37.91% <ø> (-26.11%)`	⬇️
web	`?`

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

Files	Coverage Δ
master/internal/db/postgres_test_utils.go	`82.37% <ø> (+0.21%)`	⬆️
master/internal/rm/agentrm/resource_pool.go	`34.20% <ø> (+0.07%)`	⬆️
master/internal/rm/kubernetesrm/informer.go	`81.92% <ø> (+0.97%)`	⬆️
...nal/rm/kubernetesrm/kubernetes_resource_manager.go	`29.15% <ø> (+0.76%)`	⬆️
master/internal/rm/kubernetesrm/request_queue.go	`86.58% <ø> (+1.96%)`	⬆️
master/internal/rm/kubernetesrm/request_workers.go	`89.58% <ø> (+1.05%)`	⬆️
master/internal/rm/kubernetesrm/resource_pool.go	`44.47% <ø> (+1.09%)`	⬆️
master/internal/rm/kubernetesrm/spec.go	`73.16% <ø> (+0.98%)`	⬆️
master/internal/sproto/resources.go	`23.94% <ø> (-1.06%)`	⬇️
master/internal/sproto/task.go	`41.37% <ø> (+0.47%)`	⬆️
... and 7 more

... and 671 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.

stoksc · 2024-05-17T23:36:35Z

master/internal/rm/kubernetesrm/job.go

@@ -0,0 +1,751 @@
+package kubernetesrm


note to reviewer: this code started as a copy of the old pod.go (but modified so heavily github's diff algorithm switch from calling it a rename+changes to a delete+rewrite). jobs.go is the same, copied from pods.go. I'm calling this out to say: I refactored the code as I thought it was necessary and as it made me more confident in its correctness but I didn't go a lot further. I'll probably do a style-oriented refactor once this PR is in (it's all going to a feature branch for now).

dannysauer · 2024-05-18T14:25:16Z

.circleci/real_config.yml

@@ -2158,6 +2158,8 @@ jobs:
      - setup-python-venv:
          executor: <<pipeline.parameters.machine-image>>
      - setup-go-intg-deps
+      - run: curl -LO https://storage.googleapis.com/minikube/releases/latest/minikube-linux-amd64 && sudo install minikube-linux-amd64 /usr/local/bin/minikube


Maybe not a thing for this PR, but can we move this (and maybe the start too) out to a command so we don't have a proliferation of these huge command lines everywhere minikube is used? Should I open that as an issue somewhere?

sounds like a job for infra

yeah can do. but fyi this is the only such line, I cherry picked from carolina's PR you reviewed. it'll go away once she lands and i rebase. mb, should've waited to mark it ready for review. would've saved you a bit of time.

Ah, ok. Cool.

fyi, I merged the PR, so you can rebase against main to pick it up!

dannysauer

Infra-relevant parts look fine.

Is "Eventually we can deleted restarts..." in the PR description supposed to be "delegate" though?

stoksc · 2024-05-18T17:38:41Z

Yep typo, thanks.

carolinaecalderon

got through everything but jobs.go/jobs_test.go -- everything looks like it checks out, excited for the bug bash tomorrow!

master/internal/rm/kubernetesrm/job.go

master/internal/rm/kubernetesrm/jobs.go

master/internal/rm/kubernetesrm/resource_pool.go

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

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

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.

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

stoksc force-pushed the stoksc/feat/pods2jobs branch from aad1242 to a94c3c0 Compare May 17, 2024 23:26

stoksc removed request for a team and ashtonG May 17, 2024 23:27

stoksc commented May 17, 2024

View reviewed changes

dannysauer reviewed May 18, 2024

View reviewed changes

dannysauer approved these changes May 18, 2024

View reviewed changes

JComins000 approved these changes May 20, 2024

View reviewed changes

carolinaecalderon reviewed May 20, 2024

View reviewed changes

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 added 12 commits May 22, 2024 12:11

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

a28a071

various logging fixes

9775bf3

various review feedback

4624f53

bring back user-readable pod names

b40ea50

rearrange jobsService

cc16c76

various renames and refactor

9b2cac6

get rid of job submission info

18b259e

add isSuccessfulExit

7047669

various pr feedback

3557647

various pr feedback

8b3ce4a

various fixes

96f1ce4

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

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

netlify bot commented May 2, 2024 •

edited

codecov bot commented May 2, 2024 •

edited

carolinaecalderon left a comment

stoksc commented May 17, 2024

stoksc May 17, 2024 •

edited

dannysauer May 18, 2024

JComins000 May 19, 2024

stoksc May 20, 2024

dannysauer May 20, 2024

carolinaecalderon May 20, 2024

dannysauer left a comment

stoksc commented May 18, 2024

carolinaecalderon left a comment

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

feat: convert k8s submissions from pods to jobs #9296

Are you sure you want to change the base?

feat: convert k8s submissions from pods to jobs #9296

Conversation

stoksc commented May 2, 2024 • edited

Ticket

Description

Test Plan

Checklist

netlify bot commented May 2, 2024 • edited

✅ Deploy Preview for determined-ui ready!

codecov bot commented May 2, 2024 • edited

Codecov Report

carolinaecalderon left a comment

Choose a reason for hiding this comment

stoksc commented May 17, 2024

stoksc May 17, 2024 • edited

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

dannysauer left a comment

Choose a reason for hiding this comment

stoksc commented May 18, 2024

carolinaecalderon left a comment

Choose a reason for hiding this comment

carolinaecalderon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tara-det-ai 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

stoksc commented May 2, 2024 •

edited

netlify bot commented May 2, 2024 •

edited

codecov bot commented May 2, 2024 •

edited

stoksc May 17, 2024 •

edited