Skip to content

Conversation

@fitbeard
Copy link
Contributor

@fitbeard fitbeard commented Sep 9, 2025

Second attempt to impement Snapshot Controller: #117
For reference first is here: #731

@fitbeard fitbeard force-pushed the feat/csi_snapshot_controller_helm branch 2 times, most recently from 2883775 to 8c5b2be Compare September 9, 2025 14:02
Signed-off-by: Tadas Sutkaitis <[email protected]>
@fitbeard fitbeard force-pushed the feat/csi_snapshot_controller_helm branch from 8c5b2be to 62459f3 Compare September 9, 2025 14:04
@fitbeard fitbeard force-pushed the feat/csi_snapshot_controller_helm branch from 34b040f to 61d2675 Compare September 16, 2025 10:02
@fitbeard
Copy link
Contributor Author

@mnaser just a highlight:)

Comment on lines +367 to +437
# Add snapshot controller CSI chart if either Cinder CSI or Manila CSI is enabled.
if cinder.is_enabled(self.cluster) or manila.is_enabled(self.cluster):

# Build volume snapshot classes based on enabled CSI drivers.
volume_snapshot_classes = []

# Add Cinder CSI volume snapshot class if enabled.
if cinder.is_enabled(self.cluster):
volume_snapshot_classes.append(
{
"name": "block-snapshot",
"annotations": {
"snapshot.storage.kubernetes.io/is-default-class": "true"
},
"driver": "cinder.csi.openstack.org",
"deletionPolicy": "Delete",
}
)

# Add Manila CSI volume snapshot class if enabled and share_network_id is specified.
if manila.is_enabled(self.cluster):
share_network_id = self.cluster.labels.get(
"manila_csi_share_network_id"
)
if share_network_id:
volume_snapshot_classes.append(
{
"name": "share-snapshot",
"annotations": {
"snapshot.storage.kubernetes.io/is-default-class": "true"
},
"driver": "nfs.manila.csi.openstack.org",
"deletionPolicy": "Delete",
"parameters": {
"csi.storage.k8s.io/snapshotter-secret-name": "csi-manila-secrets",
"csi.storage.k8s.io/snapshotter-secret-namespace": "kube-system",
},
}
)

controller_values = {
"fullnameOverride": "snapshot-controller",
"volumeSnapshotClasses": volume_snapshot_classes,
}

# Override image repository if custom registry is configured
if repository: # repository is the container_infra_prefix
repository_clean = repository.rstrip("/")
# When using a custom registry, the image is named 'csi-snapshot-controller'
controller_values["image"] = {
"repository": f"{repository_clean}/csi-snapshot-controller"
}
# For default registry, use the default image name 'snapshot-controller' from values.yaml

data = {
**data,
**{
"snapshot-controller-csi.yaml": helm.TemplateReleaseCommand(
namespace="kube-system",
release_name="snapshot-controller",
chart_ref=os.path.join(
pkg_resources.resource_filename(
"magnum_cluster_api", "charts"
),
"snapshot-controller-csi/",
),
values={"controller": controller_values},
)(repository=repository)
},
}

Copy link
Member

Choose a reason for hiding this comment

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

A lot of this code has been moved to the Rust counterpart, so probably best to move forward with putting it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please show me where to start looking.

Copy link
Member

Choose a reason for hiding this comment

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

This code should probably not be maintained as much since we have another Rust counterpart that is being used.

Copy link
Member

Choose a reason for hiding this comment

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

With the rust side of things, there is another way of configuring the images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

? This is for internal path for offline registry.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is 95% copy of this chart. There is no official chart for snapshot-controller so community is trying to cover this gap. If we ok to use third party helm charts then i can try to push few small changes to this mentioned repo and use this chart.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that if you don't mind, it's probably best for the community and help with maintenance overhead :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants