diff --git a/doc/components/database-administration.rst b/doc/components/database-administration.rst index 897922a9a..6f7879aba 100644 --- a/doc/components/database-administration.rst +++ b/doc/components/database-administration.rst @@ -59,6 +59,13 @@ there is one root bank which all other banks and associations descend from: Multiple levels of banks can be defined under this root bank. Users can belong to more than one bank and will have at most one default bank. +.. warning:: + You are not allowed to have both an association and a bank under the same + parent bank, as this conflicts with how fair-share and job usage are + calculated. Trying to add a sub-bank to a parent bank which already has + associations (or vice versa: trying to add an association to a bank which + already has at least one sub-bank) will result in an error. + To add a bank to the database, you can use the ``flux account add-bank`` command. Each ``add-bank`` call requires a bank name, their allocated shares, and a parent bank name (if it is not the root bank): diff --git a/src/bindings/python/fluxacct/accounting/bank_subcommands.py b/src/bindings/python/fluxacct/accounting/bank_subcommands.py index 3b052491c..6e2d0f7d3 100644 --- a/src/bindings/python/fluxacct/accounting/bank_subcommands.py +++ b/src/bindings/python/fluxacct/accounting/bank_subcommands.py @@ -102,6 +102,15 @@ def add_bank(conn, cur, bank, shares, parent_bank="", priority=0.0): except ValueError as bad_parent_bank: raise ValueError(f"parent bank {bad_parent_bank} not found in bank table") + # check that there exist no associations currently under the parent bank + cur.execute("SELECT * FROM association_table WHERE bank=?", (parent_bank,)) + associations = cur.fetchall() + if len(associations) > 0: + # there is at least one association already under the parent bank; raise an error + raise ValueError( + "banks cannot be added to a bank that currently has associations in it" + ) + # check if bank already exists and is active in bank_table; if so, raise # a sqlite3.IntegrityError if bank_is_active(cur, bank, parent_bank): diff --git a/src/bindings/python/fluxacct/accounting/user_subcommands.py b/src/bindings/python/fluxacct/accounting/user_subcommands.py index 61a45da57..6fb9f4717 100755 --- a/src/bindings/python/fluxacct/accounting/user_subcommands.py +++ b/src/bindings/python/fluxacct/accounting/user_subcommands.py @@ -356,6 +356,16 @@ def add_user( f"association {username},{bank} already active in association_table" ) + # check that there exist no sub-banks under this bank + cur.execute("SELECT * FROM bank_table WHERE parent_bank=?", (bank,)) + parent_bank = cur.fetchall() + if len(parent_bank) > 0: + # the bank that the association is trying to be added to also has at least one + # sub-bank in it; raise an error + raise ValueError( + "associations cannot be added to the same parent bank as a sub-bank" + ) + # if true, association already exists in table but is not # active, so re-activate the association and return if check_if_user_disabled(conn, cur, username, bank): diff --git a/t/Makefile.am b/t/Makefile.am index a9e9b6fc4..a1c2efb41 100644 --- a/t/Makefile.am +++ b/t/Makefile.am @@ -81,6 +81,7 @@ TESTSCRIPTS = \ t1076-cmds-short-options.t \ t1077-max-sched-jobs-basic.t \ t1078-mf-priority-sched-dependencies.t \ + t1079-issue809.t \ t5000-valgrind.t \ python/t1000-example.py \ python/t1001_db.py \ diff --git a/t/python/t1002_user_cmds.py b/t/python/t1002_user_cmds.py index e7bcda22e..6939ebba3 100755 --- a/t/python/t1002_user_cmds.py +++ b/t/python/t1002_user_cmds.py @@ -40,12 +40,14 @@ def setUpClass(self): # add a valid user to association_table def test_01_add_valid_user(self): - b.add_bank(acct_conn, bank="acct", shares=10) + b.add_bank(acct_conn, bank="root", shares=10) + b.add_bank(acct_conn, bank="A", parent_bank="root", shares=10) + b.add_bank(acct_conn, bank="B", parent_bank="root", shares=10) u.add_user( acct_conn, username="fluxuser", uid="1234", - bank="acct", + bank="A", shares="10", queues="", ) @@ -65,7 +67,7 @@ def test_02_add_duplicate_primary_key(self): acct_conn, username="fluxuser", uid="1234", - bank="acct", + bank="A", shares="10", queues="", ) @@ -73,19 +75,18 @@ def test_02_add_duplicate_primary_key(self): acct_conn, username="fluxuser", uid="1234", - bank="acct", + bank="A", shares="10", queues="", ) # add a user with the same username but a different bank def test_03_add_duplicate_user(self): - b.add_bank(acct_conn, bank="other_acct", parent_bank="acct", shares=10) u.add_user( acct_conn, username="dup_user", uid="5678", - bank="acct", + bank="A", shares="10", queues="", ) @@ -93,7 +94,7 @@ def test_03_add_duplicate_user(self): acct_conn, username="dup_user", uid="5678", - bank="other_acct", + bank="B", shares="10", queues="", ) @@ -110,7 +111,7 @@ def test_04_edit_user_value(self): u.edit_user( acct_conn, username="fluxuser", - bank="acct", + bank="A", shares=10000, ) cursor = acct_conn.cursor() @@ -124,7 +125,7 @@ def test_05_edit_reset_user_value(self): u.edit_user( acct_conn, username="fluxuser", - bank="acct", + bank="A", shares="-1", ) cursor = acct_conn.cursor() @@ -136,16 +137,16 @@ def test_05_edit_reset_user_value(self): def test_06_delete_user(self): cursor = acct_conn.cursor() cursor.execute( - "SELECT * FROM association_table WHERE username='fluxuser' AND bank='acct'" + "SELECT * FROM association_table WHERE username='fluxuser' AND bank='A'" ) num_rows_before_delete = cursor.fetchall() self.assertEqual(len(num_rows_before_delete), 1) - u.delete_user(acct_conn, username="fluxuser", bank="acct") + u.delete_user(acct_conn, username="fluxuser", bank="A") cursor.execute( - "SELECT active FROM association_table WHERE username='fluxuser' AND bank='acct'" + "SELECT active FROM association_table WHERE username='fluxuser' AND bank='A'" ) rows = cursor.fetchall() @@ -153,7 +154,7 @@ def test_06_delete_user(self): # check for a new user's default bank def test_07_check_default_bank_new_user(self): - b.add_bank(acct_conn, bank="test_bank", parent_bank="acct", shares=10) + b.add_bank(acct_conn, bank="test_bank", parent_bank="root", shares=10) u.add_user( acct_conn, username="test_user1", @@ -169,7 +170,7 @@ def test_07_check_default_bank_new_user(self): # check for an existing user's default bank def test_08_check_default_bank_existing_user(self): - b.add_bank(acct_conn, bank="other_test_bank", parent_bank="acct", shares=10) + b.add_bank(acct_conn, bank="other_test_bank", parent_bank="root", shares=10) u.add_user( acct_conn, username="test_user1", @@ -204,23 +205,23 @@ def test_10_view_nonexistent_user(self): # disable a user who belongs to multiple banks; make sure that the default_bank # is updated to the next earliest associated bank def test_11_disable_user_default_bank_row(self): - b.add_bank(acct_conn, bank="A", parent_bank="acct", shares=1) - b.add_bank(acct_conn, bank="B", parent_bank="acct", shares=1) - u.add_user(acct_conn, username="test_user2", bank="A") - u.add_user(acct_conn, username="test_user2", bank="B") + b.add_bank(acct_conn, bank="C", parent_bank="root", shares=1) + b.add_bank(acct_conn, bank="D", parent_bank="root", shares=1) + u.add_user(acct_conn, username="test_user2", bank="C") + u.add_user(acct_conn, username="test_user2", bank="D") cur = acct_conn.cursor() cur.execute( "SELECT default_bank FROM association_table WHERE username='test_user2'" ) - self.assertEqual(cur.fetchone()[0], "A") + self.assertEqual(cur.fetchone()[0], "C") - u.delete_user(acct_conn, username="test_user2", bank="A") + u.delete_user(acct_conn, username="test_user2", bank="C") cur.execute( "SELECT default_bank FROM association_table WHERE username='test_user2'" ) - self.assertEqual(cur.fetchone()[0], "B") + self.assertEqual(cur.fetchone()[0], "D") # disable a user who only belongs to one bank; make sure that the default_bank # stays the same after disabling diff --git a/t/t1017-update-db.t b/t/t1017-update-db.t index d3f0f0be8..dcd5758fc 100755 --- a/t/t1017-update-db.t +++ b/t/t1017-update-db.t @@ -125,7 +125,7 @@ for db in ${SHARNESS_TEST_SRCDIR}/expected/test_dbs/*; do test_expect_success 'add a bank: '$(basename $db) \ "flux account add-bank --parent-bank=root G 1" test_expect_success 'add a user: '$(basename $db) \ - "flux account add-user --username=fluxuser --bank=root" + "flux account add-user --username=fluxuser --bank=G" test_expect_success 'check validity of DB: '$(basename $db) \ "flux python ${DB_INTEGRITY_CHECK} $tmp_db > result.out && grep 'ok' result.out" test_expect_success 'shutdown flux-accounting service' \ diff --git a/t/t1079-issue809.t b/t/t1079-issue809.t new file mode 100755 index 000000000..507bce147 --- /dev/null +++ b/t/t1079-issue809.t @@ -0,0 +1,61 @@ +#!/bin/bash + +test_description='make sure an error is raised when creating an invalid bank hierarchy structure' + +. `dirname $0`/sharness.sh + +mkdir -p config + +DB=$(pwd)/FluxAccountingTest.db +QUERYCMD="flux python ${SHARNESS_TEST_SRCDIR}/scripts/query.py" + +export TEST_UNDER_FLUX_SCHED_SIMPLE_MODE="limited=1" +test_under_flux 1 job -o,--config-path=$(pwd)/config + +flux setattr log-stderr-level 1 + +test_expect_success 'allow guest access to testexec' ' + flux config load <<-EOF + [exec.testexec] + allow-guests = true + EOF +' + +test_expect_success 'create flux-accounting DB' ' + flux account -p ${DB} create-db +' + +test_expect_success 'start flux-accounting service' ' + flux account-service -p ${DB} -t +' + +test_expect_success 'add banks' ' + flux account add-bank root 1 && + flux account add-bank --parent-bank=root A 1 +' + +test_expect_success 'add an association' ' + flux account add-user --username=user1 --userid=50001 --bank=A +' + +# Associations can only be added to banks that do not have any sub-banks under +# them. Since the 'root' bank has a sub-bank under it (bank 'A'), this will +# fail. +test_expect_success 'adding an association under the same parent bank as a sub-bank fails' ' + test_must_fail flux account add-user --username=user1 --userid=50001 --bank=root > error_assoc.out 2>&1 && + grep "associations cannot be added to the same parent bank as a sub-bank" error_assoc.out +' + +# Banks (particularly, sub-banks) can only be added under a parent that does +# not already have associations in it. Since bank 'A' has one association in +# it, this will fail. +test_expect_success 'adding a bank under the same parent bank that has associations in it fails' ' + test_must_fail flux account add-bank --parent-bank=A B 1 > error_bank.out 2>&1 && + grep "banks cannot be added to a bank that currently has associations in it" error_bank.out +' + +test_expect_success 'shut down flux-accounting service' ' + flux python -c "import flux; flux.Flux().rpc(\"accounting.shutdown_service\").get()" +' + +test_done