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

feat!: overhaul slurmdbd charm API #22

Merged
merged 2 commits into from
Jun 27, 2024
Merged

feat!: overhaul slurmdbd charm API #22

merged 2 commits into from
Jun 27, 2024

Conversation

jamesbeedy
Copy link
Contributor

@jamesbeedy jamesbeedy commented Jun 6, 2024 •
edited by NucciTheBoss
Loading

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

Note: Unit tests have been mangled and are not currently passing, although the charm is working as expected. Need to spend a few minutes going through and fix the broken unit tests.

  1. remove slurm-ops-manager
  2. remove unused code
  3. emit relation data to the charm via events
  4. remove dependency on slurmctld
  5. general code cleanup
  6. consolidate yaml files into charmcraft.yaml
  7. add type tests
  8. rename interfaces

Related:

@jamesbeedy jamesbeedy force-pushed the slurm_config_editor_preparation branch 3 times, most recently from 82854f1 to 1e863d8 Compare June 9, 2024 04:39
@jamesbeedy jamesbeedy changed the title Slurm config editor preparation James Longdev Jun 9, 2024
@jamesbeedy jamesbeedy force-pushed the slurm_config_editor_preparation branch from 1e863d8 to 43b9ca7 Compare June 9, 2024 19:27
@NucciTheBoss NucciTheBoss self-requested a review June 18, 2024 19:31
NucciTheBoss
NucciTheBoss suggested changes Jun 19, 2024
View reviewed changes
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.

Just a few comments. We'll need to fix unit tests before we merge. Seems to just be some mismatch between expected status messages as they've changed with the updated methods.

requirements.txt Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
charmcraft.yaml Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@jamesbeedy jamesbeedy force-pushed the slurm_config_editor_preparation branch from 43b9ca7 to 6a73d4c Compare June 19, 2024 13:20
@jamesbeedy
Copy link
Contributor Author

Thanks, @NucciTheBoss! I've addressed your comments and suggestions and resolved all but the type-check conversation. I'll move these changes forward into the slurmctld-operator next.

Thanks!

NucciTheBoss
NucciTheBoss suggested changes Jun 19, 2024
View reviewed changes
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.

Just a few more suggested changes. After this I think we're good to merge. I'll just want to check against the other operators since the integration tests are relying upon incompatible charms in Charmhub currently

src/constants.py Outdated Show resolved Hide resolved
src/constants.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
@jamesbeedy
Copy link
Contributor Author

It still needs tests fixed and commenting for type ignore comments.

@NucciTheBoss
Copy link
Member

It still needs tests fixed and commenting for type ignore comments.

Okay, this is looking real good now! For the unit tests, seems like we just need to mock the outputs of getpwname and such. Not too difficult! For a future enhancement, we could consider looking at pytest-mocker for mocking objects. Has a pretty nice API, but out of scope for this PR 😄

NucciTheBoss
NucciTheBoss suggested changes Jun 20, 2024
View reviewed changes
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.

Couple comments on the ops manager. I noticed some bugs in the __init__ method that will cause the slurmdbd operator to fail to deploy 😅

src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
src/slurmdbd_ops.py Outdated Show resolved Hide resolved
@jamesbeedy jamesbeedy force-pushed the slurm_config_editor_preparation branch 5 times, most recently from e3d6ddf to cb000c8 Compare June 24, 2024 19:35
@jamesbeedy
project cleanup
cb000c8 
These changes improve the slurmdbd charm in a number of ways.

1) remove slurm-ops-manager
2) remove unused code
3) emit relation data to the charm via events
4) remove dependency on slurmctld
5) general code cleanup
6) consolidate yaml files into charmcraft.yaml
7) add type tests
8) rename interface slurmdbd -> slurmctld
9) update readme

Address feedback from PR review.

* add package managers for each package, slurmdbd and munge
* address docstring issues
* move constants to constants.py
* address test failure
* pin requirements
* install funtions handle failure cases
* upgrade to checkout action v4
NucciTheBoss
NucciTheBoss approved these changes Jun 27, 2024
View reviewed changes
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 with how this pull request has shaped up! I'm ready to merge it 🚢

Only one small nit, but I can commit that to the batch. Just removing .mypy_cache/ from .gitignore since we now using pyright instead.

We can further refine the charm in future pull requests as we stabilize the new API. I'd like to see how we can use a dedicated charm library to reduce code duplication for managing Slurm. Thank you for this work @jamesbeedy 🚀

.gitignore Outdated Show resolved Hide resolved
@NucciTheBoss
refactor: remove mypy cache
269afb5 
Swapped mypy for pyright
@NucciTheBoss NucciTheBoss merged commit a2867ba into charmed-hpc:main Jun 27, 2024
4 of 5 checks passed
@NucciTheBoss NucciTheBoss changed the title James Longdev feat!: overhaul slurmdbd charm API Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NucciTheBoss NucciTheBoss NucciTheBoss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set workload version to version of SLURM service rather than git describe ... > version
2 participants
@jamesbeedy @NucciTheBoss