Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enable bulk getting and unsetting of configuration options #12

Merged
merged 4 commits into from
Jul 10, 2024

Conversation

NucciTheBoss
Copy link
Member

This pull request adds the ability to both bulk get and unset configuration options on slurm config objects. This functionality will be helpful for both if we want to reset the state of Slurm, and quickly get configuration information for various Slurm objects for comparison against new information received by the charms.

For example:

slurmctld = SlurmManagerBase(ServiceType.SLURMCTLD)

# Get the entire Slurm controller configuration
slurmctld.config.get()

# Reset the configuration
slurmctld.config.unset()

Misc.

  • Fixes a bug with .unset(...). If we use snap unset ... instead of snap set ..., we don't need to append the ! exclamation point to the end of the key name. Whoopsies 🥶
  • ConfigurationManager now takes a string in its constructor rather than a ServiceType. This makes it much easier to create configuration managers for slurmctld components such as cgroup and acct_gather. I realized that ConfigurationManager only needs config_name, so there's no need to actually pass the full ServiceType enum.

Additional changes:

- Fixes bug with `unset`. '!' does not need to be appended
  to the end of key names if using `snap unset ...` instead
  of `snap set ...`.

Signed-off-by: Jason C. Nucciarone <[email protected]>
@NucciTheBoss NucciTheBoss added the enhancement New feature or request label Jul 10, 2024
@NucciTheBoss NucciTheBoss requested a review from jedel1043 July 10, 2024 20:59
@NucciTheBoss
Copy link
Member Author

I'll bump the LIBPATCH version after we discuss & approve

Copy link
Contributor

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Agreed with all the changes, it makes it really easy to get/unset all the slurm config at once.
However, we were planning on adding some default config to fix some slurm limitations for the snap (charmed-hpc/slurm-snap#22), and I worry that using unset to clear everything could become a pitfall.

@NucciTheBoss
Copy link
Member Author

Hmm... you make a point. Currently, slurmctld-operator defines the base set of configuration options it needs, so at least in the charm context we can just go back to that base state:

slurmctld = SlurmManagerBase(ServiceType.SLURMCTLD)
slurmctld.config.unset()
slurmctld.set_defaults()

From the snap context, if the entire configuration is blown out we could just add some logic to the configure hook that's like "if snap get -d slurm <option> is empty, revert to these defaults". How does that sound?

@jedel1043
Copy link
Contributor

From the snap context, if the entire configuration is blown out we could just add some logic to the configure hook that's like "if snap get -d slurm <option> is empty, revert to these defaults". How does that sound?

Yep, sounds good!

Signed-off-by: Jason C. Nucciarone <[email protected]>
@NucciTheBoss NucciTheBoss merged commit bacb4d3 into charmed-hpc:main Jul 10, 2024
4 checks passed
@NucciTheBoss NucciTheBoss deleted the refactor-config branch September 17, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants