Merge branch 'main' into djperrefort-patch-1

.github/workflows/CodeQL.yml

@@ -24,7 +24,7 @@ jobs:
 
     steps:
     - name: Checkout repository
-      uses: actions/checkout@v3
+      uses: actions/checkout@v4
 
     - name: Initialize CodeQL
       uses: github/codeql-action/init@v2

.github/workflows/DocumentationBuild.yml

@@ -24,7 +24,7 @@ jobs:
         run: /usr/local/bin/entrypoint.sh
 
       - name: Checkout project source
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
 
       - name: Install Poetry
         uses: snok/install-poetry@v1

.github/workflows/PackagePublish.yml

@@ -44,7 +44,7 @@ jobs:
 
     steps:
       - name: Checkout source
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
 
       - name: Set up Python
         uses: actions/setup-python@v4

.github/workflows/PackageTest.yml

@@ -35,7 +35,7 @@ jobs:
         run: /usr/local/bin/entrypoint.sh
 
       - name: Checkout source code
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
 
       - name: Install Poetry
         uses: snok/install-poetry@v1

bank/account_logic.py

@@ -21,6 +21,7 @@
 from .exceptions import *
 from .orm import Account, Allocation, DBConnection, Investment, Proposal
 from .system import EmailTemplate, Slurm, SlurmAccount
+from os import geteuid
 
 Numeric = Union[int, float]
 LOG = getLogger('bank.account_services')
@@ -543,7 +544,6 @@ def add_sus(self, inv_id: Optional[int], sus: int) -> None:
         with DBConnection.session() as session:
             investment = session.execute(query).scalars().first()
             investment.service_units += sus
-            investment.current_sus += sus
 
             session.commit()
 
@@ -573,7 +573,6 @@ def subtract_sus(self, inv_id: Optional[int], sus: int) -> None:
                     f'Cannot subtract {sus}. Investment {inv_id} only has {investment.current_sus} available.')
 
             investment.service_units -= sus
-            investment.current_sus -= sus
 
             session.commit()
 
@@ -722,6 +721,7 @@ def _build_usage_table(self) -> PrettyTable:
                                            f'    {recent_proposal_alloc_status}')
 
             # Proposal End Date as first row
+            output_table.title = f"{self._account_name} Proposal Information"
             output_table.add_row(['Proposal End Date:', proposal.end_date.strftime(settings.date_format), ""],
                                  divider=True)
 
@@ -793,27 +793,33 @@ def _build_usage_table(self) -> PrettyTable:
             usage_percentage = self._calculate_percentage(aggregate_usage_total, allocation_total)
 
             floating_su_percent = self._calculate_percentage(floating_su_usage, floating_su_total)
-            output_table.add_row(['Floating SUs', "SUs Remaining", "% Used"], divider=True)
-            output_table.add_row([f'*Floating SUs', "", ""])
-            output_table.add_row([f'are applied on', "", ""])
-            output_table.add_row([f'any cluster to', str(floating_su_remaining)+'*', floating_su_percent])
-            output_table.add_row([f'cover usage above', "", ""])
-            output_table.add_row([f'Total SUs', "", ""], divider=True)
+            output_table.add_row(["Floating SUs",       "",              ""], divider=True)
+            output_table.add_row(["Floating SUs",       "",              ""])
+            output_table.add_row(["are applied on",     "",              ""])
+            output_table.add_row(["any cluster to",     "",              ""])
+            output_table.add_row(["cover usage above",  "",              ""])
+            output_table.add_row(["Total Proposal SUs", "",              ""])
+            output_table.add_row(["Total",              "SUs Remaining", "% Used"])
+            output_table.add_row([floating_su_total, floating_su_remaining, floating_su_percent], divider=True)
 
             # Add another inner table describing aggregate usage
             if not investments:
                 output_table.add_row(['Aggregate Usage', usage_percentage, ""], divider=True)
             else:
                 investment_total = sum(inv.service_units for inv in investments)
-                investment_percentage = self._calculate_percentage(aggregate_usage_total,
-                                                                   allocation_total + investment_total)
-
-                output_table.add_row(['Investment SUs', "SUs Remaining", "% Used"], divider=True)
-                output_table.add_row([f'**Investment SUs', "",""])
-                output_table.add_row([f'are applied on', "", ""])
-                output_table.add_row([f'any cluster to', str(investment_total)+"**", investment_percentage])
-                output_table.add_row([f'cover usage above',"",""])
-                output_table.add_row([f'Total SUs', "", ""], divider=True)
+                investment_remaining = sum(inv.current_sus for inv in investments)
+                investment_used = investment_total - investment_remaining
+                investment_percentage = self._calculate_percentage(investment_used, investment_total)
+
+                output_table.add_row(["Investment SUs",         "",              ""], divider=True)
+                output_table.add_row(["Investment SUs",         "",              ""])
+                output_table.add_row(["are applied on",         "",              ""])
+                output_table.add_row(["any cluster to",         "",              ""])
+                output_table.add_row(["cover usage above",      "",              ""])
+                output_table.add_row(["Total Proposal SUs",     "",              ""])
+                output_table.add_row(["Total",                  "SUs Remaining", "% Used"])
+                output_table.add_row([investment_total, investment_remaining, investment_percentage], divider=True)
+
                 output_table.add_row(['Aggregate Usage', usage_percentage, ""])
                 output_table.add_row(['(no investments)', "", ""])
 
