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

association_table: add "default_bank" column, change plugin's default bank behavior #130

Merged
merged 5 commits into from
Jul 27, 2021

Conversation

cmoussa1
Copy link
Member

@cmoussa1 cmoussa1 commented Jul 13, 2021

Background

As discussed in #123, the "default" bank support added in #122 currently treats the first bank the user belonged to in the flux-accounting database as the default bank. It cannot be changed once the user gets added to the DB. It would be good to add more robust default bank support in both the flux-accounting database and the priority plugin, where a user's default bank could be changed by a sys admin even after they are initially added.

Implementation

This PR adds a default_bank column to the association_table in the flux-accounting database. When a user/bank combination gets added to the database, a lookup is performed to see if the user already exists in the table and, if they do, what their default bank is. It will set the default_bank to the value of the first bank they are added to; however, this value can now be changed like most other fields in the association_table with the edit-user subcommand.

The default_bank column is now also added to the payload information that is sent to the multi-factor priority plugin. When the plugin extracts this data, it will look at the default_bank key-value pair and compare it to the bank key-value pair in the same payload using strcmp. If the values are the same, it will add the fairshare information to the "default" key in the map data structure in the plugin.

This is similar to the behavior previously present in the plugin, except now, if there is ever an update to a user's default bank in the flux-accounting DB, that change should be reflected in 1) the new payloads sent by bulk_update.py, and 2) the map data structure that caches user-bank-fairshare information.

Fixes #123

@cmoussa1 cmoussa1 added new feature new feature improvement Upgrades to an already existing feature labels Jul 13, 2021
@cmoussa1 cmoussa1 changed the title [WIP] association_table: add "default_bank" column, change plugin's default bank behavior association_table: add "default_bank" column, change plugin's default bank behavior Jul 13, 2021
@cmoussa1 cmoussa1 marked this pull request as ready for review July 13, 2021 23:19
@cmoussa1 cmoussa1 requested a review from SteVwonder July 13, 2021 23:19
cmoussa1 added 5 commits July 26, 2021 08:36
Now that the flux-accounting DB keeps track of a
user's default bank via a column in the association_table,
change the behavior of unpacking this data to look at the
default column. If the bank that the user belongs to matches
the default column, add the user's fairshare data to the "default"
key in the map data structure. Otherwise, create a new entry with
the new bank and fairshare value.

Add a "default_bank" key-value pair to the payload data that gets
sent to the plugin by the bulk_update Python script.
@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #130 (848b4c6) into master (fb4fd16) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #130   +/-   ##
=======================================
  Coverage   85.53%   85.53%           
=======================================
  Files          18       18           
  Lines         892      892           
=======================================
  Hits          763      763           
  Misses        129      129           
Impacted Files Coverage Δ
src/plugins/mf_priority.cpp 94.23% <100.00%> (ø)

@cmoussa1 cmoussa1 requested review from grondo and removed request for SteVwonder July 26, 2021 15:40
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! Nice and simple approach.

I made one comment inline, but after re-reviewing the code I noted that it seems fine as is. I left the comment just to get us thinking about what a real "bulk" update protocol might look like.


if (flux_request_unpack (msg, NULL, "{s:s, s:s, s:s}",
if (flux_request_unpack (msg, NULL, "{s:s, s:s, s:s, s:s}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an idea/question: It might be slightly awkward here that the protocol requires that the default_bank is sent along with every bank update. Unless I've missed something below, if default_bank != bank, then default_bank is unused.

Another idea would be to have default_bank be a boolean, but I'm not sure if this would make the client side more difficult.

Also, I realize you have an issue open to turn this simpler protocol into a "bulk update" protocol, so that you can update multiple users in one RPC. If that is the case, then maybe a cleanup of the RPC design could be done at that time. No reason to fix the current design if it works for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, actually I see now above that this protocol simplifies the current bulk_update.py client, so it is probably fine to leave as is for now.

@cmoussa1
Copy link
Member Author

Thanks for the review @grondo - I should copy your thoughts over to #133 so that I don't forget them, and then I will set MWP.

@mergify mergify bot merged commit 336cc0d into flux-framework:master Jul 27, 2021
@cmoussa1 cmoussa1 deleted the add.default.col branch August 16, 2021 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Upgrades to an already existing feature merge-when-passing new feature new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support "default" bank for users
2 participants