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.

Sign up for GitHub

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

Jump to bottom

test: 2 Ceph clusters to ensure both are read/writable #41

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

test: 2 Ceph clusters to ensure both are read/writable #41

wants to merge 4 commits into from

Conversation

addyess
Copy link
Member

@addyess addyess commented Feb 28, 2025 •
edited
Loading

Overview

Ensure that Canonical K8s (or charmed kubernetes) can operate with N Ceph Clusters without conflict

Details

  • tests that Canonical Kubernetes can support multiple ceph clusters via multiple ceph-csi charms.
  • Each ceph-csi charm relates to a separate ceph cluster
  • Accommodations at the kubelet plugin level are made to ensure that no collisions between the csi_drivers exist on the nodes where the csi drivers operate. (rbd.csi.ceph.com vs rdb.csi.ceph.com.alt)

@addyess addyess force-pushed the KU-2647/csi-driver-collision branch from 98a97ea to 0c278fb Compare March 1, 2025 06:14
@addyess addyess force-pushed the KU-2647/csi-driver-collision branch from 0c278fb to 6d8fe47 Compare March 1, 2025 06:39
addyess
addyess commented Mar 1, 2025
View reviewed changes
@addyess addyess force-pushed the KU-2647/csi-driver-collision branch from 4c5651c to 1a54ea8 Compare March 1, 2025 13:26
@addyess addyess changed the title Test 2 Ceph clusters to ensure both are read/writable test: 2 Ceph clusters to ensure both are read/writable Mar 1, 2025
addyess
addyess commented Mar 1, 2025
View reviewed changes
Copy link
Member Author

@addyess addyess left a comment

Choose a reason for hiding this comment

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

Comments to aid the reviewer

charmcraft.yaml Outdated
- {namespace} - the charm configured namespace

Example:
juju config ceph-csi csidriver-name-formatter="{name}.{app}"
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe this isn't the best example???

Which would you rather see in your cluster:
*ceph-csi-alt.rbd.csi.ceph.com
*rbd.csi.ceph.com.ceph-csi-alt

charmcraft.yaml Outdated

NOTE: Can only be specified on deployment since some
attributes of kubernetes resources are non-modifiable.
The admin is responsible for creating the namespace.
Copy link
Member Author

Choose a reason for hiding this comment

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

The admin is responsible... part is a copy/past error. The rest of the note could be improved

charmcraft.yaml
Comment on lines -84 to +106
juju config ceph-csi cephfs-storage-class-name-formatter="{cluster}-{namespace}-{storageclass}"
juju config ceph-csi cephfs-storage-class-name-formatter="cephfs-{namespace}-{pool}"
Copy link
Member Author

Choose a reason for hiding this comment

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

Drive by docs repair

charmcraft.yaml
Comment on lines -154 to +176
juju config ceph-csi ceph-xfs-storage-class-name-formatter="{cluster}-{namespace}-{storageclass}"
juju config ceph-csi ceph-xfs-storage-class-name-formatter="ceph-xfs-{app}"
Copy link
Member Author

Choose a reason for hiding this comment

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

Drive by docs repair

charmcraft.yaml
Comment on lines -182 to +204
juju config ceph-csi ceph-ext4-storage-class-name-formatter="{cluster}-{namespace}-{storageclass}"
juju config ceph-csi ceph-ext4-storage-class-name-formatter="ceph-ext4-{app}"
Copy link
Member Author

Choose a reason for hiding this comment

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

Drive by docs repair

src/manifests_base.py
Comment on lines -39 to -48
class ManifestLabelExcluder(ManifestLabel):
"""Exclude applying labels to CSIDriver."""

def __call__(self, obj: AnyResource) -> None:
super().__call__(obj)
if obj.kind == "CSIDriver" and obj.metadata and obj.metadata.labels:
# Remove the app label from the CSIDriver to disassociate it from the application
obj.metadata.labels.pop(APP_LABEL, None)


Copy link
Member Author

Choose a reason for hiding this comment

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

Hooray, now each application deploys its own CSIDriver with a different name.

src/manifests_base.py
@@ -251,3 +248,94 @@ def from_space_separated(cls, tolerations: str) -> List["CephToleration"]:
return [cls._from_string(toleration) for toleration in tolerations.split()]
except ValueError as e:
raise ValueError(f"Invalid tolerations: {e}") from e


class ProvisionerAdjustments(Patch):
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 class was yanked from each of RBDManifest and CephFSManifest and consolidated here. It does the same jobs it did before, now it's also responsible for adjusting the csidriver name where it can.

src/manifests_base.py

for c in obj.spec.template.spec.containers:
for idx in range(len(c.args)):
if original_dn in c.args[idx] and updated_dn not in c.args[idx]:
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 is by far my least favorite line.

in the upstream manifests, there are container arguments that contain *.csi.ceph.com. We need to change those argument strings to whatever the formatted name for the driver is. But we can only replace them once. Otherwise we might end up with:

alt.alt.alt.alt.alt.alt.alt.alt.alt.rdb.csi.ceph.com

if this method is called multiple times on the same resource.

src/manifests_base.py
for v in obj.spec.template.spec.volumes:
if v.hostPath:
v.hostPath.path = v.hostPath.path.replace("/var/lib/kubelet", kubelet_dir)
v.hostPath.path = v.hostPath.path.replace(original_dn, updated_dn)
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... that repeated alt.alt.alt.alt.alt.alt.alt.alt.alt.rdb.csi.ceph.com could end up here too... need to check on that.

tests/functional/overlay.yaml Outdated
@@ -47,6 +75,7 @@ applications:
options:
provisioner-replicas: 1
namespace: {{ namespace }}
csidriver-name-formatter: '{name}.alt'
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 wonder, maybe this should be switched to alt.{name}

@addyess addyess marked this pull request as ready for review March 1, 2025 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@addyess