diff --git a/.github/workflows/CodeQL.yml b/.github/workflows/CodeQL.yml index a9c95c79..db216882 100644 --- a/.github/workflows/CodeQL.yml +++ b/.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 diff --git a/.github/workflows/DocumentationBuild.yml b/.github/workflows/DocumentationBuild.yml index 76023af9..60c03f44 100644 --- a/.github/workflows/DocumentationBuild.yml +++ b/.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 diff --git a/.github/workflows/PackagePublish.yml b/.github/workflows/PackagePublish.yml index 4acfee74..2582c3f7 100644 --- a/.github/workflows/PackagePublish.yml +++ b/.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 diff --git a/.github/workflows/PackageTest.yml b/.github/workflows/PackageTest.yml index cc7840ce..555f85fb 100644 --- a/.github/workflows/PackageTest.yml +++ b/.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 diff --git a/bank/account_logic.py b/bank/account_logic.py index 855fa02e..a845e9fc 100644 --- a/bank/account_logic.py +++ b/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,23 +836,16 @@ 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: @@ -854,6 +853,7 @@ def info(self) -> None: 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) diff --git a/bank/cli/parsers.py b/bank/cli/parsers.py index 9d59d575..6bbdfa61 100644 --- a/bank/cli/parsers.py +++ b/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): diff --git a/bank/system/slurm.py b/bank/system/slurm.py index cbaefed0..e8b56489 100644 --- a/bank/system/slurm.py +++ b/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 diff --git a/pyproject.toml b/pyproject.toml index 1d992f61..ad56effa 100644 --- a/pyproject.toml +++ b/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" diff --git a/tests/account_logic/test_AccountServices.py b/tests/account_logic/test_AccountServices.py index b0b9b02e..b276086a 100644 --- a/tests/account_logic/test_AccountServices.py +++ b/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",