You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@livy.apache.org by "ashokkumarrathore (via GitHub)" <gi...@apache.org> on 2025/01/01 13:11:42 UTC
[PR] Livy nmsp fix [incubator-livy]
ashokkumarrathore opened a new pull request, #462:
URL: https://github.com/apache/incubator-livy/pull/462
## What changes were proposed in this pull request?
(Please fill in changes proposed in this fix)
(Include a link to the associated JIRA and make sure to add a link to this pr on the JIRA as well)
## How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
Please review https://livy.incubator.apache.org/community/ before opening a pull request.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using namspaces with Kubernetes client APIs. [incubator-livy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #462:
URL: https://github.com/apache/incubator-livy/pull/462#issuecomment-3937786950
This pull request has been automatically marked as stale because it has had no activity for at least 3 months. If you are still working on this change or plan to move it forward, please leave a comment or push a new commit so we know to keep it open. Otherwise, this PR will be closed automatically in about one month. Thank you for your contribution to Apache Livy!
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on code in PR #462:
URL: https://github.com/apache/livy/pull/462#discussion_r3442010003
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review Comment:
rebased and ran test locally
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1079] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5771995677
Thanks @gyogal — done:
- **Commit message & PR description** reformatted to match the `PULL_REQUEST_TEMPLATE` (What changes / How tested / Generative AI sections).
- Replaced `Co-Authored-By:` with **`Generated-by: Claude Opus 4.8`**.
- Added an **`Author: Ashok Kumar <as...@gmail.com>`** header — please attribute the final squashed commit to **ashokkumarrathore@gmail.com** (I've also reset the commit's Author to that address). Ready to merge whenever you are, and happy to follow up separately on the K8s docs suggestion.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using namspaces with Kubernetes client APIs. [incubator-livy]
Posted by "jahstreet (via GitHub)" <gi...@apache.org>.
jahstreet commented on code in PR #462:
URL: https://github.com/apache/incubator-livy/pull/462#discussion_r1927658781
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review 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)?
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review 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.
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review Comment:
Then we also do not need to propagate app namespace everywhere.
WDYT?
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5604357924
## [Codecov](https://app.codecov.io/gh/apache/livy/pull/462?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `24.71910%` with `67 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 53.86%. Comparing base ([`501e822`](https://app.codecov.io/gh/apache/livy/commit/501e8226cbb36d6ae00d00bb208e778c9328ff72?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e1e60d2`](https://app.codecov.io/gh/apache/livy/commit/e1e60d22292bc0cd041cc11f426e29412054ec54?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 8 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/livy/pull/462?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ala/org/apache/livy/utils/SparkKubernetesApp.scala](https://app.codecov.io/gh/apache/livy/pull/462?src=pr&el=tree&filepath=server%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Flivy%2Futils%2FSparkKubernetesApp.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya0t1YmVybmV0ZXNBcHAuc2NhbGE=) | 0.00% | [48 Missing :warning: ](https://app.codecov.io/gh/apache/livy/pull/462?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...rc/main/scala/org/apache/livy/utils/SparkApp.scala](https://app.codecov.io/gh/apache/livy/pull/462?src=pr&el=tree&filepath=server%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Flivy%2Futils%2FSparkApp.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya0FwcC5zY2FsYQ==) | 14.28% | [17 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/livy/pull/462?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...e/livy/server/interactive/InteractiveSession.scala](https://app.codecov.io/gh/apache/livy/pull/462?src=pr&el=tree&filepath=server%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Flivy%2Fserver%2Finteractive%2FInteractiveSession.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvaW50ZXJhY3RpdmUvSW50ZXJhY3RpdmVTZXNzaW9uLnNjYWxh) | 90.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/livy/pull/462?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #462 +/- ##
=============================================
- Coverage 68.68% 53.86% -14.82%
+ Complexity 1218 865 -353
=============================================
Files 106 107 +1
Lines 6815 6943 +128
Branches 836 846 +10
=============================================
- Hits 4681 3740 -941
- Misses 1666 2757 +1091
+ Partials 468 446 -22
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/livy/pull/462?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using namspaces with Kubernetes client APIs. [incubator-livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on code in PR #462:
URL: https://github.com/apache/incubator-livy/pull/462#discussion_r2326034086
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review 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?
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5594437776
Hi @ashokkumarrathore , I noticed that there are multiple related PRs opened for Kubernetes enhancements (for example #544, #545 and #546 ) and some of these address issues that your PR also addresses. Could you please take a look to see how those are related to your change and if it would be possible to similarly break up your changes to individual smaller PRs that each focus on a single new feature? Also would it be possible to open a JIRA ticket for each of the remaining changes?
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1079] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5755437083
@gyogal Can we please merge this? This will drift away otherwise.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using namspaces with Kubernetes client APIs. [incubator-livy]
Posted by "jahstreet (via GitHub)" <gi...@apache.org>.
jahstreet commented on code in PR #462:
URL: https://github.com/apache/incubator-livy/pull/462#discussion_r1929708884
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review Comment:
@gyogal can u help here please?
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1079] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5709248519
Thanks for the thorough review, @gyogal! I went through all five point by point — fixes for 1–3 are pushed; 4 and 5 don't need a change, details below.
**1. Namespace resolution ignores `SPARK_CONF_DIR` — fixed.** Good catch. Resolution now honors `$SPARK_CONF_DIR` first and falls back to `$SPARK_HOME/conf`, matching how spark-submit locates `spark-defaults.conf`, so the monitored namespace stays consistent with where the driver is actually created. Added test guards for an ambient `SPARK_CONF_DIR` (mirroring the existing `SPARK_HOME` guard).
**2. Dead `.inNamespace(namespace)` wrappers — fixed.** Confirmed they were no-ops (each extension already scopes via `app.getApplicationNamespace`); removed all four call-site wrappers. Also scoped `killApplication`'s delete to the driver pod's own namespace (`inNamespace(app.getApplicationNamespace)` instead of `inAnyNamespace`) so teardown works under a namespaced RBAC role.
**3. Recovered pre-upgrade session pins `""` into `appNamespaces` — fixed.** Now guarded with `if (namespace.nonEmpty)`, so an empty ("unknown") namespace is never added to the GC set. The real namespace is still adopted and tracked once the driver pod is discovered, so recovered pre-upgrade sessions no longer force a cluster-wide scan.
**4. Rollback / `FAIL_ON_UNKNOWN_PROPERTIES` — I don't think the suggested fix actually helps here.** On a downgrade it's the *older* build's `ObjectMapper` that deserializes the new `namespace` field and throws, and that older binary won't contain this change — so adding `FAIL_ON_UNKNOWN_PROPERTIES=false` to the current build can't fix rollback to a pre-LIVY-461 build. It would only matter for a future rollback *to this* build, and it's a global change to all recovery deserialization unrelated to this feature. The upgrade direction already works: a new build reading old metadata gets the absent field as `null`, which the code treats as "unknown". Happy to add the leniency as a separate hardening change if you'd prefer, but I've left it out of this PR.
**5. fabric8 5.6.0 → 6.8.1 — intentional, and no 6.x source migration was required.** The bump is deliberate (5.6.0 carries known CVEs). It's source-compatible with 6.8.1 as-is, which the green CI confirms: fabric8 6.x still ships the deprecated `DefaultKubernetesClient` (removed only in 7.0.0) and the boolean-returning `delete(item)` overload still resolves, so `new DefaultKubernetesClient(config)`, the `DefaultKubernetesClient` types, and `killApplication: Boolean` all compile and pass tests. A future move to fabric8 7.x would need the full migration you describe (`KubernetesClientBuilder`, `delete()` → `List<StatusDetails>`); worth a follow-up when we cross that version.
All checks are green on the latest revision. Thanks again!
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5674675794
Yes @gyogal , I discussed with @soumyadeeplogin , and decided to separate out these PRs. We are part of same company.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-4751685891
Pushed an update that folds in five additional Kubernetes/session fixes that were developed on top of this branch (authored by @vallenki). All are rebased onto current `master`, compile cleanly against kubernetes-client 6.8.1, pass scalastyle/checkstyle, and have unit tests:
1. **Livy Interactive session fixes** — adds `livy.rsc.driver.address.use-hostname` (RSCConf) so the driver's reported hostname is used instead of the socket IP, which is needed for Kubernetes + Istio setups.
2. **Reduce Kubernetes API load in polling** — narrows the driver lookup to `spark-role=driver` at the API server (single-pod LIST instead of all executor pods) and gates the executor LIST behind a new flag `livy.server.kubernetes.executor-tracking.enabled` (default `true`, so no behavior change). Large clusters can disable it to skip per-executor LISTs.
3. **SessionManager refresh + `/recovery` operator endpoint** — a superuser-guarded (`livy.superusers`, returns 403 otherwise) REST endpoint to re-scan the recovery state store and import sessions written by another Livy server, without a restart.
4. **Fix recovery-file leak after session deletion** — `kill()` now removes the app from the shared monitor queue and the monitor early-returns after a terminal state, so a deleted session's recovery file is no longer resurrected by an in-flight monitor tick. `killed` becomes `AtomicBoolean` for cross-thread visibility.
5. **Fix `Promise.failure` race that falsely failed healthy apps** — switches the catch path to `Promise.tryFailure`, matching the `trySuccess` on the success path, so a late failure on an already-completed promise is dropped instead of flipping a healthy app to FAILED. This became reachable once `app-lookup.thread-pool.size > 1`.
Combined diff is ~545 insertions across 16 files. Happy to split any of these into separate PRs if you'd prefer to keep this one scoped purely to multi-namespace support — just let me know.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-4765453217
@gyogal @jahstreet looks like tests are passing now. Just fyi, we are running Livy server in production with these changes to run spark on k8s jobs.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1079] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal merged PR #462:
URL: https://github.com/apache/livy/pull/462
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5597065338
Thanks @gyogal — you're right, this PR had grown to bundle several independent changes, and there's real overlap with the newer Kubernetes PRs. Here's how they relate and how I've split things up:
**Already merged upstream — dropped from this PR:**
- The `driver.address.use-hostname` RSC flag is the same change as #544 (LIVY-1071).
- The Kubernetes polling API-load reduction is the same as #545 (LIVY-1072).
I've rebased this branch onto current `master`, which already carries both.
**Overlaps an open PR — dropped here:**
- The `SessionManager` refresh + `/recovery` operator endpoint overlaps #546 (LIVY-1073). I'll let #546 be the single source for that.
**This PR now focuses on exactly one feature:**
- **LIVY-461** — Multi-tenant (multi-namespace) Kubernetes support (namespace resolution + `inNamespace()` request threading). I've force-pushed the branch down to just this change.
**Two remaining independent bug fixes — a focused PR + JIRA each:**
1. Fix recovery-file leak after session deletion.
2. Fix `Promise.failure` race that falsely fails healthy Spark apps.
I'll file JIRAs for those two and open a small PR for each, then link them back here.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using namspaces with Kubernetes client APIs. [incubator-livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on code in PR #462:
URL: https://github.com/apache/incubator-livy/pull/462#discussion_r1928059767
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review Comment:
Thank you for inputs @jahstreet .
1. 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.
2. 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.
3. 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.
4. 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.
5. 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.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5683996833
@ashokkumarrathore Thanks for your reply! We do not use GitHub for issue tracking. Could you please create a JIRA at https://issues.apache.org/ ?
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "soumyadeeplogin (via GitHub)" <gi...@apache.org>.
soumyadeeplogin commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5675651941
Hi @gyogal — happy to weigh in. I've discussed this with @ashokkumarrathore and confirmed there's no remaining overlap between this PR (now scoped to multi-tenant/multi-namespace K8s support) and #544/#545/#546 — the overlapping pieces (RSC hostname flag, K8s polling reduction, and the /recovery endpoint) have already been dropped from this PR. Looks good from my side.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1079] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5701912820
Thanks @ashokkumarrathore ! I asked Claude to review the PR and here are the main potential issues found. Can you please check if these are relevant?
**LIVY-461 — a few things worth addressing before merge:**
1. **Namespace resolution ignores `SPARK_CONF_DIR`** (High). `namespaceFromSparkDefaults` only reads `$SPARK_HOME/conf/spark-defaults.conf`, but Spark resolves
`SPARK_CONF_DIR` first. If the namespace is set there, it's missed → falls back to `"default"` → the namespace-scoped LIST never finds the driver. (The old `inAnyNamespace` behavior masked this.)
2. **Dead `.inNamespace(namespace)` wrappers** (Medium). Every extension method already scopes internally via `app.getApplicationNamespace`, so the
`kubernetesClient.inNamespace(namespace).<ext>(...)` call sites (and the leaked-GC one) are no-ops. Drop them, and scope `killApplication`'s delete explicitly to the pod's namespace for namespaced-RBAC teardown.
3. **Recovered pre-upgrade session pins `""` into `appNamespaces`** (Medium). A session recovered from pre-LIVY-461 metadata has `namespace = ""`, which gets added to
the GC set and never removed → every GC cycle does a cluster-wide `inAnyNamespace` scan, forbidden for a namespace-scoped service account. Guard with `if (namespace.nonEmpty)`.
4. **Rollback breaks recovery deserialization** (Low). The recovery `ObjectMapper` doesn't disable `FAIL_ON_UNKNOWN_PROPERTIES`, so rolling back to a pre-LIVY-461 build
throws on the new `namespace` field. Add `.configure(FAIL_ON_UNKNOWN_PROPERTIES, false)`.
5. **fabric8 5.6.0 → 6.8.1 is a breaking major bump** (High). Needs the 6.x API migration: `KubernetesClientBuilder` instead of `DefaultKubernetesClient`, `delete()`
now returns `List<StatusDetails>` (not `boolean`), and `KubernetesClient` types in place of `DefaultKubernetesClient`. Please confirm this was intentional and migrated, not just a version pin.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5636800255
Hi @ashokkumarrathore , thank you for reworking your PR, it looks much clearer now. We have received multiple Kubernetes related changes from @soumyadeeplogin (#544, #545 and #546) and I would just like to make sure those are compatible with your changes, so if you have any feedback on those it would be much appreciated! Similarly, @soumyadeeplogin if you have any comments on this PR, please let us know, it would help us a great deal!
We are currently in the middle of the Spark 4 upgrade, so once that is merged (in a few days), we could look into merging these changes as well.
In the meantime, could you please update the name of the PR so that it includes the JIRA ticket and please also update the description to include the AI section according to the Livy PR template at https://github.com/apache/livy/blob/master/.github/PULL_REQUEST_TEMPLATE - this would help so that it is not lost among some older stale PRs.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using namspaces with Kubernetes client APIs. [incubator-livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on code in PR #462:
URL: https://github.com/apache/incubator-livy/pull/462#discussion_r1931206921
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review 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.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-4750907671
Rebased onto the latest `master` and resolved all conflicts — the PR is now mergeable. Summary of what changed since the last review:
**Rebase / conflict resolution**
- Rebased onto current `master` (the branch was ~42 commits behind).
- Dropped changes that `master` has since made independently: the `commons.lang3` import and the `SQLInterpreterSpec` assertion (kept `master`'s broader version).
- Removed redundant per-module pom overrides (maven-shade-plugin, commons-lang3, netty-all, hadoop-common) now that the parent pom manages those versions; one of them would otherwise have downgraded maven-shade-plugin 3.5.0 → 3.4.1.
**Version bumps (security)** — kept the higher version across `master`/PR: kubernetes-client `6.8.1`, netty `4.1.108.Final`. Verified the code compiles and unit tests pass against the fabric8 6.x client.
**Bug fix in `SparkApp.getNamespace`** — the previous version unconditionally read `$SPARK_HOME/conf/spark-defaults.conf` and called `.get` on `sparkHome()`, which threw `FileNotFoundException`/`NoSuchElementException` and broke batch-session creation (and several existing tests) for non-Kubernetes deployments. It now:
- resolves a namespace only on Kubernetes (empty string otherwise, no filesystem access),
- treats a missing or key-less `spark-defaults.conf` as "no value" and falls back to the `default` namespace,
- closes the input stream.
This also addresses @jahstreet's earlier question about whether the namespace can be empty initially.
**Tests** — added `SparkAppSpec` covering the full namespace-resolution precedence chain (session conf → `spark-defaults.conf` → default), plus the non-Kubernetes and missing-`SPARK_HOME` paths. All touched suites pass locally (`SparkAppSpec`, `BatchSessionSpec`, `SessionManagerSpec`, `SparkKubernetesAppSpec`); scalastyle is clean.
Commits were squashed into one. @gyogal — could you kick off the check jobs when you get a chance? Thanks!
> Note on the watched-namespace truncation idea (@jahstreet): I've left that out of this PR to keep it focused; happy to follow up in a separate change since picking a safe truncation point needs more thought.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5683639745
@gyogal The issue mentioned above is a github issue : https://github.com/apache/livy/issues/461
Not the JIRA ticket. Not sure if JIRA gets created from github issue automatically. If not, i will create a JIRA issue, requested an account for the same.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on code in PR #462:
URL: https://github.com/apache/livy/pull/462#discussion_r4016722798
##########
pom.xml:
##########
@@ -1426,6 +1426,13 @@
</reporting>
<profiles>
+ <profile>
+ <id>hadoop3</id>
+ <properties>
+ <hadoop.major-minor.version>3</hadoop.major-minor.version>
+ <hadoop.version>3.4.0</hadoop.version>
+ </properties>
+ </profile>
Review Comment:
This new profile may not be necessary, since the default Hadoop version is now 3.3.4 (there is a hadoop2 profile to set it to 2.7.3)
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on code in PR #462:
URL: https://github.com/apache/livy/pull/462#discussion_r4017985012
##########
pom.xml:
##########
@@ -1426,6 +1426,13 @@
</reporting>
<profiles>
+ <profile>
+ <id>hadoop3</id>
+ <properties>
+ <hadoop.major-minor.version>3</hadoop.major-minor.version>
+ <hadoop.version>3.4.0</hadoop.version>
+ </properties>
+ </profile>
Review Comment:
Good catch. That hadoop3 profile was a leftover from the branch's earlier base and is now redundant, removed it.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5630564641
@gyogal @Rchie121 Can we please merge this?
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using namspaces with Kubernetes client APIs. [incubator-livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on code in PR #462:
URL: https://github.com/apache/incubator-livy/pull/462#discussion_r1928866408
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review 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?
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using namspaces with Kubernetes client APIs. [incubator-livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on code in PR #462:
URL: https://github.com/apache/incubator-livy/pull/462#discussion_r1931206921
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review Comment:
Thank you @ashokkumarrathore for the PR and many thanks 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.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using namspaces with Kubernetes client APIs. [incubator-livy]
Posted by "duc-dn (via GitHub)" <gi...@apache.org>.
duc-dn commented on code in PR #462:
URL: https://github.com/apache/incubator-livy/pull/462#discussion_r2297003791
##########
server/src/main/scala/org/apache/livy/utils/SparkKubernetesApp.scala:
##########
@@ -138,6 +138,7 @@ object SparkKubernetesApp extends Logging {
private var sessionLeakageCheckInterval: Long = _
var kubernetesClient: DefaultKubernetesClient = _
+ var appNamespaces: mutable.Set[String] = mutable.Set("default")
Review Comment:
Hi @gyogal, do you have any updates on this PR?
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5683483353
Thanks for your review @soumyadeeplogin and thanks for the contribution! Please see my earlier comments, in order to commit this, a JIRA will be needed because [LIVY-461](https://issues.apache.org/jira/browse/LIVY-461) seems unrelated.
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "ashokkumarrathore (via GitHub)" <gi...@apache.org>.
ashokkumarrathore commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5698445348
Fixes : https://issues.apache.org/jira/browse/LIVY-1079
@gyogal fyi .
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5685255991
Thanks @ashokkumarrathore ! Please make sure the "Your real (public) name" field on the new user request is filled correctly, because it is not easy to change it later or and that is what will appear on JIRA tickets as "Reporter" and "Assignee".
--
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: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org