add-bank/add-user: add check for existing DB structure when trying to add a bank or association

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):

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):

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):

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 \

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,35 +67,34 @@ def test_02_add_duplicate_primary_key(self):
                 acct_conn,
                 username="fluxuser",
                 uid="1234",
-                bank="acct",
+                bank="A",
                 shares="10",
                 queues="",
             )
             u.add_user(
                 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="",
         )
         u.add_user(
             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,24 +137,24 @@ 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()
 
         self.assertEqual(rows[0][0], 0)
 
     # 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

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' \

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