Skip to content
This repository has been archived by the owner on Aug 9, 2024. It is now read-only.

feat!: overhaul slurmctld charm API #26

Merged
merged 2 commits into from
Jun 27, 2024
Merged

feat!: overhaul slurmctld charm API #26

merged 2 commits into from
Jun 27, 2024

Conversation

jamesbeedy
Copy link
Contributor

@jamesbeedy jamesbeedy commented Jun 5, 2024

Please reference the Slurm Charms Change Summary Document for an in-depth explanation of these changes.

These changes make several modifications to the slurmctld charm.

  • Removes no longer used interfaces
  • remove slurmctld dependency on slurmdbd and slurmd
  • support partitions with 0 nodes
  • refactor relation data
  • recreate how configs are written
  • support user supplied partition configuration
  • begin modeling slurm.conf
  • add type checking
  • remove unused code
  • support partition events and slurmd node events
  • remove unused upgrade-charm hook
  • use systemd and apt charm libs
  • update charmcraft.yaml

@jamesbeedy jamesbeedy requested a review from NucciTheBoss June 5, 2024 17:37
@jamesbeedy jamesbeedy changed the title james longdev james Longdev Jun 9, 2024
@jamesbeedy jamesbeedy changed the title james Longdev James Longdev Jun 9, 2024
Copy link
Member

@NucciTheBoss NucciTheBoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good. Just a couple of questions and places where I want to know your thoughts. Let me know if you have any questions!

README.md Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
src/interface_slurmd.py Outdated Show resolved Hide resolved
src/slurmctld_ops.py Outdated Show resolved Hide resolved
src/slurmctld_ops.py Outdated Show resolved Hide resolved
charmcraft.yaml Outdated Show resolved Hide resolved
src/interface_slurmd.py Outdated Show resolved Hide resolved
partition_as_dict = json.loads(partition_as_json)
except json.JSONDecodeError as e:
logger.error(e)
raise (e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise (e)
raise e

src/interface_slurmd.py Outdated Show resolved Hide resolved
src/slurm_conf_editor.py Show resolved Hide resolved
These changes make a number of modifications to the slurm charms.

* Removes no longer used interfaces
* remove slurmctld dependency on slurmdbd and slurmd
* support partitions with 0 nodes
* refactor relation data
* recreate how configs are written
* support user supplied partition configuration
* start modeling config
* add type checking
* remove unused code
* support partition events and slurmd node events
* remove upgrade-charm hook as it did nothing
* use systemd and apt charm libs
* rename interfaces
* update readme to reflect new interface names and minimal deployment using only slurmctld and slurmd
* address pr feedback
Copy link
Member

@NucciTheBoss NucciTheBoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to merge this pull request too. Thank you for this work @jamesbeedy 🎉

Similar to slurmdbd-operator, just going to drop .mypy_cache from .gitignore since we're now using pyright. I think this pull request is in good shape for landing the API overhaul. We can open subsequent pull requests to focus and further refine the features we need 😄

.gitignore Outdated Show resolved Hide resolved
Using pyright now instead of mypy
@NucciTheBoss NucciTheBoss merged commit 205b984 into charmed-hpc:main Jun 27, 2024
4 of 5 checks passed
@NucciTheBoss NucciTheBoss changed the title James Longdev feat!: overhaul slurmctld charm API Jun 27, 2024
@jamesbeedy jamesbeedy deleted the slurm_config_editor_preparation branch July 1, 2024 14:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants