pan3793 commented on code in PR #7710:
URL: https://github.com/apache/kyuubi/pull/7710#discussion_r3916091177
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1425,9 +1425,12 @@ object KyuubiConf {
val KUBERNETES_CLIENT_INITIALIZE_LIST: ConfigEntry[Seq[String]] =
buildConf("kyuubi.kubernetes.client.initialize.list")
.doc("The kubernetes client initialize list to register kubernetes
resource informers" +
- " during Kyuubi server startup. This ensure the Kyuubi server is
promptly informed for" +
- " any Kubernetes resource changes after startup. It is highly
recommend to set it for" +
- " multiple Kyuubi instances mode. The format is
`context1:namespace1,context2:namespace2`.")
+ " during Kyuubi server startup. This ensures the Kyuubi server is
promptly informed for" +
+ " any Kubernetes resource changes after startup. It is highly
recommended to set it for" +
+ " multiple Kyuubi instances mode. The format is" +
+ " `context1:namespace1,context2:namespace2`." +
+ " When the list is empty and Kyuubi runs in Kubernetes, the client for
the configured" +
+ " namespace is initialized automatically with the in-cluster
configuration.")
Review Comment:
Since `kyuubi.kubernetes.namespace` now also selects the namespace watched
by the auto-initialized in-cluster client, please fix its doc
(`KUBERNETES_NAMESPACE`, above) in this PR - the current text "running the
kyuubi pods" is misleading, as it has never controlled where the Kyuubi server
runs. Suggestion:
```
The default namespace used by the Kyuubi server's Kubernetes client to
discover and
manage engine pods, when the engine submission does not specify one (e.g.
`spark.kubernetes.namespace`). It does not control the namespace where the
Kyuubi
server itself runs.
```
Remember to regenerate `settings.md` after the change.
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1425,9 +1425,12 @@ object KyuubiConf {
val KUBERNETES_CLIENT_INITIALIZE_LIST: ConfigEntry[Seq[String]] =
buildConf("kyuubi.kubernetes.client.initialize.list")
.doc("The kubernetes client initialize list to register kubernetes
resource informers" +
- " during Kyuubi server startup. This ensure the Kyuubi server is
promptly informed for" +
- " any Kubernetes resource changes after startup. It is highly
recommend to set it for" +
- " multiple Kyuubi instances mode. The format is
`context1:namespace1,context2:namespace2`.")
+ " during Kyuubi server startup. This ensures the Kyuubi server is
promptly informed for" +
+ " any Kubernetes resource changes after startup. It is highly
recommended to set it for" +
+ " multiple Kyuubi instances mode. The format is" +
+ " `context1:namespace1,context2:namespace2`." +
+ " When the list is empty and Kyuubi runs in Kubernetes, the client for
the configured" +
+ " namespace is initialized automatically with the in-cluster
configuration.")
.version("1.11.0")
Review Comment:
Note for other reviewers: this entry shipped in v1.10.2 via the branch-1.10
backport of KYUUBI #7027, though `version()` says 1.11.0 - pre-existing, not
related to this PR.
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/KubernetesApplicationOperation.scala:
##########
@@ -217,14 +218,31 @@ class KubernetesApplicationOperation extends
ApplicationOperation with Logging {
}
private[kyuubi] def getKubernetesClientInitializeInfo(
- kyuubiConf: KyuubiConf): Seq[KubernetesInfo] = {
- kyuubiConf.get(KyuubiConf.KUBERNETES_CLIENT_INITIALIZE_LIST).map { init =>
- val (context, namespace) = init.split(":") match {
- case Array(ctx, ns) => (Some(ctx).filterNot(_.isEmpty),
Some(ns).filterNot(_.isEmpty))
- case Array(ctx) => (Some(ctx).filterNot(_.isEmpty), None)
- case _ => (None, None)
+ kyuubiConf: KyuubiConf,
+ environment: Map[String, String] = sys.env,
+ serviceAccountTokenPath: Path =
Paths.get(Config.KUBERNETES_SERVICE_ACCOUNT_TOKEN_PATH),
+ serviceAccountCaCertPath: Path = Paths.get(
+ Config.KUBERNETES_SERVICE_ACCOUNT_CA_CRT_PATH)): Seq[KubernetesInfo] =
{
+ val configuredInitializeInfo =
+ kyuubiConf.get(KyuubiConf.KUBERNETES_CLIENT_INITIALIZE_LIST).map { init
=>
+ val (context, namespace) = init.split(":") match {
+ case Array(ctx, ns) => (Some(ctx).filterNot(_.isEmpty),
Some(ns).filterNot(_.isEmpty))
+ case Array(ctx) => (Some(ctx).filterNot(_.isEmpty), None)
+ case _ => (None, None)
+ }
+ KubernetesInfo(context, namespace)
}
- KubernetesInfo(context, namespace)
+ if (configuredInitializeInfo.nonEmpty) {
+ configuredInitializeInfo
+ } else if (environment.get(KUBERNETES_SERVICE_HOST).exists(_.nonEmpty) &&
+ environment.get(KUBERNETES_SERVICE_PORT).exists(_.nonEmpty) &&
+ Files.isReadable(serviceAccountTokenPath) &&
+ Files.isReadable(serviceAccountCaCertPath)) {
+ Seq(KubernetesInfo(
+ None,
Review Comment:
Please add a brief comment that `context` is intentionally `None` here,
since kube context is meaningless for in-cluster configuration.
--
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]
