dongjoon-hyun commented on code in PR #58730:
URL: https://github.com/apache/spark/pull/58730#discussion_r3990442898
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -961,6 +961,7 @@ private[spark] object Config extends Logging {
val KUBERNETES_VOLUMES_PVC_TYPE = "persistentVolumeClaim"
val KUBERNETES_VOLUMES_EMPTYDIR_TYPE = "emptyDir"
val KUBERNETES_VOLUMES_NFS_TYPE = "nfs"
+ val KUBERNETES_VOLUMES_CSI_TYPE = "csiVolumeClaim"
Review Comment:
All existing `VolumeType` tokens are the exact pod-spec `volumes[]` field
names (`hostPath`, `emptyDir`, `nfs`, `persistentVolumeClaim`). The K8s field
for this source is `csi`, and an inline CSI ephemeral volume does not create
any claim, so `csiVolumeClaim` is both off-convention and a bit misleading.
Since this becomes a public config key that we cannot rename later without a
deprecation path, shall we use `csi` here?
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -974,6 +975,7 @@ private[spark] object Config extends Logging {
val KUBERNETES_VOLUMES_OPTIONS_SERVER_KEY = "options.server"
val KUBERNETES_VOLUMES_LABEL_KEY = "label."
val KUBERNETES_VOLUMES_ANNOTATION_KEY = "annotation."
+ val KUBERNETES_VOLUMES_OPTIONS_CSI_DRIVER_NAME_KEY = "csiDriverName"
Review Comment:
This is the only `KUBERNETES_VOLUMES_OPTIONS_*` constant whose value is not
under `options.`, and the user-facing key `<type>.<name>.csiDriverName` breaks
the `[VolumeType].[VolumeName].options.[OptionName]` form documented in
`running-on-kubernetes.md`. I guess it was placed outside `options.` so that
the generic sweep in `KubernetesVolumeUtils` does not push the driver into
`volumeAttributes`.
How about `options.driver` (matching the K8s field name) and excluding the
known keys from the sweep, or nesting attributes under
`options.volumeAttributes.<k>`? The latter also leaves room for `fsType` /
`readOnly` / `nodePublishSecretRef` later without colliding with attribute
names. Either way, please place the constant next to the other
`KUBERNETES_VOLUMES_OPTIONS_*` keys.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala:
##########
@@ -133,6 +133,16 @@ object KubernetesVolumeUtils {
options(pathKey),
options(serverKey))
+ case KUBERNETES_VOLUMES_CSI_TYPE =>
+ val driverNameKey =
+
s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_CSI_DRIVER_NAME_KEY"
+ val volumeConfPrefix = s"$volumeType.$volumeName.options."
+ val attributes = options.filter { case (k, v) =>
k.startsWith(volumeConfPrefix) }
Review Comment:
nit: `v` is unused here. The sibling sweeps for `label.` / `annotation.`
above use `.filter(_._1.startsWith(...))`.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala:
##########
@@ -133,6 +133,16 @@ object KubernetesVolumeUtils {
options(pathKey),
options(serverKey))
+ case KUBERNETES_VOLUMES_CSI_TYPE =>
+ val driverNameKey =
+
s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_CSI_DRIVER_NAME_KEY"
+ val volumeConfPrefix = s"$volumeType.$volumeName.options."
+ val attributes = options.filter { case (k, v) =>
k.startsWith(volumeConfPrefix) }
+ .map { case (k, v) => (k.substring(volumeConfPrefix.length), v) }
Review Comment:
Since every `options.*` key becomes a volume attribute, a user who sets
`options.fsType=ext4` (following the documented options form) silently gets
`volumeAttributes["fsType"]` instead of `csi.fsType`, and there is no way to
pass `nodePublishSecretRef` at all, which many drivers require. Together with
the `readOnly` comment in `MountVolumesFeatureStep`, it would be good to decide
which `CSIVolumeSource` fields (`fsType`, `readOnly`, `nodePublishSecretRef`)
are first-class options vs. attributes before the key layout is frozen.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStep.scala:
##########
@@ -134,6 +134,13 @@ private[spark] class MountVolumesFeatureStep(conf:
KubernetesConf)
.withPath(path)
.withServer(server)
.endNfs()
+
+ case KubernetesCSIVolumeConf(driverName, attributes) =>
+ new VolumeBuilder()
+ .withNewCsi()
+ .withDriver(driverName)
Review Comment:
`spec.mountReadOnly` is applied only to the `VolumeMount` here. For CSI,
kubelet takes the `readonly` flag of `NodePublishVolumeRequest` from
`volume.csi.readOnly`, not from the mount, and some drivers (e.g.
`secrets-store.csi.k8s.io`) reject the publish unless it is `true`, which
leaves the pod in `ContainerCreating`. Could you add
`.withReadOnly(spec.mountReadOnly)` like the `persistentVolumeClaim` branch
does?
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala:
##########
@@ -133,6 +133,16 @@ object KubernetesVolumeUtils {
options(pathKey),
options(serverKey))
+ case KUBERNETES_VOLUMES_CSI_TYPE =>
+ val driverNameKey =
+
s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_CSI_DRIVER_NAME_KEY"
+ val volumeConfPrefix = s"$volumeType.$volumeName.options."
+ val attributes = options.filter { case (k, v) =>
k.startsWith(volumeConfPrefix) }
+ .map { case (k, v) => (k.substring(volumeConfPrefix.length), v) }
+ KubernetesCSIVolumeConf(
+ options(driverNameKey),
Review Comment:
+1 to @uros-b's comment on `verifyOptionKey`. Please also add
`csiVolumeClaim` parse tests to `KubernetesVolumeUtilsSuite` (happy path and
missing driver name), like the `nfs` ones, since this branch is currently
exercised only indirectly through `KubernetesTestConf`.
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala:
##########
@@ -573,4 +573,39 @@ class MountVolumesFeatureStepSuite extends SparkFunSuite {
assert(configuredPod.pod.getSpec.getVolumes.size() === 2)
assert(configuredPod.container.getVolumeMounts.size() === 2)
}
+
+ test("Mounts csiVolumeClaim") {
+ val volumeConf = KubernetesVolumeSpec(
+ "spark-local-dir-0",
Review Comment:
nit: `spark-local-dir-` is the prefix that `LocalDirsFeatureStep` treats
specially, so this name suggests the test is about local dirs. The other tests
in this suite use `testVolume`. Similarly, `csi.example.com` rather than a
vendor domain, and the `mountOptions` value does not need the embedded double
quotes. Binding `val csi = configuredPod.pod.getSpec.getVolumes.get(0).getCsi`
would also keep each assertion on one line.
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesTestConf.scala:
##########
@@ -143,6 +143,11 @@ object KubernetesTestConf {
(KUBERNETES_VOLUMES_NFS_TYPE, Map(
KUBERNETES_VOLUMES_OPTIONS_PATH_KEY -> path,
KUBERNETES_VOLUMES_OPTIONS_SERVER_KEY -> server))
+
+ case KubernetesCSIVolumeConf(driverName, attributes) =>
+ (KUBERNETES_VOLUMES_CSI_TYPE, Map(
+ KUBERNETES_VOLUMES_OPTIONS_CSI_DRIVER_NAME_KEY -> driverName
+ ) ++ attributes)
Review Comment:
After parsing, `KubernetesCSIVolumeConf.attributes` holds keys with
`options.` stripped, but this helper writes them back verbatim. That is why the
new test has to construct the conf with `"options.parentIndex"`-style keys, a
shape production never produces; a production-shaped `Map("parentIndex" ->
"0")` would silently end up as an empty `volumeAttributes`. How about
`attributes.map { case (k, v) => s"options.$k" -> v }`, like the `label.` /
`annotation.` handling in the PVC case above?
--
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]
