From ca012bc3b69bd63e0bde0822639165c048617ef1 Mon Sep 17 00:00:00 2001 From: Christopher Moussa Date: Wed, 29 Jan 2025 15:57:52 -0800 Subject: [PATCH 1/2] delete-bank: add --force option Problem: The delete-bank command does not actually delete a row from the bank_table but instead sets the 'active' column for a row to 0. There might be a use case where an admin would actually want to remove a row from the table. Add a new --force optional argument to the delete-bank command which will actually remove the row from the bank_table. Add a function description to delete_bank() to describe what the function is doing and add a note about the --force option. --- .../fluxacct/accounting/bank_subcommands.py | 28 +++++++++++++++---- src/cmd/flux-account-service.py | 2 +- src/cmd/flux-account.py | 11 ++++++++ 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/bindings/python/fluxacct/accounting/bank_subcommands.py b/src/bindings/python/fluxacct/accounting/bank_subcommands.py index 914ee13c..86bc8693 100644 --- a/src/bindings/python/fluxacct/accounting/bank_subcommands.py +++ b/src/bindings/python/fluxacct/accounting/bank_subcommands.py @@ -171,11 +171,27 @@ def view_bank(conn, bank, tree=False, users=False, parsable=False, cols=None): raise ValueError(f"view-bank: {exc}") -def delete_bank(conn, bank): +def delete_bank(conn, bank, force=False): + """ + Deactivate a bank row in the bank_table by setting its 'active' status to 0. + If force=True, actually remove the bank row from the bank_table. If the bank contains + multiple sub-banks and associations, either disable or actually remove those rows as + well. + + Args: + conn: The SQLite Connection object + bank: the name of the bank + force: an option to actually remove the row from the bank_table instead of + just setting the 'active' column to 0. + """ cursor = conn.cursor() + if force: + sql_stmt = "DELETE FROM bank_table WHERE bank=?" + else: + sql_stmt = "UPDATE bank_table SET active=0 WHERE bank=?" try: - cursor.execute("UPDATE bank_table SET active=0 WHERE bank=?", (bank,)) + cursor.execute(sql_stmt, (bank,)) # helper function to traverse the bank table and disable all of its sub banks def get_sub_banks(bank): @@ -190,13 +206,13 @@ def get_sub_banks(bank): FROM association_table WHERE bank=? """ for assoc_row in cursor.execute(select_assoc_stmt, (bank,)): - u.delete_user(conn, username=assoc_row[0], bank=assoc_row[1]) + u.delete_user( + conn, username=assoc_row[0], bank=assoc_row[1], force=force + ) # else, disable all of its sub banks and continue traversing else: for row in result: - cursor.execute( - "UPDATE bank_table SET active=0 WHERE bank=?", (row[0],) - ) + cursor.execute(sql_stmt, (row[0],)) get_sub_banks(row[0]) get_sub_banks(bank) diff --git a/src/cmd/flux-account-service.py b/src/cmd/flux-account-service.py index f2a52c4a..0f779351 100755 --- a/src/cmd/flux-account-service.py +++ b/src/cmd/flux-account-service.py @@ -301,7 +301,7 @@ def add_bank(self, handle, watcher, msg, arg): def delete_bank(self, handle, watcher, msg, arg): try: - val = b.delete_bank(self.conn, msg.payload["bank"]) + val = b.delete_bank(self.conn, msg.payload["bank"], msg.payload["force"]) payload = {"delete_bank": val} diff --git a/src/cmd/flux-account.py b/src/cmd/flux-account.py index 75b63e28..0e986545 100755 --- a/src/cmd/flux-account.py +++ b/src/cmd/flux-account.py @@ -387,6 +387,17 @@ def add_delete_bank_arg(subparsers): help="bank name", metavar="BANK", ) + subparser_delete_bank.add_argument( + "--force", + action="store_const", + const=True, + default=False, + help=( + "actually remove bank from bank_table (WARNING: removing a row from " + "the bank_table can affect a bank and its users' fair-share value; " + "proceed with caution)" + ), + ) def add_edit_bank_arg(subparsers): From 39b97552fb7283c47c5195dcdbcc4131529c9fa0 Mon Sep 17 00:00:00 2001 From: Christopher Moussa Date: Wed, 29 Jan 2025 16:01:11 -0800 Subject: [PATCH 2/2] t: add tests for --force option Problem: There exist no tests for calling delete_bank() with the force option set. Add some tests. --- t/python/t1003_bank_cmds.py | 17 +++++++++++++++++ t/t1023-flux-account-banks.t | 18 ++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/t/python/t1003_bank_cmds.py b/t/python/t1003_bank_cmds.py index b38f9e88..8532f483 100755 --- a/t/python/t1003_bank_cmds.py +++ b/t/python/t1003_bank_cmds.py @@ -153,6 +153,23 @@ def test_12_reactivate_bank(self): rows = cur.fetchall() self.assertEqual(rows[0][0], 1) + # actually remove a bank row from the bank_table + def test_13_delete_bank_force(self): + b.delete_bank(acct_conn, bank="G", force=True) + cur.execute("SELECT * FROM bank_table WHERE bank='G'") + rows = cur.fetchall() + self.assertEqual(len(rows), 0) + + # actually delete multiple banks from bank_table by deleting a parent bank + def test_14_delete_parent_bank_force(self): + b.delete_bank(acct_conn, bank="C", force=True) + cur.execute("SELECT * FROM bank_table WHERE bank='C'") + rows = cur.fetchall() + self.assertEqual(len(rows), 0) + cur.execute("SELECT * FROM bank_table WHERE parent_bank='C'") + rows = cur.fetchall() + self.assertEqual(len(rows), 0) + # remove database and log file @classmethod def tearDownClass(self): diff --git a/t/t1023-flux-account-banks.t b/t/t1023-flux-account-banks.t index bd121aa8..c72817fc 100755 --- a/t/t1023-flux-account-banks.t +++ b/t/t1023-flux-account-banks.t @@ -151,6 +151,24 @@ test_expect_success 'combining --tree with --fields does not work' ' grep "tree option does not support custom formatting" error.out ' +test_expect_success 'delete a bank with --force; ensure users also get deleted' ' + flux account delete-bank C --force && + test_must_fail flux account view-bank C > nonexistent_bank.out 2>&1 && + grep "view-bank: bank C not found in bank_table" nonexistent_bank.out && + test_must_fail flux account view-user user5014 > nonexistent_user.out 2>&1 && + grep "view-user: user user5014 not found in association_table" nonexistent_user.out +' + +test_expect_success 'delete a bank with multiple sub-banks and users with --force' ' + flux account delete-bank D --force && + test_must_fail flux account view-bank E > bankE_noexist.out 2>&1 && + grep "view-bank: bank E not found in bank_table" bankE_noexist.out && + test_must_fail flux account view-bank F > bankF_noexist.out 2>&1 && + grep "view-bank: bank F not found in bank_table" bankF_noexist.out && + test_must_fail flux account view-user user5030 > nonexistent_user.out 2>&1 && + grep "view-user: user user5030 not found in association_table" nonexistent_user.out +' + test_expect_success 'remove flux-accounting DB' ' rm $(pwd)/FluxAccountingTest.db '