You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "ashangit (via GitHub)" <gi...@apache.org> on 2025/04/28 12:22:44 UTC
[PR] åCELEBORN-1990] Add support for kubernetes PVC in helm chart [celeborn]
ashangit opened a new pull request, #3237:
URL: https://github.com/apache/celeborn/pull/3237
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
Add capability to set [PVC](https://kubernetes.io/docs/concepts/storage/persistent-volumes/) as master and worker volumes
Also rely on [helm schema file](https://helm.sh/docs/topics/charts/#schema-files) to validate that volume type is within the authorised enum instead of relying on if-else-fail within the statefulset yaml file
### Why are the changes needed?
Usage of [PVC](https://kubernetes.io/docs/concepts/storage/persistent-volumes/) is a common pattern to assign storage volumes to POD within kubernetes
Those [PVC](https://kubernetes.io/docs/concepts/storage/persistent-volumes/) can be of different type (local storage, distant storage like [EBS](https://aws.amazon.com/ebs/)...)
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added helm test and deployed a celeborn cluster in our kube infrastructure based on pvc for master and workers:
```
volumes:
master:
- type: pvc
mountPath: /mnt/disk1
capacity: 10Gi
storageClassName: gp3
worker:
- type: pvc
mountPath: /mnt/disk1
capacity: 2000Gi
storageClassName: gp3
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1990] Add support for kubernetes PVC in helm chart [celeborn]
Posted by "ashangit (via GitHub)" <gi...@apache.org>.
ashangit commented on PR #3237:
URL: https://github.com/apache/celeborn/pull/3237#issuecomment-2848604219
@ChenYi015 could you please review this PR?
For the test in error I don't think it is link to this change which only affect helm chart
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1990] Add support for kubernetes PVC in helm chart [celeborn]
Posted by "ashangit (via GitHub)" <gi...@apache.org>.
ashangit closed pull request #3237: [CELEBORN-1990] Add support for kubernetes PVC in helm chart
URL: https://github.com/apache/celeborn/pull/3237
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1990] Add support for kubernetes PVC in helm chart [celeborn]
Posted by "ChenYi015 (via GitHub)" <gi...@apache.org>.
ChenYi015 commented on PR #3237:
URL: https://github.com/apache/celeborn/pull/3237#issuecomment-2871772106
> @ChenYi015 I don't think the link PR is the expected one (Merged PR around `RssShuffleManager`) I'm fine with the proposed approach and will close this one meanwhile
Sorry, have updated to PR link to #2650 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1990] Add support for kubernetes PVC in helm chart [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #3237:
URL: https://github.com/apache/celeborn/pull/3237#issuecomment-2868892822
> @ChenYi015 could you please review this PR? For the test in error I don't think it is link to this change which only affect helm chart
cc @ChenYi015
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1990] Add support for kubernetes PVC in helm chart [celeborn]
Posted by "ChenYi015 (via GitHub)" <gi...@apache.org>.
ChenYi015 commented on PR #3237:
URL: https://github.com/apache/celeborn/pull/3237#issuecomment-2871205477
Hi @ashangit @RexXiong , I have a closed PR #1528 that implements this feature, but it was too big to review. Recently, I am going to raise it again. What I am trying to do is:
- Use `master.volumes`, `master.volumeMounts` and `master.volumeClaimTemplates` to configure master pods, users will need to configure `celeborn.master.ha.ratis.raft.server.storage.dir` manually.
- Use `worker.volumes`, `worker.volumeMounts` and `worker.volumeClaimTemplates` to configure worker pods, users will need to configure `celeborn.worker.storage.dirs` manually.
This will provide users the maximum flexibility to configure Celeborn pods, as the definitions of values is the same as standard K8s Pod/Statefulset api.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1990] Add support for kubernetes PVC in helm chart [celeborn]
Posted by "ashangit (via GitHub)" <gi...@apache.org>.
ashangit commented on PR #3237:
URL: https://github.com/apache/celeborn/pull/3237#issuecomment-2871653898
@ChenYi015 I don't think the link PR is the expected one (Merged PR around `RssShuffleManager`)
I'm fine with the proposed approach and will close this one meanwhile
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1990] Add support for kubernetes PVC in helm chart [celeborn]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3237:
URL: https://github.com/apache/celeborn/pull/3237#discussion_r2084148864
##########
charts/celeborn/templates/worker/statefulset.yaml:
##########
@@ -155,4 +153,19 @@ spec:
{{- toYaml . | nindent 8 }}
{{- end }}
terminationGracePeriodSeconds: 30
-
+ {{- if eq (include "celeborn.worker.withPvc" .) "true" }}
+ volumeClaimTemplates:
+ {{- range $index, $volume := .Values.volumes.worker }}
+ {{- if eq "pvc" $volume.type }}
Review Comment:
The pvc volumeClaimTemplates block does not incorporate the diskType field, even though the values.yaml documentation indicates that diskType works with both hostPath and pvc types. Consider clarifying whether diskType should be used in the PVC configuration or update the documentation accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org