Spark on k8s : changes for using namspaces with Kubernetes client APIs. #462
Conversation
ashokkumarrathore
changed the title
| private var sessionLeakageCheckInterval: Long = _ | ||
|
|
||
| var kubernetesClient: DefaultKubernetesClient = _ | ||
| var appNamespaces: mutable.Set[String] = mutable.Set("default") |
There was a problem hiding this comment.
Why do we need to have a default namespaces at all? Can it be empty initially? Also is it thread safe (or can we make it so)?
There was a problem hiding this comment.
Did you consider making configurable allowed namespaces list? We anyway know it upfront when configuring permissions for Live service account. It worths checking how the recovery works, I haven't looked at it for a while, but having Livy monitoring all the namespaces from the get go may save us from leaked/unrecovered apps.
There was a problem hiding this comment.
Then we also do not need to propagate app namespace everywhere.
WDYT?
There was a problem hiding this comment.
Thank you for inputs @jahstreet .
- If user does not specify a namespace, the job goes to default namespace. That's why we start with that. But we can remove it too. As it will anyway get populated after first job submission. For thread safety, i will switch it to java.util.concurrent.ConcurrentHashMap.
- The configurable namespace allowlist would work but that would need deployment everytime a new namespace is allowed. The rolebindings can be created in new namespaces(to allow job submission from livy service account) independently.
- The reason we are maintaining this list of namespaces is for leaked apps. The idea is, if we go through all namespaces where jobs were submitted, that should be enough.
- Livy monitoring all namespaces is not feasible due to security. In a multi tenant cluster, due to security, livy service account might not have any permissions on some namespaces.
- For recovery, we are also saving the namespace now in [server/src/main/scala/org/apache/livy/server/batch/BatchSession.scala] which will be used to recover.
There was a problem hiding this comment.
Thanks, it covers most of my thoughts 👍 .
In case namespaces are dynamic, should we also truncate the set of watched namespaces to avoid revoked access rights to the namespaces and namespace deletion edge cases?
There was a problem hiding this comment.
That's a good point. The challenge would be to when to truncate. One condition i can think of is, just after deleting all leaked apps. In a rare case this might still leave some leaked apps(apps which were just added to leaked list).I will see what other options we have.
I still need to write some unit tests. But before that i want to make sure existing tests pass. How do i get this workflow approved from maintainer?
There was a problem hiding this comment.
@gyogal can u help here please?
There was a problem hiding this comment.
Thank you @ashokkumarrathore for the PR and many thanks also to @askhatri, @jahstreet for the feedback! The unit test executor is currently broken and I am in the process of fixing it. As soon as it is working again, I will start the check jobs on this PR.
There was a problem hiding this comment.
Hi @gyogal, do you have any updates on this PR?
There was a problem hiding this comment.
The unit test jobs are now fixed, however the PR needs to be rebased to the end of the branch so that they can be started. There are also some new conflicts. Would it be possible to rebase this PR?
4 participants
What changes were proposed in this pull request?
Related issue : #461
How was this patch tested?
Tested by deploying updated server in a k8s cluster with namespaces and running a job.