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

flux-jobs: add -i, --include=HOSTS|RANKS option #6209

Merged
merged 6 commits into from
Aug 12, 2024

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Aug 10, 2024

This PR takes a bit of the minor work from #5711 and uses it to implement a -i, --include=HOSTS|RANKS option for flux jobs as suggested in #6202.

This might be a more palatable first step that what's proposed in #5711, is useful on its own, and if the advanced -f, --filter "query syntax" is ever merged, the hostlist/ranks constrain can easily be combined (if used) with and.

The option name of -i, --include could be controversial here. I wouldn't have thought of it except that it was proposed by @kkier, and I like that it matches the similar options in flux-resource(1) for consistency. I also like that it works for both ranks and hostnames.

However, easy to change if others do not agree.

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

Looks great to me. Just one optional suggestion for the man page.

Comment on lines 56 to 59
List only jobs where the assigned resources intersect with the supplied
argument, which may be specified either as an idset of broker ranks or
list of hosts in hostlist form. It is not an error to specify ranks or
hosts which do not exist.
Copy link
Member

Choose a reason for hiding this comment

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

Might want to use "RFC 22 idset" and "RFC 29 hostlist" and add the references to a FLUX RFC section e.g.

FLUX RFC
========

| :doc:`rfc:spec_22`
| :doc:`rfc:spec_29`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion! I've done that and also fixed a typo and force-pushed the result.

Problem: There is no way to pass a constraint to flux.job.job_list()

Add an optional constraint parameter to job_list() and
job_list_inactive()
Problem: There is no way to pass extra constraints via the JobList
interface.

Add an optional constraint parameter to the JobList initializer. This
parameter takes a string in constraint query syntax which is parsed
by an instance of JobListConstraintParser. The result is then joined
with ``and`` to any other constraints before being passed to the
job-list module.
Problem: There is no easy way to restrict the output of flux-jobs(1)
based on a set of hosts or ranks, but this is basic and useful
functionality.

Add a new `-i, --include=` optiont that takes a hostlist or idset of
targets, and adds a constraint to the JobList query that restricts
matching jobs to those that were allocated the provided targets.

Fixes flux-framework#6202
Problem: None of the Python tests ensure the job_list and
job_list_inactive optional constraint parameters work.

Add a couple simple tests to t0013-job-list.py that ensure the
constraint parameters to these functions do something.
Problem: There are no tests of the flux-jobs -i, --include option.

Add some simple tests of this option to t2800-jobs-cmd.t.
Problem: The -i, --include option is not documented in flux-jobs(1).

Document it.
@grondo
Copy link
Contributor Author

grondo commented Aug 12, 2024

Setting MWP here unless I hear objections.

@mergify mergify bot merged commit d7bd7d4 into flux-framework:master Aug 12, 2024
32 of 33 checks passed
@grondo grondo deleted the flux-jobs-include branch August 12, 2024 18:24
Copy link

codecov bot commented Nov 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.31%. Comparing base (7d18dd7) to head (b2868cc).
Report is 467 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6209      +/-   ##
==========================================
- Coverage   83.62%   83.31%   -0.31%     
==========================================
  Files         512      521       +9     
  Lines       83199    85203    +2004     
==========================================
+ Hits        69576    70990    +1414     
- Misses      13623    14213     +590     
Files with missing lines Coverage Δ
src/bindings/python/flux/job/list.py 96.96% <100.00%> (+1.62%) ⬆️
src/cmd/flux-jobs.py 96.49% <100.00%> (+0.19%) ⬆️

... and 192 files with indirect coverage changes

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

Successfully merging this pull request may close these issues.

2 participants