advancedxy commented on code in PR #466:
URL: https://github.com/apache/incubator-uniffle/pull/466#discussion_r1065768888
##########
deploy/kubernetes/operator/api/uniffle/v1alpha1/zz_generated.deepcopy.go:
##########
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
Review Comment:
is this necessary?
##########
deploy/kubernetes/operator/pkg/controller/sync/shuffleserver/shuffleserver.go:
##########
@@ -250,6 +261,11 @@ func generateStorageBasePath(rss
*unifflev1alpha1.RemoteShuffleService) string {
}
paths = append(paths, strings.TrimSuffix(v, "/")+"/rssdata")
}
+
+ for _, vm := range rss.Spec.ShuffleServer.VolumeMounts {
+ paths = append(paths, strings.TrimSuffix(vm.MountPath,
"/"+"rssdata"))
Review Comment:
Is this correct? The `TrimSuffix` doesn't match with L262.
##########
deploy/kubernetes/operator/api/uniffle/v1alpha1/remoteshuffleservice_types.go:
##########
@@ -136,6 +136,29 @@ type ShuffleServerConfig struct {
// UpgradeStrategy defines upgrade strategy of shuffle servers.
UpgradeStrategy *ShuffleServerUpgradeStrategy `json:"upgradeStrategy"`
+
+ // volumeClaimTemplates is a list of claims that pods are allowed to
reference.
+ // The StatefulSet controller is responsible for mapping network
identities to
+ // claims in a way that maintains the identity of a pod. Every claim in
+ // this list must have at least one matching (by name) volumeMount in
one
+ // container in the template. A claim in this list takes precedence over
+ // any volumes in the template, with the same name.
+ // +optional
+ VolumeClaimTemplates []ShuffleServerPersistentVolumeClaimTemplate
`json:"volumeClaimTemplates,omitempty"
protobuf:"bytes,4,rep,name=volumeClaimTemplates"`
Review Comment:
is it better to define `VolumeClaimTemplates` as part of RSSPodSpec?
##########
deploy/kubernetes/operator/api/uniffle/v1alpha1/remoteshuffleservice_types.go:
##########
@@ -136,6 +136,29 @@ type ShuffleServerConfig struct {
// UpgradeStrategy defines upgrade strategy of shuffle servers.
UpgradeStrategy *ShuffleServerUpgradeStrategy `json:"upgradeStrategy"`
+
+ // volumeClaimTemplates is a list of claims that pods are allowed to
reference.
+ // The StatefulSet controller is responsible for mapping network
identities to
+ // claims in a way that maintains the identity of a pod. Every claim in
+ // this list must have at least one matching (by name) volumeMount in
one
+ // container in the template. A claim in this list takes precedence over
+ // any volumes in the template, with the same name.
+ // +optional
+ VolumeClaimTemplates []ShuffleServerPersistentVolumeClaimTemplate
`json:"volumeClaimTemplates,omitempty"
protobuf:"bytes,4,rep,name=volumeClaimTemplates"`
+}
+
+type ShuffleServerPersistentVolumeClaimTemplate struct {
+ // May contain labels and annotations that will be copied into the PVC
+ // when creating it. No other fields are allowed and will be rejected
during
+ // validation.
+ //
+ VolumeNameTemplate *string `json:"volumeNameTemplate"`
+
+ // The specification for the PersistentVolumeClaim. The entire content
is
+ // copied unchanged into the PVC that gets created from this
+ // template. The same fields as in a PersistentVolumeClaim
+ // are also valid here.
+ Spec corev1.PersistentVolumeClaimSpec `json:"spec"
protobuf:"bytes,2,name=spec"`
Review Comment:
To be consistent with other field definitions, I believe the `protobuf`
struct tag is not necessary?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
