flux-framework/flux-accounting

plugin: create external `bank_info` class

Closed this issue · 1 comments

cmoussa1 commented

We've discussed it a bit in other issues/PRs (I've also opened flux-accounting #403 to begin to tackle this issue), but there is no official tracker for moving some methods/definitions out of the plugin and into its own class.

background

As the requirements/feature set of the priority plugin has grown over time, I've defaulted to just adding all definitions/functions inside of the plugin, which has resulted in the plugin file growing very large and hard to parse. There are sections of repeated code throughout it, and it could use some cleanup.

in-progress work

#403 has been opened to begin to make this transition to use an external class. Here is a summary of what this PR is looking to accomplish:

  • add a definition of the user/bank class (previously known as the bank_info struct in the plugin) and replace all instances of the old struct with the new user_bank_info class
  • add a helper function to this class called get_user_info (), which will perform a lookup of a user/bank in the plugin's internal map of user/banks and either return its location in the map (or NULL on failure)
  • add some basic unit tests for the get_user_info () helper function which get executed with make check
  • adjust and clean up validate_cb (), the callback for job.validate, to make use of this new user_bank_info class

to do

It would be helpful to outline the other locations/methods in the plugin that can be abstracted out so that I can stay organized and gather feedback/suggestions wherever helpful. I'll try to step through the plugin and make notes about what things can be abstracted, starting first with the callbacks:

job.validate

In this callback, a manual lookup of a user/bank is performed using a combination of iterators that access the plugin's internal map. This sort of lookup of a user/bank is repeated numerous times throughout the plugin code.

Instead, I think I can create an external function that lives in bank_info.cpp that does this lookup, and replace the manual lookups in validate_cb () and similar callbacks with calls to this new external function. This new external function can return an address of the user/bank in the plugin's internal map or NULL on failure, and validate_cb () can either continue on with its job validation using this address, or reject the job on failure (because it can't find the user/bank passed in).

The other small improvement that comes with using this new external function is reading the different validation steps in the callback become a little easier to read, i.e bank_it->second.active == 0 becomes user_bank->active == 0 since we don't have to use an iterator here in the callback.

This callback could also use some improvements to its comments and function description.

This set of improvements are proposed in #403.

job.new, job.state.depend, job.state.run

  • use new user_bank_info class
  • convert manual lookup of user/bank (with defined map iterators) and instead just use get_user_info () helper function to look up user/bank

Same thing here. Instead of performing a manual look up of user/bank information in job.new, we can use get_user_info () to determine what to do with the job when it is in this state. I think this function reads a lot cleaner now without the use of iterators.

This work is proposed in a local branch improve.new.depend.run.cbs that can be opened as a PR later.

job.state.priority

  • use new user_bank_info class
  • convert manual lookup of user/bank (with defined map iterators) and instead just use get_user_info () helper function to look up user/bank in the case where jobs submitted before plugin had any flux-accounting information and have to be updated

This callback has a section of code that is responsible for performing a lookup of a user/bank in the case where a job is submitted before the plugin has any accounting information loaded and is being reprioritized. It performs a manual lookup using iterators that follows a pretty similar format to the lookup required in job.validate. So, I can instead use the same external function in bank_info.cpp to do this lookup.

Once a user/bank is found in the plugin's internal map, accessing its attributes becomes a little easier to read. Instead of checking if the user/bank data was actually loaded like this:

if (bank_it->second.max_run_jobs == BANK_INFO_MISSING)

I can check it like this:

if (ub->bank_name == "DNE")

Much cleaner. The rest of the function stays pretty much the exact same.

This function could also use some updated comments/descriptions. It's work is in a local branch improve.priority_cb that I can post as a PR later.

job.update/job.update.attributes.system.*

  • use new user_bank_info class
  • convert manual lookup of user/bank (with defined map iterators) and instead just use get_user_info () helper function to look up user/bank

I can probably also get rid of the get_bank_info () and bank_info_result definitions here since they are very much similar to the aforementioned helper function in the user_bank_info class.

job.state.inactive

  • use new user_bank_info class

Next, I'll take a look at the various helper functions that currently exist in the plugin and see which ones should maybe be abstracted out:

TODO: walk through plugin and look at helper functions

bool check_map_for_dne_only ()

  • move out of mf_priority.cpp

This helper function, designed to iterate through the plugin's internal map of users/banks and check to see if any one of them have a valid default bank, can be moved to the external bank_info file.

This work is added as a couple commits in #403.

query_cb ()

This callback, designed to create a JSON object containing the contents of the plugin's data structure which contains all accounting information, uses a number of helper functions that iterate through this map and construct JSON objects of each user/bank combination. It was mentioned in #403 that these functions could be made a method as part of the new user_bank_info class.

  • look to add method which converts user_bank_info object to a JSON object

There were a number of helper functions defined in the plugin itself that were all used to convert all of the data in the user/bank map to JSON format, and it took up an awful lot of space in the plugin file. Since these functions are designed to deal directly with the map (i.e, the flux-accounting data), it might make more sense to define this externally along with the other functions that deal with user/bank lookups.

The new user_bank_info class has both a new method called .to_json () and a new function map_to_json () that convert this map data to JSON format, which is utilized in the plugin.query callback. Additional data was also added for each user/bank JSON object constructed since there were some fields that were not returned when the callback was first created.

This work is proposed in #405.

cmoussa1 commented

A new discussion #410 has been opened and a more detailed set of goals and objectives for tackling improving the plugin has been created with the help of @jameshcorbett - as a result, I can close this more broadly-defined issue and open more specific ones to handle each task outlined in that discussion.