@@ -830,30 +836,24 @@ def _build_investment_table(self) -> PrettyTable:
             if not investments:
                 raise MissingInvestmentError('Account has no investments')
 
-            table = PrettyTable(header=False, padding_width=5)
-            table.add_row(['Investment ID',
-                           'Total Investment SUs',
-                           'Start Date',
-                           'Current SUs',
-                           'Withdrawn SUs',
-                           'Rollover SUs'])
-
-            for inv in investments:
-                table.add_row([
-                    inv.id,
-                    inv.service_units,
-                    inv.start_date.strftime(settings.date_format),
-                    inv.current_sus,
-                    inv.withdrawn_sus,
-                    inv.withdrawn_sus])
+            table = PrettyTable(header=False, padding_width=5, max_width=80)
+            table.title =f"{self._account_name} Investment Information"
+
+            for inv in investments: 
+                table.add_row(['Investment ID','Start Date','End Date'], divider=True)
+                table.add_row([inv.id, inv.start_date.strftime(settings.date_format), inv.end_date.strftime(settings.date_format)], divider=True)
 
+                table.add_row(['Total Service Units', 'Current SUs', 'Withdrawn SUs'], divider=True)
+                table.add_row([inv.service_units, inv.current_sus, inv.withdrawn_sus], divider=True)
+                table.add_row(['','',''], divider=True)
         return table
 
     def info(self) -> None:
         """Print a summary of service units allocated to and used by the account"""
 
         try:
             print(self._build_usage_table())
+            print("\n")
 
         except MissingProposalError as e:
             print(f'Account {self._account_name} has no active proposal: {str(e)}')
@@ -1024,26 +1024,13 @@ def update_status(self) -> None:
                             continue
 
                         investment_sus_remaining = source.current_sus - total_usage_exceeding_limits
-                        # Investment can not cover, attempt to withdraw remaining SUs in the investment
+                        # Investment can not cover
                         if investment_sus_remaining < 0:
                             total_usage_exceeding_limits -= source.current_sus
+                            source.current_sus = 0
+                            continue
 
-                            # TODO: This is the full amount, should it be less?
-                            #  Could use total divided by 5 to represent 5 year investment,
-                            #  while disbursement available and total_usage_exceeding_limits > 0,
-                            #  determine if usage covered like below.
-                            source.current_sus = source.service_units - source.withdrawn_sus
-                            source.withdrawn_sus = source.service_units
-
-                            investment_sus_remaining = source.current_sus - total_usage_exceeding_limits
-
-                            # Still can not cover after withdrawal
-                            if investment_sus_remaining < 0:
-                                total_usage_exceeding_limits -= source.current_sus
-                                source.current_sus = 0
-                                continue
-
-                        if investment_sus_remaining >= 0:
+                        else:
                             source.current_sus -= total_usage_exceeding_limits
                             lock_clusters = []
                             total_usage_exceeding_limits = 0
@@ -1109,10 +1096,13 @@ def unlock(self, clusters: Optional[Collection[str]] = None, all_clusters=False)
         """Unlock the account on the given clusters
 
         Args:
-            clusters: Name of the clusters to unlock the account on. Defaults to all clusters.
+            clusters: Name of the clusters to unlock the account on. Defaults to all clusters. Must have sudo privileges to execute.
             all_clusters: Lock the user on all clusters
         """
 
+        if geteuid() != 0:
+           exit("ERROR: `unlock` must be run with sudo privileges!")
+
         self._set_account_lock(False, clusters, all_clusters)
 
 

bank/cli/parsers.py

