feat: RM-130 add determined info as pod labels #9140

kkunapuli · 2024-04-10T13:20:31Z

Ticket

Description

Add Determined system information such as username, workspace name, and resource pool as pod labels for kubernetes environments.

Test Plan

Inspect pods created by Determined: kubectl describe pod <POD NAME> | grep grep determined.ai

For a notebook (or anything that's not an experiment), the labels should look something like this:

determined.ai/container_id=167b8380-9776-494d-acf6-5f8c29f52d52
determined.ai/resource_pool=default
determined.ai/task_id=3278502b-3d76-4a52-a902-9833166ef5e6
determined.ai/task_type=NOTEBOOK
determined.ai/user=determined
determined.ai/workspace=my-workspace

If the pod is running a trail, there will be two additional labels:

determined.ai/experiment_id=16
determined.ai/task_type=TRIAL
determined.ai/trial_id=23

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.

netlify · 2024-04-10T13:20:51Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`e8bee81`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/661ea23e22f4c6000818201f

codecov · 2024-04-10T13:24:20Z

Codecov Report

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

Project coverage is 38.34%. Comparing base (3f70a46) to head (e8bee81).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9140      +/-   ##
==========================================
- Coverage   45.71%   38.34%   -7.38%     
==========================================
  Files        1179      989     -190     
  Lines      146795   130092   -16703     
  Branches     2419     2420       +1     
==========================================
- Hits        67110    49886   -17224     
- Misses      79471    79992     +521     
  Partials      214      214

Flag	Coverage Δ
harness	`35.24% <ø> (-28.79%)`	⬇️
web	`35.82% <ø> (ø)`

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

Files	Coverage Δ
master/internal/api_command.go	`47.84% <100.00%> (+0.75%)`	⬆️
master/internal/core_experiment.go	`61.50% <100.00%> (ø)`
master/internal/rm/kubernetesrm/spec.go	`72.60% <100.00%> (+0.97%)`	⬆️
master/pkg/tasks/task.go	`48.67% <ø> (ø)`
master/pkg/tasks/task_trial.go	`0.00% <0.00%> (ø)`

... and 270 files with indirect coverage changes

master/internal/rm/kubernetesrm/resource_pool.go

master/internal/rm/kubernetesrm/spec.go

master/internal/core.go

master/internal/rm/kubernetesrm/spec_test.go

cla-bot · 2024-04-12T14:01:21Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @kkunapuli on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla.

After we approve your CLA, we will update the contributors list (private) and comment @cla-bot[bot] check to rerun the check.

master/internal/api_command.go

master/internal/core_experiment.go

NicholasBlaskey · 2024-04-12T14:34:50Z

master/internal/core_experiment.go

@@ -388,6 +389,11 @@ func (m *Master) parseCreateExperiment(ctx context.Context, req *apiv1.CreateExp

 	taskSpec.Project = config.Project()
 	taskSpec.Workspace = config.Workspace()
+	if p.Id > 1 {


We are changing Project/Workspace which is used in Slurm. Is this change of this going to affect any behavior in Slurm? Is this field not being set always causing any bugs in Slurm?

I know I said it was a bug but it is probably worth checking that this does not affect slurm.

master/internal/rm/kubernetesrm/spec.go

…9155)

* add helper scripts for new image publishing workflow.

master/internal/api_command.go

master/pkg/tasks/task_trial.go

master/internal/rm/kubernetesrm/spec_test.go

master/internal/rm/kubernetesrm/spec.go

Co-authored-by: Nick Blaskey <nick.blaskey@hpe.com>

NicholasBlaskey

LGTM

master/internal/api_command.go

Co-authored-by: Keita Nonaka <keita.nonaka@hpe.com> Co-authored-by: Thiago Dallacqua <104855841+thiagodallacqua-hpe@users.noreply.github.com> Co-authored-by: Michael Kardash <mikhail.kardash@hpe.com> Co-authored-by: Amanda Vialva <144278621+amandavialva01@users.noreply.github.com> Co-authored-by: Nick Blaskey <nick.blaskey@hpe.com>

first draft

5af5b2d

kkunapuli requested a review from a team as a code owner April 10, 2024 13:20

kkunapuli requested a review from NicholasBlaskey April 10, 2024 13:20

cla-bot bot added the cla-signed label Apr 10, 2024

kkunapuli marked this pull request as draft April 10, 2024 13:20

NicholasBlaskey reviewed Apr 10, 2024

View reviewed changes

master/internal/rm/kubernetesrm/resource_pool.go Outdated Show resolved Hide resolved

master/internal/rm/kubernetesrm/spec.go Outdated Show resolved Hide resolved

master/internal/rm/kubernetesrm/spec.go Outdated Show resolved Hide resolved

kkunapuli added 6 commits April 11, 2024 10:38

feedback

b04ccc8

add spec test coverage

3636e05

remove comment

f3ee686

add container id

2c2347d

format

958c1ba

fix checks

e540c0d

kkunapuli commented Apr 11, 2024

View reviewed changes

master/internal/core.go Outdated Show resolved Hide resolved

kkunapuli added 5 commits April 11, 2024 18:25

propagate project and workspace name from CLI

491dd55

add experiment and trial request IDs as labels

799dc3e

use empty string as a default

9aa14bd

add task id formatting checks for trials

e8a2503

missed tests

515c92c

kkunapuli changed the title ~~first draft~~ RM-130 add determined info as pod labels Apr 11, 2024

kkunapuli changed the title ~~RM-130 add determined info as pod labels~~ feat: RM-130 add determined info as pod labels Apr 11, 2024

kkunapuli commented Apr 12, 2024

View reviewed changes

master/internal/rm/kubernetesrm/spec_test.go Outdated Show resolved Hide resolved

Merge branch 'main' into kunapuli/add-pod-labels

bdb70ea

cla-bot bot removed the cla-signed label Apr 12, 2024

kkunapuli marked this pull request as ready for review April 12, 2024 14:05

kkunapuli requested a review from a team as a code owner April 12, 2024 14:05

kkunapuli requested a review from NicholasBlaskey April 12, 2024 14:05

NicholasBlaskey reviewed Apr 12, 2024

View reviewed changes

kkunapuli and others added 7 commits April 15, 2024 11:09

fetch default workspace from command

42b783d

always set workspace, project name

afa47d8

use map for arbitrary pod labels; check owner for nil

02ba5b3

fix: fix disable button condition in launch jupyter notebook modal (#…

a0c52e5

…9155)

fix: TensorBoard visualization from batch actions. (#9156)

f752177

chore: new image publishing (#9090)

c914a63

* add helper scripts for new image publishing workflow.

chore: style fix for helm tls (#9163)

30bda02

kkunapuli requested a review from a team as a code owner April 15, 2024 15:09

kkunapuli requested a review from julian-determined-ai April 15, 2024 15:09

cla-bot bot added the cla-signed label Apr 15, 2024

kkunapuli added 2 commits April 15, 2024 11:19

Merge remote-tracking branch 'origin' into kunapuli/add-pod-labels

327eefb

rerun tests

0192304

NicholasBlaskey reviewed Apr 15, 2024

View reviewed changes

kkunapuli and others added 6 commits April 15, 2024 18:26

add CanGetWorkspace mocks to intg tests

24e28d4

Update master/pkg/tasks/task_trial.go

ebac80a

Co-authored-by: Nick Blaskey <nick.blaskey@hpe.com>

Update master/internal/rm/kubernetesrm/spec_test.go

9901da7

Co-authored-by: Nick Blaskey <nick.blaskey@hpe.com>

add extraPodLabels coverage

9066a49

update comments

4196731

return error if workspace not found

e13a0e2

kkunapuli requested a review from NicholasBlaskey April 16, 2024 14:26

kkunapuli marked this pull request as draft April 16, 2024 14:26

kkunapuli removed request for a team and julian-determined-ai April 16, 2024 14:29

kkunapuli marked this pull request as ready for review April 16, 2024 14:29

NicholasBlaskey approved these changes Apr 16, 2024

View reviewed changes

master/internal/api_command.go Outdated Show resolved Hide resolved

don't wrap error

e8bee81

kkunapuli merged commit 5541e54 into main Apr 16, 2024
69 of 84 checks passed

kkunapuli deleted the kunapuli/add-pod-labels branch April 16, 2024 17:58

NicholasBlaskey mentioned this pull request Apr 18, 2024

test: can't launch NSC test assert 404 instead of 403 #9197

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: RM-130 add determined info as pod labels #9140

feat: RM-130 add determined info as pod labels #9140

kkunapuli commented Apr 10, 2024 •

edited

Loading

netlify bot commented Apr 10, 2024 •

edited

Loading

codecov bot commented Apr 10, 2024 •

edited

Loading

cla-bot bot commented Apr 12, 2024

NicholasBlaskey Apr 12, 2024

NicholasBlaskey left a comment

feat: RM-130 add determined info as pod labels #9140

feat: RM-130 add determined info as pod labels #9140

Conversation

kkunapuli commented Apr 10, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Apr 10, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

codecov bot commented Apr 10, 2024 • edited Loading

Codecov Report

cla-bot bot commented Apr 12, 2024

NicholasBlaskey Apr 12, 2024

Choose a reason for hiding this comment

NicholasBlaskey left a comment

Choose a reason for hiding this comment

kkunapuli commented Apr 10, 2024 •

edited

Loading

netlify bot commented Apr 10, 2024 •

edited

Loading

codecov bot commented Apr 10, 2024 •

edited

Loading