abstraction strategy for priority plugin

[cmoussa1]

I've been playing around with improving the priority plugin over the last couple weeks, and it would be helpful for me to outline a strategy to plan how to improve the plugin over a series of pull requests that are concise and easy to review/manage. It would also be nice to step through this outline process with a partner and have another set of eyes on the design process. I'll most likely rely on @jameshcorbett to help me with this. 😄

As I see it now, the plugin suffers from a couple design decisions that I made when the plugin was first written that are the most primed for improvement:

1. The plugin currently just uses a struct bank_info to represent a user/bank object (a user can belong to multiple banks and have different attributes & limits within each bank they belong to). This can be moved to a class that is defined elsewhere and #included in the plugin.
2. The plugin currently uses two maps to manage user/bank information: one whose key-value pair is a userid and then another map data structure that stores the attribute/limit information for every bank that the user belongs to, and a second to keep track of each userid's default bank. This could probably be condensed into just one map data structure, which would be easier to manage.
3. The plugin has a number of helper functions that are specific to searching for/validating/managing flux-accounting information. When the plugin was first written, there were not a lot of features required of the plugin, so defining them within the plugin itself was fine, but as the feature set grew, the plugin grew very long with lots of helper functions that could probably just be abstracted out to an external file, probably just the same one where the proposed flux-accounting class is defined.
4. The plugin does not have unit tests for the helper functions defined, and moving them out to an external file might make testing a lot easier so that we do not have to solely rely on sharness tests to make sure the helper functions work as expected.

I've stepped through the plugin and tried to mark which stuff can be abstracted out:

• any helper functions that deal with searching for user/bank information within the maps that store user/bank information
• any helper functions that create JSON objects of the user/bank information stored in the plugin that is returned in the callback for plugin.query
• any helper functions that deal with creating temporary user/bank objects in the case where the plugin is waiting on flux-accounting data to be loaded

Below is my proposal for the steps of how I was planning on going through this improvement process for the plugin:

• I think a new class should be created called Association since that is what we define as the combination of a user who belongs to a certain bank in the flux-accounting DB. This would replace the use of the bank_info struct throughout the plugin. This can be proposed as one PR. (this has been converted to an issue plugin: create new Association class #411)
• For each major callback in the plugin, calls to any helper functions defined within the plugin itself should be examined to see if they can be abstracted out. Certain logic within each callback should also be analyzed to see if a function can be defined to reduce duplicate code within the plugin (e.g. there is definitely more than one place where user/bank information is searched in the plugin's map in the same way; this can be converted to just a call to a function). For each function, unit tests can be created to ensure they work as intended, and the current testsuite should be run to ensure that they still pass as expected. This should probably be proposed as multiple PRs, perhaps ones that cover 1 to 2 callbacks at a time.

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 accounting.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 association 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 association 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 association->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 work was proposed in plugin: improve callback for job.validate #415

plugin.query

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.

• this work is was proposed in plugin: move helper functions for plugin.query callback #417

job.new

This callback can also be improved by converting the manual lookups of associations to just using external lookup functions.

• this work was proposed in plugin: improve callback for job.new #421

---

The multiple maps that deal with user/bank information (users and users_def_bank) could be condensed into one map. This can be proposed as one PR.

---

Documentation that is specific to flux-accounting information as it relates to the plugin should be added, i.e the structure of the proposed Association class and its attributes and methods. This can be proposed as one or more PRs.

Any feedback or suggestions are more than welcome as I go through this improvement process!! I think multiple issues could be created to track each piece of work that is proposed above.

[jameshcorbett]

The plugin currently uses two maps to manage user/bank information: one whose key-value pair is a userid and then another map data structure that stores the attribute/limit information for every bank that the user belongs to, and a second to keep track of each userid's default bank. This could probably be condensed into just one map data structure, which would be easier to manage.

Would you use the first map like mymap[30013]["pdebug"] which would return something like {"attributes": [], "limits": {"time": 30, "nodecount": 5}? Just checking my understanding.

And then the second map would be like mymap2[30013] which would return "pbatch" or similar?

[jameshcorbett]

Are banks' limits user-specific? That's not my understanding of how things work at LC but maybe that's the most general way to handle it.

Asking because it sounds like the attribute/limit information for each bank is duplicated across users, like the dictionary (in my example above) {"attributes": [], "limits": {"time": 30, "nodecount": 5} would be duplicated for every user who had access to pdebug.

[cmoussa1]

My understanding of limits as it stands is that limits are enforced per-user, i.e multiple users could belong to the same bank, but their limits could be different (one user has a different max number of jobs they can submit than another user), not per-bank. The structure of the association_table (the table that stores the info for all of the users) is structured like this:

            CREATE TABLE IF NOT EXISTS association_table (
                creation_time    bigint(20)                         NOT NULL,
                mod_time         bigint(20)  DEFAULT 0              NOT NULL,
                active           int(11)     DEFAULT 1              NOT NULL,
                username         tinytext                           NOT NULL,
                userid           int(11)     DEFAULT 65534          NOT NULL,
                bank             tinytext                           NOT NULL,
                default_bank     tinytext                           NOT NULL,
                shares           int(11)     DEFAULT 1              NOT NULL    ON CONFLICT REPLACE DEFAULT 1,
                job_usage        real        DEFAULT 0.0            NOT NULL,
                fairshare        real        DEFAULT 0.5            NOT NULL,
                max_running_jobs int(11)     DEFAULT 5              NOT NULL    ON CONFLICT REPLACE DEFAULT 5,
                max_active_jobs  int(11)     DEFAULT 7              NOT NULL    ON CONFLICT REPLACE DEFAULT 7,
                max_nodes        int(11)     DEFAULT 2147483647     NOT NULL    ON CONFLICT REPLACE DEFAULT 2147483647,
                queues           tinytext    DEFAULT ''             NOT NULL    ON CONFLICT REPLACE DEFAULT '',
                projects         tinytext    DEFAULT '*'            NOT NULL    ON CONFLICT REPLACE DEFAULT '*',
                default_project  tinytext    DEFAULT '*'            NOT NULL    ON CONFLICT REPLACE DEFAULT '*',
                PRIMARY KEY   (username, bank)

At least, that has been the structure of the flux-accounting database for a long while, and nobody has corrected me yet. 🤷

[cmoussa1]

To answer your question about the map structure, yes, that is how it is currently structured. mymap2 just stores the name of the default bank for a given userID, whereas mymap is a multi-map data structure that actually has all of the information for every userID's bank. I think this could be restructured to just one map where an additional attribute could be set to designate which bank is the default one.

[jameshcorbett]

Oh that's a good point, I didn't think about structuring it so that a boolean attribute identifies the default bank! That sounds reasonable too. I take back what I said below then.

(I do also think keeping them in separate maps would work too, I think it mostly would come down to how you want to access them. But you know that a lot better than me)

[jameshcorbett]

Overall this sounds great! I'm not immediately sold on this:

[the maps] could probably be condensed into just one map data structure, which would be easier to manage.

I just doesn't strike me as an easy-win type scenario, it seems like the information stored is different. I might change my mind if I knew the code better though!

[cmoussa1]

OK! I see where you are coming from. In this case, let's continue to use a two map structure for the time being.

As for the other areas of improvement, I think I'll go ahead and get started on them. I'll start first with 1. replacing the use of a bank_info struct with a new class Association. Does that sound okay?

[jameshcorbett]

Yeah definitely!

[cmoussa1]

Going to close this discussion as the work to improve the priority plugin with the new external classes and functions has now been pretty much completed.