@@ -252,11 +252,11 @@ def _add_cluster_args(parser: ArgumentParser) -> None:
         """
 
         su_argument = dict(metavar='su', type=NonNegativeInt, default=0)
-        parser.add_argument('--all-clusters', **su_argument, help='service units awarded across all clusters')
+        parser.add_argument('--all-clusters', **su_argument, help='service units awarded/added/subtracted from all-clusters allocation, depending on invoked proposal operation')
 
         # Add per-cluster arguments for setting service units
         for cluster in settings.clusters:
-            parser.add_argument(f'--{cluster}', **su_argument, help=f'service units awarded on the {cluster} cluster')
+            parser.add_argument(f'--{cluster}', **su_argument, help=f'service units awarded/added/subtracted on the {cluster} cluster, depending on invoked proposal operation')
 
 
 class InvestmentParser(BaseParser):

bank/system/slurm.py

@@ -128,7 +128,7 @@ def get_locked_state(self, cluster: str) -> bool:
             raise ClusterNotFoundError(f'Cluster {cluster} is not configured with Slurm')
 
         cmd = f'sacctmgr -n -P show assoc account={self.account_name} format=GrpTresRunMins clusters={cluster}'
-        return 'cpu=0' in ShellCmd(cmd).out
+        return 'billing=0' in ShellCmd(cmd).out
 
     def set_locked_state(self, lock_state: bool, cluster: str) -> None:
         """Lock or unlock the current slurm account
@@ -146,10 +146,7 @@ def set_locked_state(self, lock_state: bool, cluster: str) -> None:
             raise ClusterNotFoundError(f'Cluster {cluster} is not configured with Slurm')
 
         lock_state_int = 0 if lock_state else -1
-        ShellCmd(f'sacctmgr -i modify account where account={self.account_name} cluster={cluster} '
-                 f'set GrpTresRunMins=cpu={lock_state_int}').raise_if_err()
-        ShellCmd(f'sacctmgr -i modify account where account={self.account_name} cluster={cluster} '
-                 f'set GrpTresRunMins=gres/gpu={lock_state_int}').raise_if_err()
+        ShellCmd(f'sacctmgr -i modify account where account={self.account_name} cluster={cluster} set GrpTresRunMins=billing={lock_state_int}').raise_if_err()
 
     def get_cluster_usage_per_user(self, cluster: str, start: date, end: date, in_hours: bool = True) -> Dict[str, int]:
         """Return the raw account usage per user on a given cluster

pyproject.toml

@@ -32,12 +32,12 @@ crc-bank = "bank.cli.app:CommandLineApplication.execute"
 [tool.poetry.dependencies]
 beautifulsoup4 = "4.12.2"
 pandas = "2.0.3"
-prettytable = "3.8.0"
+prettytable = "3.9.0"
 python = ">=3.8, <4.0"
 python-environ = "0.4.54"
-SQLAlchemy = "2.0.19"
+SQLAlchemy = "2.0.21"
 sqlalchemy-utils = "0.41.1"
-time-machine = "2.11.0"
+time-machine = "2.13.0"
 
 [tool.poetry.group.tests]
 optional = true
@@ -49,8 +49,8 @@ coverage = "*"
 optional = true
 
 [tool.poetry.group.docs.dependencies]
-sphinx = "<7.0.0"
+sphinx = "<8.0.0"
 sphinx-argparse = "0.4.0"
-sphinx_autodoc_typehints = "1.23.0"
+sphinx_autodoc_typehints = "1.24.0"
 sphinx-copybutton = "0.5.2"
-sphinx-rtd-theme = "1.2.2"
+sphinx-rtd-theme = "1.3.0"

tests/account_logic/test_AccountServices.py

@@ -455,40 +455,6 @@ def test_status_unlocked_with_investment_sus_applied(self) -> None:
 
             self.assertEqual(900, investment.current_sus)
 
-    @patch.object(SlurmAccount,
-                  "get_cluster_usage_per_user",
-                  lambda self, cluster, start, end, in_hours: {'account1': 550, 'account2': 550})
-    def test_status_unlocked_with_multiple_investments_disbursements_applied(self) -> None:
-        """Test that update_status uses investment SUs to cover usage over limits, exhausting the first investment
-        and withdrawing from a future disbursement of the investment"""
-
-        self.slurm_account.set_locked_state(False, cluster=settings.test_cluster)
-
-        with DBConnection.session() as session:
-            # Proposal is expired
-            proposal = session.execute(active_proposal_query).scalars().first()
-            proposal.allocations[0].service_units_total = 10_000
-            proposal.allocations[0].service_units_used = 35_000
-
-            # Investment is expired
-            investment = session.execute(active_investment_query).scalars().first()
-            investment.current_sus = 1000
-            investment.withdrawn_sus = 1000
-            investment.service_units = 2000
-
-            session.commit()
-
-        self.account.update_status()
-
-        # cluster should be unlocked due to exceeding usage being covered by investment
-        self.assertFalse(self.slurm_account.get_locked_state(cluster=settings.test_cluster))
-
-        with DBConnection.session() as session:
-            # check that investment SUs were used to cover usage
-            investment = session.execute(active_investment_query).scalars().first()
-
-            self.assertEqual(900, investment.current_sus)
-            self.assertEqual(investment.withdrawn_sus, investment.service_units)
 
     @patch.object(SlurmAccount,
                   "get_cluster_usage_per_user",