Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report all nodes in lease request #2401

Merged
merged 9 commits into from
May 12, 2023
Merged

Conversation

JamesMurkin
Copy link
Contributor

@JamesMurkin JamesMurkin commented Apr 27, 2023

We now send all nodes in the cluster in the lease call

We used to only send nodes that could currently be scheduled on however that caused issues:

  • Gang jobs were not getting pre-empted if they were on a node that was excluded for reasons such as being cordoned
  • The armada-scheduler expects all jobs being run to be returned each lease call. The jobs are on the node object, so if we excluded nodes that were cordoned, the armada-scheduler assumed the job has been lost

We now set Unschedulable to mark if the executor thinks jobs can be scheduled on this node or not

┆Issue is synchronized with this Jira Task by Unito

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Patch coverage: 22.22% and no project coverage change.

Comparison is base (cbcbd5d) 58.49% compared to head (f17fc7f) 58.49%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2401   +/-   ##
=======================================
  Coverage   58.49%   58.49%           
=======================================
  Files         234      234           
  Lines       29296    29296           
=======================================
  Hits        17136    17136           
  Misses      10844    10844           
  Partials     1316     1316           
Flag Coverage Δ
armada-server 58.49% <22.22%> (ø)

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

Impacted Files Coverage Δ
...ternal/executor/utilisation/cluster_utilisation.go 41.07% <18.75%> (ø)
internal/executor/node/node_group.go 87.87% <50.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JamesMurkin JamesMurkin added the do-not-merge Do not merge this PR label Apr 28, 2023
@JamesMurkin JamesMurkin marked this pull request as ready for review May 11, 2023 10:24
@JamesMurkin JamesMurkin removed the do-not-merge Do not merge this PR label May 11, 2023
@JamesMurkin JamesMurkin enabled auto-merge (squash) May 12, 2023 08:39
@JamesMurkin JamesMurkin merged commit 0fd3091 into master May 12, 2023
32 of 33 checks passed
@JamesMurkin JamesMurkin deleted the report_all_nodes_lease_request branch May 12, 2023 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants