fix: add missing k8s job submission times to allocations #9028

hamidzr · 2024-03-20T18:50:47Z

Description

add missing job submission time on k8s assigned resources
reuse average stats calculation between rms

TODO:

release notes
tests
[no] rectify/address old db records? https://hpe-aiatscale.atlassian.net/browse/RM-111

Test Plan

have a k8s cluster,
submit jobs and check the queued stats for the corresponding resource pool the queued values should not be in the range of years (more likely seconds or minutes)

Commentary (optional)

We might also want to run a migration or add a measure to ignore erroneous previous records
or rectify them through tasks.start_time or as an approximation or allocation start time
perhaps via a migration

https://hpe.sharepoint.com/:w:/r/teams/detai/Shared%20Documents/Engineering/Resource%20Management/Random%20Things/Web%20Cluster%20Requirements.docx?d=waae7403bf7b04e0fb9832ac010108574&csf=1&web=1&e=vqsH06

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.

Ticket

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

netlify · 2024-03-20T18:51:07Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`72a1375`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/660d7ddc9774ba0007d1639a
😎 Deploy Preview	https://deploy-preview-9028--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-03-20T19:04:38Z

Codecov Report

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

Project coverage is 47.10%. Comparing base (f78b9aa) to head (72a1375).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9028      +/-   ##
==========================================
+ Coverage   47.07%   47.10%   +0.02%     
==========================================
  Files        1155     1156       +1     
  Lines      142400   142378      -22     
  Branches     2421     2423       +2     
==========================================
+ Hits        67034    67064      +30     
+ Misses      75176    75124      -52     
  Partials      190      190

Flag	Coverage Δ
backend	`43.03% <72.72%> (+0.09%)`	⬆️
harness	`64.07% <ø> (ø)`
web	`38.95% <ø> (ø)`

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

Files	Coverage Δ
...ster/internal/rm/agentrm/agent_resource_manager.go	`45.04% <100.00%> (-2.95%)`	⬇️
master/internal/rm/kubernetesrm/resource_pool.go	`17.80% <100.00%> (+12.39%)`	⬆️
master/pkg/model/task.go	`15.26% <ø> (ø)`
master/internal/db/postgres_tasks.go	`69.23% <66.66%> (+0.36%)`	⬆️
...nal/rm/kubernetesrm/kubernetes_resource_manager.go	`23.45% <0.00%> (+2.02%)`	⬆️
master/internal/rm/agentrm/agent.go	`25.42% <0.00%> (ø)`
master/internal/rm/tasklist/task_list.go	`9.63% <0.00%> (-0.24%)`	⬇️
master/internal/rm/db.go	`80.64% <80.64%> (ø)`

... and 6 files with indirect coverage changes

hamidzr · 2024-03-20T21:02:52Z

master/internal/rm/db.go

