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

Allow configurable range of Job priorities #16084

Merged
merged 15 commits into from
Feb 17, 2023

Conversation

alessio-perugini
Copy link
Contributor

#15516

Summary

To allow our users to better fine-grain the job priorities, we're expanding the upper limit from 100 to MaxInt16-1.

  • CoreJobPriority is hard-coded to math.MaxInt16 (32767)
  • JobMaxPriority defaults to 100 and can be configured in the range 100 <= JobMaxPriority < math.MaxInt16.
  • JobDefaultPriority defaults to 50 and can be configured in the range 50 <= JobDefaultPriority < JobMaxPriority
  • The new configuration value is set into our server configuration.

Notes for the reviewer

This quick draft aims to gather feedback to understand if the solution is on the right path. After that, I'll add tests, docs, proper commits names, and the other missing stuff.

Bear with me, this is my first time contributing to the codebase. 🙏 I'm looking forward to your comments. 🤓

@hashicorp-cla
Copy link

hashicorp-cla commented Feb 7, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alessio-perugini! It looks like you've discovered how gross it is to wire up all the configuration so that it makes its way from the agent to the server, and how it gets merged to the default config. So yes, this looks like it's on the right track!

api/jobs.go Outdated Show resolved Hide resolved
api/jobs.go Outdated Show resolved Hide resolved
nomad/structs/structs.go Show resolved Hide resolved
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good, @alessio-perugini!

I've left some comments to address. In addition, if you can run make cl that'll ask you some questions to generate a changelog file for this PR.

website/content/docs/configuration/server.mdx Outdated Show resolved Hide resolved
nomad/job_endpoint_test.go Outdated Show resolved Hide resolved
command/agent/agent.go Outdated Show resolved Hide resolved
command/agent/agent.go Outdated Show resolved Hide resolved
nomad/structs/structs.go Outdated Show resolved Hide resolved
api/jobs.go Outdated Show resolved Hide resolved
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you picked up the tests, thanks. I think we're getting close on this one

command/agent/job_endpoint.go Outdated Show resolved Hide resolved
website/content/docs/job-specification/job.mdx Outdated Show resolved Hide resolved
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I know reviews for seemingly minor features can end up taking a surprising amount of back and of forth. Thanks for sticking with this, @alessio-perugini!

This will ship in Nomad 1.5.0 GA

@tgross tgross merged commit 365ccf4 into hashicorp:main Feb 17, 2023
@alessio-perugini
Copy link
Contributor Author

@tgross Thank you so much for your patience and help. This was my first time contributing to a project of this scale. I had so much fun working on this. Looking forward to helping you with other issues.
Keep rocking! 🤘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants