-
Notifications
You must be signed in to change notification settings - Fork 14.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
spark-on-k8s sensor - add driver logs #10023
Conversation
@roitvt Can I ask for a review? |
For sure I'll do it tomorrow :) |
@roitvt, sorry to bother you - do you think you'll have time to take a look this week? |
Hi Beni, I'm really sorry I'll do it this week. thank you very much for adding this functionality and I'll be glad to have a talk with you about using this integration and Spark on K8s :) |
Great, thanks 👍 |
LGTM works well with spark-pi. |
What is your mail? |
sent on FB |
@mik-laj Can this be merged? |
watcher = watch.Watch() | ||
return ( | ||
watcher, | ||
watcher.stream( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#9570
Does this problem occur here? What permissions are needed for this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes a role similar to the one below ( specific namespace role or ClusterRole. In any case the operator is constrained to the job namespace )
- apiGroups: [""]
resources: ["pods", "pods/log"]
verbs: ["get", "watch", "list"]
It's important to mention that I opted against using the watcher, but left this here for future use ( I'm using read_namespaced_pod_log
, which still requires the mentioned permissions )
Should we add a manifest that deals with it for a system test? similar to RBAC yamls from the operator here
airflow/tests/providers/cncf/kubernetes/operators/test_spark_kubernetes_system.py
Line 36 in 7d24b08
SPARK_OPERATOR_MANIFESTS = [ |
In any case I don't think it should block, as it depends on explicitly adding the flag
Is this log available in another application? Maybe a better solution would be to add a link to another console? https://airflow.readthedocs.io/en/latest/howto/write-logs.html#external-links |
Not necessarily, as it requires either For a user with airflow access alone, we want the ability to quickly inspect driver logs while inspecting the task without delegating to external systems |
c50feca
to
4990b29
Compare
… call (#11199) This is a follow up to #10023 - it seems that passing the defined namespace to the log call was missed in one of the rebases done during the PR. Without this fix, logging will fail when the k8s connection uses a different namespace than the one SparkApplication(s) are actually submitted to.
Simple implementation for appending driver pod logs on completion when using SparkKubernetesSensor.
Originally we discussed monitoring the pod as part of the operator, similar to the implementation for the k8s pod launcher but decided against it for now so we don't introduce breaking changes.