Reamer commented on a change in pull request #4174:
URL: https://github.com/apache/zeppelin/pull/4174#discussion_r685015571
##########
File path:
zeppelin-plugins/launcher/k8s-standard/src/main/java/org/apache/zeppelin/interpreter/launcher/K8sStandardInterpreterLauncher.java
##########
@@ -95,9 +100,11 @@ String getHostname() {
*/
private String getZeppelinService(InterpreterLaunchContext context) throws
IOException {
if (isRunningOnKubernetes()) {
+ //The namespace of zeppelin server can only be read from
Config.KUBERNETES_NAMESPACE_PATH while it runs in k8s cluster, it may be
different from the namespace of interpreter
+ String serverNamespace = readFile(Config.KUBERNETES_NAMESPACE_PATH,
Charset.defaultCharset()).trim();
Review comment:
Please move this part to a separate function. Something like
`getServerNamespace()`.
##########
File path:
zeppelin-plugins/launcher/k8s-standard/src/main/java/org/apache/zeppelin/interpreter/launcher/K8sStandardInterpreterLauncher.java
##########
@@ -70,6 +70,11 @@ boolean isRunningOnKubernetes() {
* @throws IOException if namespace file could not be read
*/
String getNamespace() throws IOException {
+ //namespace may be set from the interpreter properties
Review comment:
The `K8sStandardInterpreterLauncher` is executed only on the Zeppelin
server side.
Please rename this function to `getInterpreterNamespace()`.
##########
File path:
zeppelin-plugins/launcher/k8s-standard/src/main/java/org/apache/zeppelin/interpreter/launcher/K8sRemoteInterpreterProcess.java
##########
@@ -275,7 +275,7 @@ Properties getTemplateBindings(String userName) {
Properties k8sProperties = new Properties();
// k8s template properties
- k8sProperties.put("zeppelin.k8s.namespace", getNamespace());
+ k8sProperties.put("zeppelin.k8s.interpreter.namespace", getNamespace());
Review comment:
I think this should be the place where you need to call another method,
something like `getInterpreterNamespace()`, where the default value is the
output of `getServerNamespace()`.
##########
File path: k8s/zeppelin-server.yaml
##########
@@ -213,14 +213,15 @@ rules:
resources: ["roles", "rolebindings"]
verbs: ["bind", "create", "get", "update", "patch", "list", "delete",
"watch"]
---
-kind: RoleBinding
+kind: ClusterRoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: zeppelin-server-role-binding
subjects:
- kind: ServiceAccount
name: zeppelin-server
+ namespace: default
Review comment:
It is not sure that the `zeppelin-server` ServiceAccount is part of the
`default` namespace.
##########
File path:
zeppelin-plugins/launcher/k8s-standard/src/main/java/org/apache/zeppelin/interpreter/launcher/K8sRemoteInterpreterProcess.java
##########
@@ -135,6 +139,43 @@ public String getNamespace() {
return namespace;
Review comment:
Rename this variable to `serverNamespace` and change the function name
to `getServerNamespace()`.
--
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: dev-unsubscr...@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
