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

Adjust the UX of the storage class parameters feature #21

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

addyess
Copy link
Member

@addyess addyess commented Jul 17, 2024

Update

Overview

Addresses LP#2073297 by allowing the ceph-csi charm to provision storage-class parameters of each of the Storage Classes created by this charm.

Details

  1. Creates a delete-storage-class action

  2. Updates three new config options:

  • cephfs-storage-class-parameters
  • ceph-xfs-storage-class-parameters
  • ceph-ext4-storage-class-parameters

Each of these config options allows one to adjust the parameters of the storage class patched in by the charm.

Storage-Class parameters like these are not run-time updatable yet, but this can be set at charm deploy-time to correctly match any settings on any storage classes which exist in the cluster.

For example)

You may deploy the charm like this:

juju deploy ceph-csi --channel=1.30/stable \
    --config ceph-xfs-storage-class-parameters='removed-key- custom-value=true'

This ensures that the best defaults possible are baked into the charm config
The charm managed parameters are removable and can be updated (while not recommended), but not listed in the charm config since the charm config doesn't support formatting the configured parameters.

@addyess addyess force-pushed the bug/lp2073297/rbd-sc-parameters branch 2 times, most recently from ac31374 to fa415cb Compare July 17, 2024 17:59
@addyess addyess force-pushed the bug/lp2073297/rbd-sc-parameters branch from fa415cb to 7922bb3 Compare July 17, 2024 19:29
Comment on lines +50 to +56
name:
type: string
description: Name of a specific storage class to delete.
enum:
- cephfs
- ceph-xfs
- cephfs-ext4
Copy link
Member Author

Choose a reason for hiding this comment

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

i love that juju supports json-schema for action properties!!

Copy link
Member Author

Choose a reason for hiding this comment

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

look how cool:

⚡ juju run ceph-csi/leader delete-storage-class name=thing

Operation 1 failed to schedule any tasks:
validation failed: (root).name : must match one of the enum values ["cephfs","ceph-xfs","cephfs-ext4"], given "thing"

Copy link
Member

Choose a reason for hiding this comment

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

🤩 Amazing stuff!

"pool": f"{self.fs_type}-pool",
"thickProvision": "false",
Copy link
Member Author

Choose a reason for hiding this comment

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

this has been deprecated since v3.6.0. It has no business being there.

Copy link
Member Author

Choose a reason for hiding this comment

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

this may result in situations where the charm fails with an error about not being able to patch the storage class. So we've added this handy action to delete the storage class:

juju run ceph-csi/leader delete-storage-class name=ceph-xfs
juju run ceph-csi/leader sync-resources

Copy link
Member Author

@addyess addyess Jul 17, 2024

Choose a reason for hiding this comment

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

if it can't delete the storage class, we're in a real situation here where either the config needs to change to match the installed storage class or the cluster admin needs to check if there are PVCs using that storage class which are incorrectly provisioned

@@ -80,9 +80,7 @@ def __call__(self) -> Optional[AnyResource]:
"csi.storage.k8s.io/node-stage-secret-namespace": ns,
"csi.storage.k8s.io/provisioner-secret-name": StorageSecret.SECRET_NAME,
"csi.storage.k8s.io/provisioner-secret-namespace": ns,
"imageFeatures": "layering",
Copy link
Member Author

Choose a reason for hiding this comment

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

don't worry, this is moved to the config.yaml

Copy link
Member

@mateoflorido mateoflorido left a comment

Choose a reason for hiding this comment

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

Great work, thanks for implementing the action, it greatly improves the UX

@addyess addyess merged commit 70b81d9 into main Jul 17, 2024
7 checks passed
@addyess addyess deleted the bug/lp2073297/rbd-sc-parameters branch July 17, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants