From 43854feda54ac8aa0897ccc18c99b7a013feb473 Mon Sep 17 00:00:00 2001 From: Christopher Moussa Date: Thu, 31 Oct 2024 13:11:07 -0700 Subject: [PATCH 1/8] formatter: add BankFormatter subclass Problem: The AccountingFormatter class has good overarching methods for printing the results of a query to the flux-accounting database, but there is some functionality specific to viewing bank information from the database that could use its own subclass. Add a new subclass called BankFormatter, which contains unique methods for viewing bank/user information in hierarchical and parsable formats with the view-bank command. Remove the helper functions previously defined in bank_subcommands.py in favor of using this new subclass. Add a --fields optional argument to allow the user to customize which columns they want to be returned when looking at a row of information for a bank. --- .../fluxacct/accounting/bank_subcommands.py | 225 +++--------------- .../python/fluxacct/accounting/formatter.py | 180 ++++++++++++++ src/cmd/flux-account-service.py | 1 + src/cmd/flux-account.py | 7 + 4 files changed, 217 insertions(+), 196 deletions(-) diff --git a/src/bindings/python/fluxacct/accounting/bank_subcommands.py b/src/bindings/python/fluxacct/accounting/bank_subcommands.py index 6781867b..fa24299f 100644 --- a/src/bindings/python/fluxacct/accounting/bank_subcommands.py +++ b/src/bindings/python/fluxacct/accounting/bank_subcommands.py @@ -21,63 +21,6 @@ # Helper Functions # # # ############################################################### - - -def print_user_rows(cur, rows, bank): - """Print user information in a table format.""" - user_str = "\nUsers Under Bank {bank_name}:\n\n".format(bank_name=bank) - user_headers = [description[0] for description in cur.description] - # print column names of association_table - for header in user_headers: - user_str += header.ljust(18) - user_str += "\n" - for row in rows: - for col in list(row): - user_str += str(col).ljust(18) - user_str += "\n" - - return user_str - - -def get_bank_rows(cur, rows, bank): - """Print bank information in a table format.""" - bank_str = "" - bank_headers = [description[0] for description in cur.description] - # bank has sub banks, so list them - for header in bank_headers: - bank_str += header.ljust(15) - bank_str += "\n" - for row in rows: - for col in list(row): - bank_str += str(col).ljust(15) - bank_str += "\n" - - return bank_str - - -def print_sub_banks(conn, bank, bank_str, indent=""): - """Traverse the bank table and print all sub banks ansd users.""" - select_stmt = "SELECT bank FROM bank_table WHERE parent_bank=?" - cur = conn.cursor() - cur.execute(select_stmt, (bank,)) - result = cur.fetchall() - - # we've reached a bank with no sub banks - if len(result) == 0: - cur.execute("SELECT username FROM association_table WHERE bank=?", (bank,)) - result = cur.fetchall() - if result: - for row in result: - bank_str += indent + " " + row[0] + "\n" - # else, delete all of its sub banks and continue traversing - else: - for row in result: - bank_str += indent + " " + row[0] + "\n" - bank_str = print_sub_banks(conn, row[0], bank_str, indent + " ") - - return bank_str - - def validate_parent_bank(cur, parent_bank): try: cur.execute("SELECT shares FROM bank_table WHERE bank=?", (parent_bank,)) @@ -134,83 +77,6 @@ def reactivate_bank(conn, cur, bank, parent_bank): conn.commit() -def print_hierarchy(cur, bank, hierarchy_str, indent=""): - # look for all sub banks under this parent bank - select_stmt = "SELECT bank,shares,job_usage FROM bank_table WHERE parent_bank=?" - cur.execute(select_stmt, (bank,)) - sub_banks = cur.fetchall() - - if len(sub_banks) == 0: - # we've reached a bank with no sub banks, so print out every user - # under this bank - cur.execute( - "SELECT username,shares,job_usage,fairshare FROM association_table WHERE bank=?", - (bank,), - ) - users = cur.fetchall() - if users: - for user in users: - hierarchy_str += ( - indent - + " " - + bank.ljust(20) - + str(user[0]).rjust(20 - (len(indent) + 1)) - + str(user[1]).rjust(20) - + str(user[2]).rjust(20) - + str(user[3]).rjust(20) - + "\n" - ) - else: - # continue traversing the hierarchy - for sub_bank in sub_banks: - hierarchy_str += ( - indent - + " " - + str(sub_bank[0]).ljust(20) - + "".rjust(20 - (len(indent) + 1)) # this skips the "Username" column - + str(sub_bank[1]).rjust(20) - + str(sub_bank[2]).rjust(20) - + "\n" - ) - hierarchy_str = print_hierarchy( - cur, sub_bank[0], hierarchy_str, indent + " " - ) - - return hierarchy_str - - -def print_parsable_hierarchy(cur, bank, hierarchy_str, indent=""): - # look for all sub banks under this parent bank - select_stmt = "SELECT bank,shares,job_usage FROM bank_table WHERE parent_bank=?" - cur.execute(select_stmt, (bank,)) - sub_banks = cur.fetchall() - - if len(sub_banks) == 0: - # we've reached a bank with no sub banks, so print out every user - # under this bank - cur.execute( - "SELECT username,shares,job_usage,fairshare FROM association_table WHERE bank=?", - (bank,), - ) - users = cur.fetchall() - if users: - for user in users: - hierarchy_str += ( - f"{indent} {bank}|{user[0]}|{user[1]}|{user[2]}|{user[3]}\n" - ) - else: - # continue traversing the hierarchy - for sub_bank in sub_banks: - hierarchy_str += ( - f"{indent} {str(sub_bank[0])}||{str(sub_bank[1])}|{str(sub_bank[2])}\n" - ) - hierarchy_str = print_parsable_hierarchy( - cur, sub_bank[0], hierarchy_str, indent + " " - ) - - return hierarchy_str - - ############################################################### # # # Subcommand Functions # @@ -270,72 +136,39 @@ def add_bank(conn, bank, shares, parent_bank=""): raise sqlite3.IntegrityError(f"bank {bank} already exists in bank_table") -def view_bank(conn, bank, tree=False, users=False, parsable=False): - cur = conn.cursor() - bank_str = "" +def view_bank(conn, bank, tree=False, users=False, parsable=False, cols=None): + if tree and cols is not None: + # tree format cannot be combined with custom formatting, so raise an Exception + raise ValueError(f"--tree option does not support custom formatting") + if parsable and not tree: + # --parsable can only be called with --tree, so raise an Exception + raise ValueError(f"-P/--parsable can only be passed with -t/--tree") + + # use all column names if none are passed in + cols = cols or fluxacct.accounting.BANK_TABLE + try: - cur.execute("SELECT * FROM bank_table WHERE bank=?", (bank,)) - result = cur.fetchall() - - if result: - bank_str = get_bank_rows(cur, result, bank) - else: - raise ValueError(f"bank {bank} not found in bank_table") - - name = result[0][1] - shares = result[0][4] - usage = result[0][5] - - if parsable is True: - # print out the database hierarchy starting with the bank passed in - hierarchy_str = "Bank|Username|RawShares|RawUsage|Fairshare\n" - hierarchy_str += f"{name}||{str(shares)}|{str(round(usage, 2))}\n" - hierarchy_str = print_parsable_hierarchy(cur, bank, hierarchy_str, "") - return hierarchy_str - if tree is True: - # print out the hierarchy view with the specified bank as the root of the tree - hierarchy_str = ( - "Bank".ljust(20) - + "Username".rjust(20) - + "RawShares".rjust(20) - + "RawUsage".rjust(20) - + "Fairshare".rjust(20) - + "\n" - ) - # add the bank passed in to the hierarchy string - hierarchy_str += ( - name.ljust(20) - + "".rjust(20) - + str(shares).rjust(20) - + str(round(usage, 2)).rjust(20) - + "\n" - ) + cur = conn.cursor() - hierarchy_str = print_hierarchy(cur, name, hierarchy_str, "") - bank_str += "\n" + hierarchy_str - # if users is passed in, print out all potential users under - # the passed in bank - if users is True: - select_stmt = """ - SELECT username,userid,default_bank,shares,job_usage, - fairshare,max_running_jobs,queues FROM association_table - WHERE bank=? - """ - cur.execute( - select_stmt, - (bank,), - ) - result = cur.fetchall() + sql.validate_columns(cols, fluxacct.accounting.BANK_TABLE) + # construct SELECT statement + select_stmt = f"SELECT {', '.join(cols)} FROM bank_table WHERE bank=?" + cur.execute(select_stmt, (bank,)) - if result: - user_str = print_user_rows(cur, result, bank) - bank_str += user_str - else: - bank_str += "\nno users under {bank_name}".format(bank_name=bank) + # initialize AccountingFormatter object + formatter = fmt.BankFormatter(cur) - return bank_str - except sqlite3.OperationalError as exc: - raise sqlite3.OperationalError(f"an sqlite3.OperationalError occurred: {exc}") + if tree: + if parsable: + return formatter.as_parsable_tree(bank) + return formatter.as_tree() + if users: + return formatter.with_users(bank) + return formatter.as_json() + except sqlite3.Error as err: + raise sqlite3.Error(f"view-bank: an sqlite3.Error occurred: {err}") + except ValueError as exc: + raise ValueError(f"view-bank: {exc}") def delete_bank(conn, bank): diff --git a/src/bindings/python/fluxacct/accounting/formatter.py b/src/bindings/python/fluxacct/accounting/formatter.py index 9c8077d4..5d4c708b 100644 --- a/src/bindings/python/fluxacct/accounting/formatter.py +++ b/src/bindings/python/fluxacct/accounting/formatter.py @@ -94,3 +94,183 @@ def as_json(self): json_string = json.dumps(table_data, indent=2) return json_string + + +class BankFormatter(AccountingFormatter): + """ + Subclass of AccountingFormatter that includes additional methods for printing + out banks/sub-banks in a hierarchical format and lists of users under banks. + """ + + def as_tree(self): + """ + Format the flux-accounting bank hierarchy in tree format. The bank passed + into the query will serve as the root of the tree. + + Returns: + hierarchy: the hierarchy of banks in bank_table with the passed-in bank + as the root of the tree. + """ + + def construct_hierarchy(cur, bank, hierarchy, indent=""): + """ + Recursively traverse bank_table and look for sub banks and associations. Add + them to the string representing the hierarchy of banks and users. + + Args: + cur: the SQLite Cursor object used to execute SQL queries. + bank: the current bank being passed to the SQL query. + hierarchy: the string representing the hierarchy of banks and users. + indent: the level of indent for each level of sub bank or users. + Each traversed level will have one additional space (" ") before the + row. + """ + select_stmt = ( + "SELECT bank,shares,job_usage FROM bank_table WHERE parent_bank=?" + ) + cur.execute(select_stmt, (bank,)) + sub_banks = cur.fetchall() + + if len(sub_banks) == 0: + # reached a bank with no sub banks, so get associations under this bank + cur.execute( + "SELECT username,shares,job_usage,fairshare FROM association_table WHERE bank=?", + (bank,), + ) + users = cur.fetchall() + if users: + for user in users: + hierarchy += ( + indent + + " " + + bank.ljust(20) + + str(user[0]).rjust(20 - (len(indent) + 1)) + + str(user[1]).rjust(20) + + str(user[2]).rjust(20) + + str(user[3]).rjust(20) + + "\n" + ) + else: + # continue traversing the hierarchy + for sub_bank in sub_banks: + hierarchy += ( + indent + + " " + + str(sub_bank[0]).ljust(20) + + "".rjust( + 20 - (len(indent) + 1) + ) # this skips the "Username" column + + str(sub_bank[1]).rjust(20) + + str(sub_bank[2]).rjust(20) + + "\n" + ) + hierarchy = construct_hierarchy( + cur, sub_bank[0], hierarchy, indent + " " + ) + + return hierarchy + + # construct header of the hierarchy + hierarchy = ( + "Bank".ljust(20) + + "Username".rjust(20) + + "RawShares".rjust(20) + + "RawUsage".rjust(20) + + "Fairshare".rjust(20) + + "\n" + ) + # add the bank passed in to the hierarchy string + hierarchy += ( + self.rows[0][1].ljust(20) + + "".rjust(20) + + str(self.rows[0][4]).rjust(20) + + str(round(self.rows[0][5], 2)).rjust(20) + + "\n" + ) + + hierarchy = construct_hierarchy(self.cursor, self.rows[0][1], hierarchy, "") + return hierarchy + + def as_parsable_tree(self, bank): + """ + Format the flux-accounting bank hierarchy in a parsable tree format starting with + the bank passed in serving as the root of the tree. Delimit the items in each row + with a pipe ('|') character. + + Returns: + hierarchy: a string representing the hierarchy of banks in the + flux-accounting DB as a parsable tree. + """ + + def construct_parsable_hierarchy(cur, bank, hierarchy, indent=""): + """ + Recursively traverse bank_table and look for sub banks and users and add + them to a string representing the flux-accounting bank hierarchy.. + + Args: + cur: the SQLite Cursor object used to execute SQL queries. + bank: the current bank being passed to the SQL query. + hierarchy: the string holding the parsable hierarchy tree. + indent: the level of indent for each level of sub bank or associations. + Each traversed level will have one additional space " " before the + row. + """ + select_stmt = ( + "SELECT bank,shares,job_usage FROM bank_table WHERE parent_bank=?" + ) + cur.execute(select_stmt, (bank,)) + sub_banks = cur.fetchall() + + if len(sub_banks) == 0: + # reached a bank with no sub banks, so get associations under this bank + cur.execute( + "SELECT username,shares,job_usage,fairshare FROM association_table WHERE bank=?", + (bank,), + ) + users = cur.fetchall() + if users: + for user in users: + hierarchy += ( + f"{indent} {bank}|{user[0]}|{user[1]}|{user[2]}|{user[3]}\n" + ) + else: + # continue traversing the hierarchy + for sub_bank in sub_banks: + hierarchy += f"{indent} {str(sub_bank[0])}||{str(sub_bank[1])}|{str(sub_bank[2])}\n" + hierarchy = construct_parsable_hierarchy( + cur, sub_bank[0], hierarchy, indent + " " + ) + + return hierarchy + + # construct a hierarchy string starting with the bank passed in + hierarchy = "Bank|Username|RawShares|RawUsage|Fairshare\n" + hierarchy += f"{self.rows[0][1]}||{str(self.rows[0][4])}|{str(round(self.rows[0][5], 2))}\n" + hierarchy = construct_parsable_hierarchy(self.cursor, bank, hierarchy, "") + return hierarchy + + def with_users(self, bank): + """ + Print basic information for all of the users under a given bank in table + format. + + Returns: + info: the information for both the bank and basic information for all + users under that bank. + """ + try: + info = self.as_table() + + select_stmt = """SELECT username,default_bank,shares,job_usage,fairshare + FROM association_table + WHERE bank=?""" + self.cursor.execute( + select_stmt, + (bank,), + ) + + formatter = AccountingFormatter(self.cursor) + info += "\n\n" + formatter.as_table() + return info + except ValueError: + return info + f"\n\nno users under {bank}" diff --git a/src/cmd/flux-account-service.py b/src/cmd/flux-account-service.py index 3c9abae5..c6e45822 100755 --- a/src/cmd/flux-account-service.py +++ b/src/cmd/flux-account-service.py @@ -248,6 +248,7 @@ def view_bank(self, handle, watcher, msg, arg): msg.payload["tree"], msg.payload["users"], msg.payload["parsable"], + msg.payload["fields"].split(",") if msg.payload.get("fields") else None, ) payload = {"view_bank": val} diff --git a/src/cmd/flux-account.py b/src/cmd/flux-account.py index 2127b3d6..41cd33f5 100755 --- a/src/cmd/flux-account.py +++ b/src/cmd/flux-account.py @@ -320,6 +320,13 @@ def add_view_bank_arg(subparsers): help="list all sub banks in a parsable format with specified bank as root of tree", metavar="PARSABLE", ) + subparser_view_bank.add_argument( + "--fields", + type=str, + help="list of fields to include in JSON output", + default=None, + metavar="BANK_ID,BANK,ACTIVE,PARENT_BANK,SHARES,JOB_USAGE", + ) def add_delete_bank_arg(subparsers): From b99f598e87b31d45a92f7edf13f29ec73b95626f Mon Sep 17 00:00:00 2001 From: Christopher Moussa Date: Thu, 31 Oct 2024 13:47:14 -0700 Subject: [PATCH 2/8] python: add unit tests for calling view-bank Problem: view-bank does not have many unit tests, especially for calling view-bank with custom outputs or viewing information in either JSON or table format. Add some unit tests. --- t/Makefile.am | 3 +- t/python/t1008_banks_output.py | 320 +++++++++++++++++++++++++++++++++ 2 files changed, 322 insertions(+), 1 deletion(-) create mode 100755 t/python/t1008_banks_output.py diff --git a/t/Makefile.am b/t/Makefile.am index 2a50f316..69f5b749 100644 --- a/t/Makefile.am +++ b/t/Makefile.am @@ -51,7 +51,8 @@ TESTSCRIPTS = \ python/t1004_queue_cmds.py \ python/t1005_project_cmds.py \ python/t1006_job_archive.py \ - python/t1007_formatter.py + python/t1007_formatter.py \ + python/t1008_banks_output.py dist_check_SCRIPTS = \ $(TESTSCRIPTS) \ diff --git a/t/python/t1008_banks_output.py b/t/python/t1008_banks_output.py new file mode 100755 index 00000000..313d463a --- /dev/null +++ b/t/python/t1008_banks_output.py @@ -0,0 +1,320 @@ +#!/usr/bin/env python3 + +############################################################### +# Copyright 2024 Lawrence Livermore National Security, LLC +# (c.f. AUTHORS, NOTICE.LLNS, COPYING) +# +# This file is part of the Flux resource manager framework. +# For details, see https://github.com/flux-framework. +# +# SPDX-License-Identifier: LGPL-3.0 +############################################################### +import unittest +import os +import sqlite3 +import textwrap + +import fluxacct.accounting +from fluxacct.accounting import create_db as c +from fluxacct.accounting import bank_subcommands as b +from fluxacct.accounting import formatter as fmt + + +class TestAccountingCLI(unittest.TestCase): + @classmethod + def setUpClass(self): + # create test accounting database + c.create_db("test_view_banks.db") + global conn + global cur + + conn = sqlite3.connect("test_view_banks.db") + cur = conn.cursor() + + # add some banks, initialize formatter + def test_AccountingFormatter_with_banks(self): + b.add_bank(conn, bank="root", shares=1) + b.add_bank(conn, bank="A", shares=1, parent_bank="root") + + cur.execute("SELECT * FROM bank_table") + formatter = fmt.AccountingFormatter(cur) + + self.assertIsInstance(formatter, fmt.AccountingFormatter) + + def test_default_columns_bank_table(self): + cur.execute("PRAGMA table_info (bank_table)") + columns = cur.fetchall() + bank_table = [column[1] for column in columns] + + self.assertEqual(fluxacct.accounting.BANK_TABLE, bank_table) + + # test JSON output for listing all banks + def test_list_banks_default(self): + expected = textwrap.dedent( + """\ + [ + { + "bank_id": 1, + "bank": "root", + "active": 1, + "parent_bank": "", + "shares": 1, + "job_usage": 0.0 + }, + { + "bank_id": 2, + "bank": "A", + "active": 1, + "parent_bank": "root", + "shares": 1, + "job_usage": 0.0 + } + ] + """ + ) + test = b.list_banks(conn) + self.assertEqual(expected.strip(), test.strip()) + + # test JSON output with custom columns + def test_list_banks_custom_one(self): + expected = textwrap.dedent( + """\ + [ + { + "bank_id": 1 + }, + { + "bank_id": 2 + } + ] + """ + ) + test = b.list_banks(conn, cols=["bank_id"]) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_custom_two(self): + expected = textwrap.dedent( + """\ + [ + { + "bank_id": 1, + "bank": "root" + }, + { + "bank_id": 2, + "bank": "A" + } + ] + """ + ) + test = b.list_banks(conn, cols=["bank_id", "bank"]) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_custom_three(self): + expected = textwrap.dedent( + """\ + [ + { + "bank_id": 1, + "bank": "root", + "active": 1 + }, + { + "bank_id": 2, + "bank": "A", + "active": 1 + } + ] + """ + ) + test = b.list_banks(conn, cols=["bank_id", "bank", "active"]) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_custom_four(self): + expected = textwrap.dedent( + """\ + [ + { + "bank_id": 1, + "bank": "root", + "active": 1, + "parent_bank": "" + }, + { + "bank_id": 2, + "bank": "A", + "active": 1, + "parent_bank": "root" + } + ] + """ + ) + test = b.list_banks(conn, cols=["bank_id", "bank", "active", "parent_bank"]) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_custom_five(self): + expected = textwrap.dedent( + """\ + [ + { + "bank_id": 1, + "bank": "root", + "active": 1, + "parent_bank": "", + "shares": 1 + }, + { + "bank_id": 2, + "bank": "A", + "active": 1, + "parent_bank": "root", + "shares": 1 + } + ] + """ + ) + test = b.list_banks( + conn, cols=["bank_id", "bank", "active", "parent_bank", "shares"] + ) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_custom_six(self): + expected = textwrap.dedent( + """\ + [ + { + "bank_id": 1, + "bank": "root", + "active": 1, + "parent_bank": "", + "shares": 1, + "job_usage": 0.0 + }, + { + "bank_id": 2, + "bank": "A", + "active": 1, + "parent_bank": "root", + "shares": 1, + "job_usage": 0.0 + } + ] + """ + ) + test = b.list_banks( + conn, + cols=["bank_id", "bank", "active", "parent_bank", "shares", "job_usage"], + ) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_table_default(self): + expected = textwrap.dedent( + """\ + bank_id | bank | active | parent_bank | shares | job_usage + --------+------+--------+-------------+--------+---------- + 1 | root | 1 | | 1 | 0.0 + 2 | A | 1 | root | 1 | 0.0 + """ + ) + test = b.list_banks(conn, table=True) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_table_custom_one(self): + expected = textwrap.dedent( + """\ + bank_id + ------- + 1 + 2 + """ + ) + test = b.list_banks(conn, table=True, cols=["bank_id"]) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_table_custom_two(self): + expected = textwrap.dedent( + """\ + bank_id | bank + --------+----- + 1 | root + 2 | A + """ + ) + test = b.list_banks(conn, table=True, cols=["bank_id", "bank"]) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_table_custom_three(self): + expected = textwrap.dedent( + """\ + bank_id | bank | active + --------+------+------- + 1 | root | 1 + 2 | A | 1 + """ + ) + test = b.list_banks(conn, table=True, cols=["bank_id", "bank", "active"]) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_table_custom_four(self): + expected = textwrap.dedent( + """\ + bank_id | bank | active | parent_bank + --------+------+--------+------------ + 1 | root | 1 | + 2 | A | 1 | root + """ + ) + test = b.list_banks( + conn, table=True, cols=["bank_id", "bank", "active", "parent_bank"] + ) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_table_custom_five(self): + expected = textwrap.dedent( + """\ + bank_id | bank | active | parent_bank | shares + --------+------+--------+-------------+------- + 1 | root | 1 | | 1 + 2 | A | 1 | root | 1 + """ + ) + test = b.list_banks( + conn, + table=True, + cols=["bank_id", "bank", "active", "parent_bank", "shares"], + ) + self.assertEqual(expected.strip(), test.strip()) + + def test_list_banks_table_custom_five(self): + expected = textwrap.dedent( + """\ + bank_id | bank | active | parent_bank | shares | job_usage + --------+------+--------+-------------+--------+---------- + 1 | root | 1 | | 1 | 0.0 + 2 | A | 1 | root | 1 | 0.0 + """ + ) + test = b.list_banks( + conn, + table=True, + cols=["bank_id", "bank", "active", "parent_bank", "shares", "job_usage"], + ) + self.assertEqual(expected.strip(), test.strip()) + + # remove database and log file + @classmethod + def tearDownClass(self): + conn.close() + os.remove("test_view_banks.db") + + +def suite(): + suite = unittest.TestSuite() + + return suite + + +if __name__ == "__main__": + from pycotap import TAPTestRunner + + unittest.main(testRunner=TAPTestRunner()) From 3fac10e3338077fdcaed89e0ef6468747931ab90 Mon Sep 17 00:00:00 2001 From: Christopher Moussa Date: Thu, 31 Oct 2024 13:20:50 -0700 Subject: [PATCH 3/8] t: change expected format of "view-bank --users" Problem: The --users option for the view-bank command lists a lot of columns for every user row in the association_table, which crowds the output with potentially unneccessary information when looking at which users belong to the bank being looked at. The format of this option has been changed to: 1) be formatted in tables, and 2) only include a couple of columns per user row Adjust the expected output of the --users optional argument to account for the use of the BankFormatter subclass. --- t/expected/flux_account/A_bank.expected | 15 +++++++-------- t/expected/flux_account/F_bank_users.expected | 5 +++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/t/expected/flux_account/A_bank.expected b/t/expected/flux_account/A_bank.expected index f35c6ffe..2e5e8f22 100644 --- a/t/expected/flux_account/A_bank.expected +++ b/t/expected/flux_account/A_bank.expected @@ -1,9 +1,8 @@ -bank_id bank active parent_bank shares job_usage -2 A 1 root 1 0.0 - -Users Under Bank A: - -username userid default_bank shares job_usage fairshare max_running_jobs queues -user5011 5011 A 1 0.0 0.5 5 -user5012 5012 A 1 0.0 0.5 5 +bank_id | bank | active | parent_bank | shares | job_usage +--------+------+--------+-------------+--------+---------- +2 | A | 1 | root | 1 | 0.0 +username | default_bank | shares | job_usage | fairshare +---------+--------------+--------+-----------+---------- +user5011 | A | 1 | 0.0 | 0.5 +user5012 | A | 1 | 0.0 | 0.5 diff --git a/t/expected/flux_account/F_bank_users.expected b/t/expected/flux_account/F_bank_users.expected index 73ea4109..8b0e2745 100644 --- a/t/expected/flux_account/F_bank_users.expected +++ b/t/expected/flux_account/F_bank_users.expected @@ -1,4 +1,5 @@ -bank_id bank active parent_bank shares job_usage -7 F 1 D 1 0.0 +bank_id | bank | active | parent_bank | shares | job_usage +--------+------+--------+-------------+--------+---------- +7 | F | 1 | D | 1 | 0.0 no users under F From c7983247b99a94fa93877dd6429c427ea5762c27 Mon Sep 17 00:00:00 2001 From: Christopher Moussa Date: Thu, 31 Oct 2024 13:23:00 -0700 Subject: [PATCH 4/8] t: remove single bank info from "view-bank --tree" Problem: The --tree option for the view-bank command still lists the default information for the bank being viewed *before* printing the bank hierarchy, which is not necessary when the user passes the --tree option. Now that the view_bank() function has been reworked to use the new BankFormatter subclass, these options are more distinct from one another, and the --tree option no longer includes this info for the single bank at the top of the output when the --tree option is passed. Adjust the expected output in the sharness tests to account for the removal of the default info for the passed-in bank when calling view-bank with the --tree option. --- t/expected/flux_account/D_bank.expected | 3 --- t/expected/flux_account/E_bank.expected | 3 --- t/expected/flux_account/F_bank_tree.expected | 3 --- t/expected/flux_account/full_hierarchy.expected | 3 --- t/expected/pop_db/db_hierarchy_base.expected | 3 --- t/expected/pop_db/db_hierarchy_new_users.expected | 3 --- t/expected/print_hierarchy/small_no_tie.txt | 3 --- t/expected/print_hierarchy/small_tie.txt | 3 --- t/expected/print_hierarchy/small_tie_all.txt | 3 --- t/expected/update_fshare/post_fshare_update.expected | 3 --- t/expected/update_fshare/pre_fshare_update.expected | 3 --- 11 files changed, 33 deletions(-) diff --git a/t/expected/flux_account/D_bank.expected b/t/expected/flux_account/D_bank.expected index f8356075..553321fd 100644 --- a/t/expected/flux_account/D_bank.expected +++ b/t/expected/flux_account/D_bank.expected @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -5 D 1 root 1 0.0 - Bank Username RawShares RawUsage Fairshare D 1 0.0 E 1 0.0 diff --git a/t/expected/flux_account/E_bank.expected b/t/expected/flux_account/E_bank.expected index e7f29cc9..f041aea4 100644 --- a/t/expected/flux_account/E_bank.expected +++ b/t/expected/flux_account/E_bank.expected @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -6 E 1 D 1 0.0 - Bank Username RawShares RawUsage Fairshare E 1 0.0 E user5030 1 0.0 0.5 diff --git a/t/expected/flux_account/F_bank_tree.expected b/t/expected/flux_account/F_bank_tree.expected index b6cd5052..e408c80a 100644 --- a/t/expected/flux_account/F_bank_tree.expected +++ b/t/expected/flux_account/F_bank_tree.expected @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -7 F 1 D 1 0.0 - Bank Username RawShares RawUsage Fairshare F 1 0.0 diff --git a/t/expected/flux_account/full_hierarchy.expected b/t/expected/flux_account/full_hierarchy.expected index 2c1e5d74..e3d795a9 100644 --- a/t/expected/flux_account/full_hierarchy.expected +++ b/t/expected/flux_account/full_hierarchy.expected @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -1 root 1 1 0.0 - Bank Username RawShares RawUsage Fairshare root 1 0.0 A 1 0.0 diff --git a/t/expected/pop_db/db_hierarchy_base.expected b/t/expected/pop_db/db_hierarchy_base.expected index c3cfb42d..66f082a2 100644 --- a/t/expected/pop_db/db_hierarchy_base.expected +++ b/t/expected/pop_db/db_hierarchy_base.expected @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -1 root 1 1 0.0 - Bank Username RawShares RawUsage Fairshare root 1 0.0 A 1 0.0 diff --git a/t/expected/pop_db/db_hierarchy_new_users.expected b/t/expected/pop_db/db_hierarchy_new_users.expected index 3a4c0e68..9684ad35 100644 --- a/t/expected/pop_db/db_hierarchy_new_users.expected +++ b/t/expected/pop_db/db_hierarchy_new_users.expected @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -1 root 1 1 0.0 - Bank Username RawShares RawUsage Fairshare root 1 0.0 A 1 0.0 diff --git a/t/expected/print_hierarchy/small_no_tie.txt b/t/expected/print_hierarchy/small_no_tie.txt index f7c6165b..ff837006 100644 --- a/t/expected/print_hierarchy/small_no_tie.txt +++ b/t/expected/print_hierarchy/small_no_tie.txt @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -1 root 1 1000 133.0 - Bank Username RawShares RawUsage Fairshare root 1000 133.0 account1 1000 121.0 diff --git a/t/expected/print_hierarchy/small_tie.txt b/t/expected/print_hierarchy/small_tie.txt index 382c3a21..75ac3223 100644 --- a/t/expected/print_hierarchy/small_tie.txt +++ b/t/expected/print_hierarchy/small_tie.txt @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -1 root 1 1000 133.0 - Bank Username RawShares RawUsage Fairshare root 1000 133.0 account1 1000 120.0 diff --git a/t/expected/print_hierarchy/small_tie_all.txt b/t/expected/print_hierarchy/small_tie_all.txt index 5d40fd21..55415a29 100644 --- a/t/expected/print_hierarchy/small_tie_all.txt +++ b/t/expected/print_hierarchy/small_tie_all.txt @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -1 root 1 1000 1332.0 - Bank Username RawShares RawUsage Fairshare root 1000 1332.0 account1 1000 120.0 diff --git a/t/expected/update_fshare/post_fshare_update.expected b/t/expected/update_fshare/post_fshare_update.expected index 51479e10..a9fdb48b 100644 --- a/t/expected/update_fshare/post_fshare_update.expected +++ b/t/expected/update_fshare/post_fshare_update.expected @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -1 root 1 1000 180.0 - Bank Username RawShares RawUsage Fairshare root 1000 180.0 account1 1000 121.0 diff --git a/t/expected/update_fshare/pre_fshare_update.expected b/t/expected/update_fshare/pre_fshare_update.expected index f7c6165b..ff837006 100644 --- a/t/expected/update_fshare/pre_fshare_update.expected +++ b/t/expected/update_fshare/pre_fshare_update.expected @@ -1,6 +1,3 @@ -bank_id bank active parent_bank shares job_usage -1 root 1 1000 133.0 - Bank Username RawShares RawUsage Fairshare root 1000 133.0 account1 1000 121.0 From 4a902d01bfdc186441d412af8f856ece65974202 Mon Sep 17 00:00:00 2001 From: Christopher Moussa Date: Thu, 31 Oct 2024 13:26:53 -0700 Subject: [PATCH 5/8] t: change expected format of "view-bank" Problem: The default output of the view-bank command is now JSON by default, but one of the sharness tests still looks for the old format before the introduction of the BankFormatter subclass. Change the expected output to match the new format of calling view-bank with no optional arguments. --- t/expected/flux_account/root_bank.expected | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/t/expected/flux_account/root_bank.expected b/t/expected/flux_account/root_bank.expected index cb42503f..cb273a2a 100644 --- a/t/expected/flux_account/root_bank.expected +++ b/t/expected/flux_account/root_bank.expected @@ -1,3 +1,10 @@ -bank_id bank active parent_bank shares job_usage -1 root 1 1 0.0 - +[ + { + "bank_id": 1, + "bank": "root", + "active": 1, + "parent_bank": "", + "shares": 1, + "job_usage": 0.0 + } +] From 4ada7369b0740d4c547660aaf7862c8844144d78 Mon Sep 17 00:00:00 2001 From: Christopher Moussa Date: Thu, 31 Oct 2024 13:43:06 -0700 Subject: [PATCH 6/8] t: add --tree to "view-bank -P" in tests Problem: The parsable option for viewing bank hierarchies in the flux-accounting database is meant just to be another option for the --tree option, but now the -P option requires --tree to also be passed when calling view-bank. There are multiple tests in the flux-accounting testsuite that call -P without --tree, and thus, these tests fail. Add --tree to the view-bank -P calls in the testsuite. --- t/t1036-hierarchy-small-no-tie-db.t | 2 +- t/t1037-hierarchy-small-tie-db.t | 2 +- t/t1038-hierarchy-small-tie-all-db.t | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t1036-hierarchy-small-no-tie-db.t b/t/t1036-hierarchy-small-no-tie-db.t index 124a6740..4e0084b4 100755 --- a/t/t1036-hierarchy-small-no-tie-db.t +++ b/t/t1036-hierarchy-small-no-tie-db.t @@ -54,7 +54,7 @@ test_expect_success 'view database hierarchy' ' ' test_expect_success 'view database hierarchy in a parsable format' ' - flux account view-bank -P root > small_no_tie_parsable.test && + flux account view-bank --tree -P root > small_no_tie_parsable.test && test_cmp ${EXPECTED_FILES}/small_no_tie_parsable.txt small_no_tie_parsable.test ' diff --git a/t/t1037-hierarchy-small-tie-db.t b/t/t1037-hierarchy-small-tie-db.t index 0a2afa1b..38c88fb2 100755 --- a/t/t1037-hierarchy-small-tie-db.t +++ b/t/t1037-hierarchy-small-tie-db.t @@ -55,7 +55,7 @@ test_expect_success 'view database hierarchy' ' ' test_expect_success 'view database hierarchy in a parsable format' ' - flux account view-bank -P root > small_tie_parsable.test && + flux account view-bank --tree -P root > small_tie_parsable.test && test_cmp ${EXPECTED_FILES}/small_tie_parsable.txt small_tie_parsable.test ' diff --git a/t/t1038-hierarchy-small-tie-all-db.t b/t/t1038-hierarchy-small-tie-all-db.t index dde9bfc6..c868e11f 100755 --- a/t/t1038-hierarchy-small-tie-all-db.t +++ b/t/t1038-hierarchy-small-tie-all-db.t @@ -58,7 +58,7 @@ test_expect_success 'view database hierarchy' ' ' test_expect_success 'view database hierarchy in a parsable format' ' - flux account view-bank -P root > small_tie_all_parsable.test && + flux account view-bank --tree -P root > small_tie_all_parsable.test && test_cmp ${EXPECTED_FILES}/small_tie_all_parsable.txt small_tie_all_parsable.test ' From 750496642ae5f9d04d8a9df35006a0232100e1c7 Mon Sep 17 00:00:00 2001 From: Christopher Moussa Date: Thu, 31 Oct 2024 13:38:22 -0700 Subject: [PATCH 7/8] t: add tests for combining --tree, --fields Problem: The view-bank command does not support the combination of both --tree and --fields, but there is no check for this in the testsuite. Add a test in t1023-flux-account-banks.t that makes sure the appropriate error message is raised when combining both --tree and --fields. --- t/t1023-flux-account-banks.t | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/t/t1023-flux-account-banks.t b/t/t1023-flux-account-banks.t index 088bd481..eabb6a85 100755 --- a/t/t1023-flux-account-banks.t +++ b/t/t1023-flux-account-banks.t @@ -146,6 +146,11 @@ test_expect_success 'call list-banks with a bad field' ' grep "invalid fields: foo" error.out ' +test_expect_success 'combining --tree with --fields does not work' ' + test_must_fail flux account view-bank root --tree --fields=bank_id > error.out 2>&1 && + grep "tree option does not support custom formatting" error.out +' + test_expect_success 'remove flux-accounting DB' ' rm $(pwd)/FluxAccountingTest.db ' From bd78e1649d36e998b354df3d60414135289d6b49 Mon Sep 17 00:00:00 2001 From: Christopher Moussa Date: Thu, 31 Oct 2024 13:11:07 -0700 Subject: [PATCH 8/8] formatter: add BankFormatter subclass Problem: The AccountingFormatter class has good overarching methods for printing the results of a query to the flux-accounting database, but there is some functionality specific to viewing bank information from the database that could use its own subclass. Add a new subclass called BankFormatter, which contains unique methods for viewing bank/user information in hierarchical and parsable formats with the view-bank command. Remove the helper functions previously defined in bank_subcommands.py in favor of using this new subclass. Add a --fields optional argument to allow the user to customize which columns they want to be returned when looking at a row of information for a bank. --- .../fluxacct/accounting/bank_subcommands.py | 4 ++-- .../python/fluxacct/accounting/formatter.py | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/bindings/python/fluxacct/accounting/bank_subcommands.py b/src/bindings/python/fluxacct/accounting/bank_subcommands.py index fa24299f..914ee13c 100644 --- a/src/bindings/python/fluxacct/accounting/bank_subcommands.py +++ b/src/bindings/python/fluxacct/accounting/bank_subcommands.py @@ -155,8 +155,8 @@ def view_bank(conn, bank, tree=False, users=False, parsable=False, cols=None): select_stmt = f"SELECT {', '.join(cols)} FROM bank_table WHERE bank=?" cur.execute(select_stmt, (bank,)) - # initialize AccountingFormatter object - formatter = fmt.BankFormatter(cur) + # initialize BankFormatter object + formatter = fmt.BankFormatter(cur, bank) if tree: if parsable: diff --git a/src/bindings/python/fluxacct/accounting/formatter.py b/src/bindings/python/fluxacct/accounting/formatter.py index 5d4c708b..a49ac27a 100644 --- a/src/bindings/python/fluxacct/accounting/formatter.py +++ b/src/bindings/python/fluxacct/accounting/formatter.py @@ -13,7 +13,7 @@ class AccountingFormatter: - def __init__(self, cursor): + def __init__(self, cursor, error_msg="no results found in query"): """ Initialize an AccountingFormatter object with a SQLite cursor. @@ -26,7 +26,7 @@ def __init__(self, cursor): if not self.rows: # the SQL query didn't fetch any results; raise an Exception - raise ValueError("no results found in query") + raise ValueError(error_msg) def get_column_names(self): """ @@ -102,6 +102,18 @@ class BankFormatter(AccountingFormatter): out banks/sub-banks in a hierarchical format and lists of users under banks. """ + def __init__(self, cursor, bank_name): + """ + Initialize a BankFormatter object with a SQLite cursor. + Args: + cursor: a SQLite Cursor object that has the results of a SQL query. + bank_name: the name of the bank. + """ + self.bank_name = bank_name + super().__init__( + cursor, error_msg=f"bank {self.bank_name} not found in bank_table" + ) + def as_tree(self): """ Format the flux-accounting bank hierarchy in tree format. The bank passed