From d46827ab5b3e52354480bd4fb93d46cedff0920f Mon Sep 17 00:00:00 2001 From: cmoussa1 Date: Thu, 29 Aug 2024 09:20:13 -0700 Subject: [PATCH 1/2] cmd: add pop-db as a flux account command Problem: The "pop-db" command is located in a separate file in the src/cmd/ directory and thus requires a different method of calling it than the other flux-accounting commands, i.e you need to write "flux account-pop-db" instead of "flux account pop-db". Move this file to the directory containing the other Python bindings for flux-accounting and add it as a regular command like the other bindings so that you can just call it like "flux account pop-db". Remove the methods and command line argument that deal with establishing a connection to the flux-accounting DB since the systemd service will handle establishing that connection. Adjust the calls in the sharness tests and top-level README for pop-db to account for the change. --- README.md | 2 +- src/Makefile.am | 1 - .../accounting/db_info_subcommands.py | 51 ++++++ src/cmd/flux-account-pop-db.py | 153 ------------------ src/cmd/flux-account-service.py | 19 +++ src/cmd/flux-account.py | 42 ++++- t/t1009-pop-db.t | 6 +- t/t1016-export-db.t | 4 +- 8 files changed, 117 insertions(+), 161 deletions(-) delete mode 100755 src/cmd/flux-account-pop-db.py diff --git a/README.md b/README.md index 095c1a9ca..0f6b8bf95 100644 --- a/README.md +++ b/README.md @@ -202,7 +202,7 @@ creation_time mod_time deleted username bank shares max_jobs max_wall_ ``` Multiple rows of data can be loaded to the database at once using `.csv` files -and the `flux account-pop-db` command. Run `flux account-pop-db --help` for +and the `flux account pop-db` command. Run `flux account pop-db --help` for `.csv` formatting instructions. User and bank information can also be exported from the database using the diff --git a/src/Makefile.am b/src/Makefile.am index b969c1e53..4863481e3 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -124,7 +124,6 @@ dist_fluxcmd_SCRIPTS = \ cmd/flux-account.py \ cmd/flux-account-update-fshare \ cmd/flux-account-priority-update.py \ - cmd/flux-account-pop-db.py \ cmd/flux-account-update-db.py \ cmd/flux-account-service.py \ cmd/flux-account-fetch-job-records.py diff --git a/src/bindings/python/fluxacct/accounting/db_info_subcommands.py b/src/bindings/python/fluxacct/accounting/db_info_subcommands.py index c9882d8b8..1ebe7329f 100644 --- a/src/bindings/python/fluxacct/accounting/db_info_subcommands.py +++ b/src/bindings/python/fluxacct/accounting/db_info_subcommands.py @@ -11,6 +11,9 @@ ############################################################### import csv +from fluxacct.accounting import bank_subcommands as b +from fluxacct.accounting import user_subcommands as u + def export_db_info(conn, users=None, banks=None): try: @@ -46,3 +49,51 @@ def export_db_info(conn, users=None, banks=None): writer.writerow(row) except IOError as err: print(err) + + +def populate_db(conn, users=None, banks=None): + if banks is not None: + try: + with open(banks) as csv_file: + csv_reader = csv.reader(csv_file, delimiter=",") + + for row in csv_reader: + b.add_bank( + conn, + bank=row[0], + parent_bank=row[1], + shares=row[2], + ) + except IOError as err: + print(err) + + if users is not None: + try: + with open(users) as csv_file: + csv_reader = csv.reader(csv_file, delimiter=",") + + # assign default values to fields if + # their slot is empty in the csv file + for row in csv_reader: + username = row[0] + uid = row[1] + bank = row[2] + shares = row[3] if row[3] != "" else 1 + max_running_jobs = row[4] if row[4] != "" else 5 + max_active_jobs = row[5] if row[5] != "" else 7 + max_nodes = row[6] if row[6] != "" else 2147483647 + queues = row[7] + + u.add_user( + conn, + username, + bank, + uid, + shares, + max_running_jobs, + max_active_jobs, + max_nodes, + queues, + ) + except IOError as err: + print(err) diff --git a/src/cmd/flux-account-pop-db.py b/src/cmd/flux-account-pop-db.py deleted file mode 100755 index c0d7e4950..000000000 --- a/src/cmd/flux-account-pop-db.py +++ /dev/null @@ -1,153 +0,0 @@ -#!/usr/bin/env python3 - -############################################################### -# Copyright 2020 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 argparse -import csv -import os -import sqlite3 -import sys - -from argparse import RawDescriptionHelpFormatter - -import fluxacct.accounting -from fluxacct.accounting import bank_subcommands as b -from fluxacct.accounting import user_subcommands as u - - -def set_db_loc(args): - path = args.path if args.path else fluxacct.accounting.db_path - - return path - - -def est_sqlite_conn(path): - # try to open database file; will exit with -1 if database file not found - if not os.path.isfile(path): - print(f"Database file does not exist: {path}", file=sys.stderr) - sys.exit(1) - - db_uri = "file:" + path + "?mode=rw" - try: - conn = sqlite3.connect(db_uri, uri=True) - # set foreign keys constraint - conn.execute("PRAGMA foreign_keys = 1") - except sqlite3.OperationalError as exc: - print(f"Unable to open database file: {db_uri}", file=sys.stderr) - print(f"Exception: {exc}") - sys.exit(1) - - # check version of database; if not up to date, output message - # and exit - cur = conn.cursor() - cur.execute("PRAGMA user_version") - db_version = cur.fetchone()[0] - if db_version < fluxacct.accounting.db_schema_version: - print( - "flux-accounting database out of date; please update DB with 'flux account-update-db' before running commands" - ) - sys.exit(1) - - return conn - - -def populate_db(path, users=None, banks=None): - conn = est_sqlite_conn(path) - cur = conn.cursor() - - if banks is not None: - try: - with open(banks) as csv_file: - csv_reader = csv.reader(csv_file, delimiter=",") - - for row in csv_reader: - b.add_bank( - conn, - bank=row[0], - parent_bank=row[1], - shares=row[2], - ) - except IOError as err: - print(err) - - if users is not None: - try: - with open(users) as csv_file: - csv_reader = csv.reader(csv_file, delimiter=",") - - # assign default values to fields if - # their slot is empty in the csv file - for row in csv_reader: - username = row[0] - uid = row[1] - bank = row[2] - shares = row[3] if row[3] != "" else 1 - max_running_jobs = row[4] if row[4] != "" else 5 - max_active_jobs = row[5] if row[5] != "" else 7 - max_nodes = row[6] if row[6] != "" else 5 - queues = row[7] - - u.add_user( - conn, - username, - bank, - uid, - shares, - max_running_jobs, - max_active_jobs, - max_nodes, - queues, - ) - except IOError as err: - print(err) - - -def main(): - parser = argparse.ArgumentParser( - description=""" - Description: Populate a flux-accounting database with a .csv file. - - Order of elements required for populating association_table: - - Username,UserID,Bank,Shares,MaxRunningJobs,MaxActiveJobs,MaxNodes,Queues - - [Shares], [MaxRunningJobs], [MaxActiveJobs], and [MaxNodes] can be left - blank ('') in the .csv file for a given row. - - ---------------- - - Order of elements required for populating bank_table: - - Bank,ParentBank,Shares - - [ParentBank] can be left blank ('') in .csv file for a given row. - """, - formatter_class=RawDescriptionHelpFormatter, - ) - - parser.add_argument( - "-p", "--path", dest="path", help="specify location of database file" - ) - parser.add_argument( - "-u", "--users", help="path to a .csv file containing user information" - ) - parser.add_argument( - "-b", "--banks", help="path to a .csv file containing bank information" - ) - - args = parser.parse_args() - - path = set_db_loc(args) - - populate_db(path, args.users, args.banks) - - -if __name__ == "__main__": - main() diff --git a/src/cmd/flux-account-service.py b/src/cmd/flux-account-service.py index fef4c5dfd..93be3f91f 100755 --- a/src/cmd/flux-account-service.py +++ b/src/cmd/flux-account-service.py @@ -109,6 +109,7 @@ def __init__(self, flux_handle, conn): "delete_project", "scrub_old_jobs", "export_db", + "pop_db", "shutdown_service", ] @@ -543,6 +544,24 @@ def export_db(self, handle, watcher, msg, arg): msg, 0, f"a non-OSError exception was caught: {str(exc)}" ) + def pop_db(self, handle, watcher, msg, arg): + try: + val = d.populate_db( + self.conn, + msg.payload["users"], + msg.payload["banks"], + ) + + payload = {"pop_db": val} + + handle.respond(msg, payload) + except KeyError as exc: + handle.respond_error(msg, 0, f"missing key in payload: {exc}") + except Exception as exc: + handle.respond_error( + msg, 0, f"a non-OSError exception was caught: {str(exc)}" + ) + LOGGER = logging.getLogger("flux-uri") diff --git a/src/cmd/flux-account.py b/src/cmd/flux-account.py index e2520617f..a421b485f 100755 --- a/src/cmd/flux-account.py +++ b/src/cmd/flux-account.py @@ -556,7 +556,7 @@ def add_export_db_arg(subparsers): Use these two files to populate a new flux-accounting DB with: - flux account-pop-db -p path/to/db -b banks.csv -u users.csv + flux account pop-db -b banks.csv -u users.csv """, formatter_class=flux.util.help_formatter(), ) @@ -569,6 +569,38 @@ def add_export_db_arg(subparsers): ) +def add_pop_db_arg(subparsers): + subparser = subparsers.add_parser( + "pop-db", + help=""" + Description: Populate a flux-accounting database with a .csv file. + + Order of elements required for populating association_table: + + Username,UserID,Bank,Shares,MaxRunningJobs,MaxActiveJobs,MaxNodes,Queues + + [Shares], [MaxRunningJobs], [MaxActiveJobs], and [MaxNodes] can be left + blank ('') in the .csv file for a given row. + + ---------------- + + Order of elements required for populating bank_table: + + Bank,ParentBank,Shares + + [ParentBank] can be left blank ('') in .csv file for a given row. + """, + formatter_class=flux.util.help_formatter(), + ) + subparser.set_defaults(func="pop_db") + subparser.add_argument( + "-u", "--users", help="path to a .csv file containing user information" + ) + subparser.add_argument( + "-b", "--banks", help="path to a .csv file containing bank information" + ) + + def add_arguments_to_parser(parser, subparsers): add_path_arg(parser) add_output_file_arg(parser) @@ -593,6 +625,7 @@ def add_arguments_to_parser(parser, subparsers): add_delete_project_arg(subparsers) add_scrub_job_records_arg(subparsers) add_export_db_arg(subparsers) + add_pop_db_arg(subparsers) def set_db_location(args): @@ -779,6 +812,13 @@ def select_accounting_function(args, output_file, parser): "banks": args.banks, } return_val = flux.Flux().rpc("accounting.export_db", data).get() + elif args.func == "pop_db": + data = { + "path": args.path, + "users": args.users, + "banks": args.banks, + } + return_val = flux.Flux().rpc("accounting.pop_db", data).get() else: print(parser.print_usage()) return diff --git a/t/t1009-pop-db.t b/t/t1009-pop-db.t index e3cc8e529..f80b5bd6a 100755 --- a/t/t1009-pop-db.t +++ b/t/t1009-pop-db.t @@ -31,7 +31,7 @@ test_expect_success 'create a banks.csv file containing bank information' ' ' test_expect_success 'populate flux-accounting DB with banks.csv' ' - flux account-pop-db -p ${DB_PATH} -b banks.csv + flux account pop-db -b banks.csv ' test_expect_success 'create a users.csv file containing user information' ' @@ -45,7 +45,7 @@ test_expect_success 'create a users.csv file containing user information' ' ' test_expect_success 'populate flux-accounting DB with users.csv' ' - flux account-pop-db -p ${DB_PATH} -u users.csv + flux account pop-db -u users.csv ' test_expect_success 'check database hierarchy to make sure all banks & users were added' ' @@ -64,7 +64,7 @@ test_expect_success 'create a users.csv file with some missing optional user inf ' test_expect_success 'populate flux-accounting DB with users_optional_vals.csv' ' - flux account-pop-db -p ${DB_PATH} -u users_optional_vals.csv + flux account pop-db -u users_optional_vals.csv ' test_expect_success 'check database hierarchy to make sure new users were added' ' diff --git a/t/t1016-export-db.t b/t/t1016-export-db.t index 482f0a7e2..41d1d7606 100755 --- a/t/t1016-export-db.t +++ b/t/t1016-export-db.t @@ -82,8 +82,8 @@ test_expect_success 'restart service against new DB' ' ' test_expect_success 'import information into new DB' ' - flux account-pop-db -p ${DB_PATHv2} -b banks.csv && - flux account-pop-db -p ${DB_PATHv2} -u users.csv + flux account pop-db -b banks.csv && + flux account pop-db -u users.csv ' test_expect_success 'create hierarchy output of the new DB and store it in a file' ' From bd09f72d1602a9ac24e7304f7171b640b9d21783 Mon Sep 17 00:00:00 2001 From: cmoussa1 Date: Thu, 29 Aug 2024 09:25:29 -0700 Subject: [PATCH 2/2] t1026: add export-db, pop-db commands to test Problem: t1026-flux-account-perms.t goes through all of the flux account commands that require admin privileges to run and makes sure that they cannot be accessed by regular users, but it does not test the "export-db" or "pop-db" commands. Add both of these commands to the test suite to ensure they require admin privileges to be run. --- t/t1026-flux-account-perms.t | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/t/t1026-flux-account-perms.t b/t/t1026-flux-account-perms.t index 2ff0082cb..e67daaeb4 100755 --- a/t/t1026-flux-account-perms.t +++ b/t/t1026-flux-account-perms.t @@ -137,6 +137,26 @@ test_expect_success 'scrub-old-jobs should not be accessible by all users' ' ) ' +test_expect_success 'export-db should not be accessible by all users' ' + newid=$(($(id -u)+1)) && + ( export FLUX_HANDLE_ROLEMASK=0x2 && + export FLUX_HANDLE_USERID=$newid && + test_must_fail flux account export-db > no_access_export_db.out 2>&1 && + grep "Request requires owner credentials" no_access_export_db.out + ) +' + +test_expect_success 'pop-db should not be accessible by all users' ' + newid=$(($(id -u)+1)) && + ( export FLUX_HANDLE_ROLEMASK=0x2 && + export FLUX_HANDLE_USERID=$newid && + touch users.csv && + touch banks.csv && + test_must_fail flux account pop-db -u users.csv -b banks.csv > no_access_pop_db.out 2>&1 && + grep "Request requires owner credentials" no_access_pop_db.out + ) +' + test_expect_success 'remove flux-accounting DB' ' rm $(pwd)/FluxAccountingTest.db '