+)
+
+// FetchAvgQueuedTime fetches the average queued time for a resource pool.
+func FetchAvgQueuedTime(pool string) (


are there any Go practices for keeping the exposure limited this and subpackages aka rm/* ?

hamidzr · 2024-03-25T15:54:59Z

any suggestions on the best way to add automated tests here?

stoksc · 2024-03-26T20:22:37Z

master/internal/task/allocation.go

-		// allocation.startTime?
-		StartTime: &msg.JobSubmissionTime,
-		EndTime:   &now,
+		StartTime:    &msg.JobSubmissionTime,


hmm. i think JobSubmissionTime is wrong. if anything the language here is wrong. "task stats" implies this is talking about the task level, but job submission time is when the experiment was submitted.

yeah, I think there's more to look at here but it isn't immediately obvious to me and that'd be a different level of wrong than the current state of "we put time.zero as the start of queued period for k8s". Does it make sense to work on that on a separate ticket w/ a different priority?

yeah, they're equally totally wrong, so i'd lean to just fixing it. i wouldnt call this commit "fix: k8s rp queued stats" if you do it in two PRs.

that's fair. #9028 (comment)

hamidzr · 2024-04-02T19:56:15Z

I updated the title and the associated ticket.
I created a branch off of this to work on and explore bigger change to how we compute these stats for various RMs
#9088

NicholasBlaskey

I'm approving because there is nothing in the PR I disagree with

I don't think it makes average queued time much more correct. We still are not having the right end time. We end the tasks stats when we get resources allocated, which in kubernetes is going to be roughly right after we submit the job. We really want to change this till after the pod gets gpus assigned. Maybe a better change is just hiding the chart on Kubernetes / Slurm clusters until grafana dashboards can include queued time information?

I'm cool if you want to land this since I think it is a slight improvement.

master/internal/db/postgres_tasks.go

master/internal/rm/db.go

master/internal/rm/kubernetesrm/resource_pool.go

NicholasBlaskey · 2024-04-02T20:16:14Z

master/internal/rm/kubernetesrm/kubernetes_resource_manager.go

@@ -572,39 +572,7 @@ func (k *ResourceManager) createResourcePoolSummary(
 func (k *ResourceManager) fetchAvgQueuedTime(pool string) (
 	[]*jobv1.AggregateQueueStats, error,
 ) {
-	aggregates := []model.ResourceAggregates{}


the database query doesn't change at all in this PR right?

no the pr should not have any effectvie change other than

add missing job submission time on k8s assigned resources
reuse average stats calculation between rms

master/internal/task/allocation.go

docs/release-notes/job-queued-stats.rst

identical pathes to get avg queued time fix warn remove shell functions

This reverts commit 81bb5bd.

This reverts commit d5fb289.

cla-bot bot added the cla-signed label Mar 20, 2024

hamidzr changed the title ~~Hz rm34 queued stats~~ fix rm queued stats Mar 20, 2024

hamidzr changed the title ~~fix rm queued stats~~ fix: k8s rp queued stats Mar 20, 2024

hamidzr force-pushed the hz-rm34-queued-stats branch from c301769 to ea836d5 Compare March 20, 2024 19:00

hamidzr commented Mar 20, 2024

View reviewed changes

hamidzr marked this pull request as ready for review March 25, 2024 15:54

hamidzr requested review from a team as code owners March 25, 2024 15:54

hamidzr requested a review from stoksc March 25, 2024 15:54

determined-ci added the documentation Improvements or additions to documentation label Mar 25, 2024

determined-ci requested a review from a team March 25, 2024 16:02

stoksc reviewed Mar 26, 2024

View reviewed changes

hamidzr changed the title ~~fix: k8s rp queued stats~~ fix: add missing k8s job submission times to allocations Apr 2, 2024

NicholasBlaskey approved these changes Apr 2, 2024

View reviewed changes

determined-ci removed the documentation Improvements or additions to documentation label Apr 3, 2024

hamidzr added 10 commits April 3, 2024 11:03

refactor and code reuse

0da81ce

identical pathes to get avg queued time fix warn remove shell functions

the actual fix: add missing JobSubmissionTime

df0fe3a

some comments

64ae3c1

warn about allocations w/ job submission time

b2dbd0f

docs: add release notes

78b5809

Revert "warn about allocations w/ job submission time"

da9e257

This reverts commit 81bb5bd.

test assigned gets a submission time

4b393ec

rev comments

740e486

Reapply "warn about allocations w/ job submission time"

a81596e

This reverts commit d5fb289.

remove release notes, wrap some errors

72a1375

hamidzr force-pushed the hz-rm34-queued-stats branch from e60e87e to 72a1375 Compare April 3, 2024 16:03

hamidzr merged commit f03a8a8 into main Apr 4, 2024
68 of 81 checks passed

hamidzr deleted the hz-rm34-queued-stats branch April 4, 2024 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add missing k8s job submission times to allocations #9028

fix: add missing k8s job submission times to allocations #9028

hamidzr commented Mar 20, 2024 •

edited

Loading

netlify bot commented Mar 20, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

hamidzr Mar 20, 2024

hamidzr commented Mar 25, 2024

stoksc Mar 26, 2024 •

edited

Loading

hamidzr Mar 27, 2024 •

edited

Loading

stoksc Mar 28, 2024

hamidzr Apr 2, 2024 •

edited

Loading

hamidzr commented Apr 2, 2024

NicholasBlaskey left a comment

NicholasBlaskey Apr 2, 2024

hamidzr Apr 2, 2024

fix: add missing k8s job submission times to allocations #9028

fix: add missing k8s job submission times to allocations #9028

Conversation

hamidzr commented Mar 20, 2024 • edited Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

netlify bot commented Mar 20, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented Mar 20, 2024 • edited Loading

Codecov Report

hamidzr Mar 20, 2024

Choose a reason for hiding this comment

hamidzr commented Mar 25, 2024

stoksc Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

hamidzr Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

stoksc Mar 28, 2024

Choose a reason for hiding this comment

hamidzr Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

hamidzr commented Apr 2, 2024

NicholasBlaskey left a comment

Choose a reason for hiding this comment

NicholasBlaskey Apr 2, 2024

Choose a reason for hiding this comment

hamidzr Apr 2, 2024

Choose a reason for hiding this comment

hamidzr commented Mar 20, 2024 •

edited

Loading

netlify bot commented Mar 20, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

stoksc Mar 26, 2024 •

edited

Loading

hamidzr Mar 27, 2024 •

edited

Loading

hamidzr Apr 2, 2024 •

edited

Loading