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

command suite: add new list-projects command #496

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

cmoussa1
Copy link
Member

@cmoussa1 cmoussa1 commented Oct 7, 2024

Problem

There currently exists no way to list all of the currently registered projects in the flux-accounting database.


This PR adds a new command to the flux-accounting command suite, list-projects, that creates a table of all of the currently registered projects in the flux-accounting database.

I've also added a couple sharness tests to t1025-flux-account-projects.t that check the output of calling list-projects with just the default project listed and after registering a couple of other projects.

@cmoussa1 cmoussa1 added the new feature new feature label Oct 7, 2024
@cmoussa1 cmoussa1 requested a review from wihobbs October 7, 2024 19:02
Copy link
Member

@wihobbs wihobbs left a comment

Choose a reason for hiding this comment

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

My two comments are my misunderstanding and nothing you did wrong. Go ahead with MWP!

cat <<-EOF >project_table.expected
project_id | project | usage
-----------+---------+------
1 | * | 0.0
Copy link
Member

Choose a reason for hiding this comment

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

Usage is reported in what unit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Usage in accounting is calculated as a product of nnodes and t_elapsed (which is defined as t_inactive - t_run). I have some more detailed explanations and examples here.

@@ -115,3 +115,32 @@ def delete_project(conn, project):
return warning_stmt

return 0


def list_projects(conn):
Copy link
Member

@wihobbs wihobbs Oct 7, 2024

Choose a reason for hiding this comment

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

Ok, sorry for another newbie question but... In my mental map projects were subsidiaries of banks. So does this list only the projects for one bank, or can projects cut across banks altogether?

Copy link
Member Author

Choose a reason for hiding this comment

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

Projects are created and managed separately from banks. My understanding of projects is that they can be thought of as annotations on a job that multiple users (who might belong to different banks) can make. So, this command will list all registered projects that the flux-accounting database knows about, i.e ones that are added via flux account add-project my_project.

Copy link
Member

@wihobbs wihobbs Oct 8, 2024

Choose a reason for hiding this comment

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

Thank you so much @cmoussa1 for all the clarifications. I created more work for you by trying to use these reviews to learn about the interfaces we might use to solve some issues raised by our users at Sandia. I really appreciate the explanations!

Problem: There currently exists no way to list all of the currently
registered projects in the flux-accounting database.

Add a new command to the flux-accounting command suite that creates a
table of all of the currently registered projects in the
flux-accounting database.

Add a couple sharness tests to t1025-flux-account-projects.t that check
the output of calling list-projects with just the default project
listed and after registering a couple of other projects.
@cmoussa1 cmoussa1 force-pushed the add.list-projects.cmd branch from fd2589d to b67446a Compare October 8, 2024 17:08
@cmoussa1
Copy link
Member Author

cmoussa1 commented Oct 8, 2024

Thanks for reviewing @wihobbs! Setting MWP here too

@mergify mergify bot merged commit e7e38fe into flux-framework:master Oct 8, 2024
11 checks passed
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