potiuk commented on PR #68134:
URL: https://github.com/apache/airflow/pull/68134#issuecomment-5149107474
The core fix is right, and the failure it addresses is a good catch. The old
pattern was non-greedy, so an app name that itself contains `-driver` got
truncated at the first occurrence:
```
spark:my-driver-app-abc-driver old -> my-driver new ->
my-driver-app-abc-driver
```
The parametrized test pins exactly that, and it fails without the change.
Two things before this can go in.
**The description doesn't match the diff.** It says:
> Polling also now fails on pod 404 instead of treating the missing pod as a
clean exit, preventing false success and unintended pod operations.
There's no 404 handling in the diff — the only change is the regex. I'm
guessing the PR was scoped down at some point and the body wasn't updated.
Worth fixing before merge, because this body becomes the squash commit message
and would ship a changelog entry claiming a fix that isn't in the commit.
Either trim it to the regex change, or add the 404 handling back if it was
meant to be here.
**The new pattern rejects some valid pod names.** The review comment above
reads it as excluding only obviously-invalid names, but that isn't quite the
case — pod names must be RFC 1123 *subdomains*, and those permit dots:
```
app.name-driver valid k8s pod name = True matched by new regex =
False
my.team.job-driver valid k8s pod name = True matched by new regex =
False
```
Spark sanitises generated names down to `[a-z0-9-]`, so the default path is
unaffected. But `spark.kubernetes.driver.pod.name` can be set explicitly, and
then this silently stops tracking: no match leaves `_kubernetes_driver_pod` as
`None` with nothing logged, so the driver pod is simply never followed. Adding
`.` to the character class covers it:
```python
_K8S_DRIVER_POD_NAME_REGEX = re.compile(
r"(?:^|\s)submission ID
spark:([a-z0-9](?:[-.a-z0-9]*[a-z0-9])?-driver)(?=\s|$)"
)
```
The boundary cases suggested in the review above (`a-driver`, `123-driver`,
`a-1-b-2-c-3-driver`) are worth adding to the parametrize list while you're in
there — a dotted name would be a good one to include too.
Ignore the stale bot; this is a real fix and worth finishing.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
