Skip to content

Conversation

@croomes
Copy link
Collaborator

@croomes croomes commented Jul 14, 2025

Helm chart will now install the StorageClass by default. Can be disabled using --values storageClass.enabled=false.

@croomes croomes marked this pull request as ready for review July 14, 2025 16:22
@croomes croomes requested review from a team, jmclong and landreasyan as code owners July 14, 2025 16:22
@jmclong
Copy link
Contributor

jmclong commented Jul 14, 2025

Do we need to cleanup creation of this storageclass in tests? It matches so there is no conflict for now

@croomes
Copy link
Collaborator Author

croomes commented Jul 16, 2025

Do we need to cleanup creation of this storageclass in tests? It matches so there is no conflict for now

Yes, good idea. I'm also reconsidering whether we should create the StorageClass by default?

@@ -0,0 +1,10 @@
{{- if .Values.storageClass.enabled }}
Copy link
Member

Choose a reason for hiding this comment

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

You may need this check https://github.com/kaito-project/kaito/blob/main/charts/kaito/workspace/templates/local-csi-storageclass.yaml#L3
to make it idempotent.

I meet an issue when rollback the helm.

'Error: cannot patch kaito-local-nvme-disk with kind StorageClass: StorageClass.storage.k8s.io kaito-local-nvme-disk is invalid: parameters: Forbidden: updates to parameters are forbidden.'

# should be created manually or by another tool.
enabled: true
# The name of the storage class.
name: "local"
Copy link

Choose a reason for hiding this comment

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

suggest local-csi as local is very generic,
and could easily be interpreted/conflict w/ a storage class for kubernetes-native local volumes (https://kubernetes.io/docs/concepts/storage/storage-classes/#local)

@croomes
Copy link
Collaborator Author

croomes commented Oct 28, 2025

Closing until we decide we really want this.

@croomes croomes closed this Oct 28, 2025
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.

5 participants