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

plugin: add project validation/annotation #443

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

cmoussa1
Copy link
Member

@cmoussa1 cmoussa1 commented Apr 12, 2024

Background

The priority plugin does not do any validation or annotation for a specified or default project associated with an association's jobs.


This PR looks to add basic support for validating a project if one is specified when submitting a job as well as adding a default project to jobspec when one is not specified. If a project is specified on the command line, it is validated by looking at the association's list of valid projects. If the specified project cannot be found, the job is rejected.

In the case where no project is specified, the association's default project is added to jobspec via jobspec-update, similar to how the default bank for an association is added to jobspec.

Some basic tests are also added that simulate setting project names in multiple cases - where the plugin is still waiting for flux-accounting data to be loaded, where associations set specific projects, or use and/or update their default project.

@cmoussa1 cmoussa1 added new feature new feature plugin related to the multi-factor priority plugin labels Apr 12, 2024
@cmoussa1 cmoussa1 force-pushed the projects-plugin-annotation branch from 212fd9f to 5e79541 Compare June 12, 2024 16:25
@cmoussa1 cmoussa1 force-pushed the projects-plugin-annotation branch 2 times, most recently from dbccc67 to 52cf8f8 Compare July 8, 2024 19:04
@cmoussa1 cmoussa1 changed the title [WIP] plugin: add project validation/annotation plugin: add project validation/annotation Jul 8, 2024
@cmoussa1 cmoussa1 marked this pull request as ready for review July 8, 2024 19:12
@cmoussa1
Copy link
Member Author

cmoussa1 commented Jul 8, 2024

@ryanday36 if you have some time this week, do you think you could give this PR a high-level look? I want to make sure my understanding of how project support is to work in flux-accounting is as expected, or if you were looking for something else.

@ryanday36
Copy link
Contributor

This looks pretty good. My high level question is, are we going to need to set a default project for every association, or can we just set a global default for all associations that don't have a project?

@cmoussa1
Copy link
Member Author

All associations that do not have a specified project when they are added to the flux-accounting database will have a default project of * assigned to them and their jobs. Does that sound okay?

@ryanday36
Copy link
Contributor

That's perfect. Thanks!

@cmoussa1 cmoussa1 force-pushed the projects-plugin-annotation branch from 52cf8f8 to 9532129 Compare August 14, 2024 16:32
@cmoussa1
Copy link
Member Author

Thanks! I'll mark this as ready for review then

@cmoussa1 cmoussa1 requested a review from grondo August 14, 2024 16:50
Add project validation to the job.validate callback, which checks
that if a project is specified by an association when a job is
submitted, it:

1) is a valid project to submit jobs under, and

2) is a valid project for the association to submit jobs under.

If either one of these checks fail, the job is rejected with a message
including the project name.
Problem: The priority plugin has no way to update jobspec with the
project used for the job in the case where an association submits a job
under a default project.

Add a helper function to the plugin that adds the project name for
an association's job to jobspec via jobspec-update under
attributes.system.project.

Add a jobspec-update of the association's default project name when the
job is in the job.new callback.

In the event where an association submits a job before any information
is loaded to the priority plugin and their job is held in PRIORITY
state, add a jobspec-update of the association's default project name in
job.state.priority if they are submitting under a default project.
@cmoussa1 cmoussa1 force-pushed the projects-plugin-annotation branch from 9532129 to 227f44b Compare September 10, 2024 18:32
Problem: There are no tests for project validation and annotation in the
priority plugin.

Add some tests.
@cmoussa1 cmoussa1 force-pushed the projects-plugin-annotation branch from 227f44b to 779da78 Compare September 10, 2024 18:49
Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@cmoussa1
Copy link
Member Author

Thanks @grondo! Setting MWP here

@mergify mergify bot merged commit 309afff into flux-framework:master Sep 12, 2024
13 checks passed
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

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

Project coverage is 82.39%. Comparing base (1374786) to head (779da78).
Report is 22 commits behind head on master.

Files with missing lines Patch % Lines
src/plugins/mf_priority.cpp 85.71% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #443      +/-   ##
==========================================
+ Coverage   82.34%   82.39%   +0.04%     
==========================================
  Files          20       20              
  Lines        1456     1477      +21     
==========================================
+ Hits         1199     1217      +18     
- Misses        257      260       +3     
Files with missing lines Coverage Δ
src/plugins/mf_priority.cpp 83.20% <85.71%> (+0.14%) ⬆️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-passing new feature new feature plugin related to the multi-factor priority plugin
Projects
Development

Successfully merging this pull request may close these issues.

3 participants