add-bank: add a check when adding a root bank

src/bindings/python/fluxacct/accounting/bank_subcommands.py

@@ -219,6 +219,12 @@ def print_parsable_hierarchy(cur, bank, hierarchy_str, indent=""):
 def add_bank(conn, bank, shares, parent_bank=""):
     cur = conn.cursor()
 
+    if parent_bank == "":
+        # a root bank is trying to be added; check that one does not already exist
+        cur.execute("SELECT * FROM bank_table WHERE parent_bank=''")
+        if len(cur.fetchall()) > 0:
+            raise ValueError(f"bank_table already has a root bank")
+
     # if the parent bank is not "", that means the bank trying
     # to be added wants to be placed under an existing parent bank
     try:

t/Makefile.am

@@ -41,6 +41,7 @@ TESTSCRIPTS = \
 	t1039-issue476.t \
 	t1040-mf-priority-projects.t \
 	t1041-view-jobs-by-project.t \
+	t1042-issue508.t \
 	t5000-valgrind.t \
 	python/t1000-example.py \
 	python/t1001_db.py \

t/python/t1002_user_cmds.py

@@ -78,7 +78,7 @@ def test_02_add_duplicate_primary_key(self):
 
     # 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", shares=10)
+        b.add_bank(acct_conn, bank="other_acct", parent_bank="acct", shares=10)
         u.add_user(
             acct_conn,
             username="dup_user",
@@ -151,7 +151,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", shares=10)
+        b.add_bank(acct_conn, bank="test_bank", parent_bank="acct", shares=10)
         u.add_user(
             acct_conn,
             username="test_user1",
@@ -167,7 +167,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", shares=10)
+        b.add_bank(acct_conn, bank="other_test_bank", parent_bank="acct", shares=10)
         u.add_user(
             acct_conn,
             username="test_user1",
@@ -202,8 +202,8 @@ 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", shares=1)
-        b.add_bank(acct_conn, bank="B", shares=1)
+        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")
         cur = acct_conn.cursor()

t/python/t1003_bank_cmds.py

@@ -39,10 +39,10 @@ def test_01_add_bank_success(self):
         rows = cur.fetchall()
         self.assertEqual(len(rows), 1)
 
-    # check for an IntegrityError when trying to add a duplicate bank
+    # a root bank already exists, so a ValueError will be raised
     def test_02_add_dup_bank(self):
-        with self.assertRaises(sqlite3.IntegrityError):
-            b.add_bank(acct_conn, bank="root", shares=100)
+        with self.assertRaises(ValueError):
+            b.add_bank(acct_conn, bank="second_root", shares=100)
 
     # trying to add a sub account with an invalid parent bank
     # name should result in a ValueError
@@ -76,19 +76,15 @@ def test_05_disable_bank_success(self):
 
     # disabling a parent bank should disable all of its sub banks
     def test_06_disable_parent_bank(self):
-        b.delete_bank(acct_conn, bank="root")
-        b.delete_bank(acct_conn, bank="sub_account_2")
-
-        b.add_bank(acct_conn, bank="A", shares=1)
-        b.add_bank(acct_conn, bank="B", parent_bank="A", shares=1)
+        b.add_bank(acct_conn, bank="B", parent_bank="root", shares=1)
         b.add_bank(acct_conn, bank="D", parent_bank="B", shares=1)
         b.add_bank(acct_conn, bank="E", parent_bank="B", shares=1)
-        b.add_bank(acct_conn, bank="C", parent_bank="A", shares=1)
+        b.add_bank(acct_conn, bank="C", parent_bank="root", shares=1)
         b.add_bank(acct_conn, bank="F", parent_bank="C", shares=1)
         b.add_bank(acct_conn, bank="G", parent_bank="C", shares=1)
 
-        b.delete_bank(acct_conn, bank="A")
-        cur.execute("SELECT active FROM bank_table WHERE bank='A'")
+        b.delete_bank(acct_conn, bank="root")
+        cur.execute("SELECT active FROM bank_table WHERE bank='root'")
         rows = cur.fetchall()
         self.assertEqual(rows[0][0], 0)
 
@@ -102,7 +98,6 @@ def test_06_disable_parent_bank(self):
 
     # edit a bank value
     def test_07_edit_bank_value(self):
-        b.add_bank(acct_conn, bank="root", shares=100)
         b.edit_bank(acct_conn, bank="root", shares=50)
         cursor = acct_conn.cursor()
         cursor.execute("SELECT shares FROM bank_table where bank='root'")

t/t1042-issue508.t

@@ -0,0 +1,38 @@
+#!/bin/bash
+
+test_description='test trying to add more than one root bank to the bank_table'
+
+. `dirname $0`/sharness.sh
+
+mkdir -p conf.d
+
+ACCOUNTING_DB=$(pwd)/FluxAccountingTest.db
+
+export TEST_UNDER_FLUX_SCHED_SIMPLE_MODE="limited=1"
+test_under_flux 1 job -o,--config-path=$(pwd)/conf.d
+
+flux setattr log-stderr-level 1
+
+test_expect_success 'create flux-accounting DB, start flux-accounting service' '
+	flux account -p ${ACCOUNTING_DB} create-db &&
+	flux account-service -p ${ACCOUNTING_DB} -t
+'
+
+test_expect_success 'add a root bank' '
+	flux account add-bank root 1
+'
+
+test_expect_success 'adding a second root bank will raise a ValueError' '
+	test_must_fail flux account add-bank second_root 1 > error.out 2>&1 &&
+	grep "bank_table already has a root bank" error.out
+'
+
+test_expect_success 'shut down flux-accounting service' '
+	flux python -c "import flux; flux.Flux().rpc(\"accounting.shutdown_service\").get()"
+'
+
+test_expect_success 'remove flux-accounting DB' '
+	rm ${ACCOUNTING_DB}
+'
+
+test_done