Add istio support to k8s operator #31389
Conversation
Continuing from joshua's work to integrate istio support into KubernetesPodOperator
boring-cyborg
bot
added
provider:cncf-kubernetes
Fix failing spell check
Add a new system test dag with istio enabled
…/Owen-CH-Leung/airflow into add-istio-support-to-k8sOperator
|
My primary concern with this is that I don't think it'd work when |
There is a pending PR to deprecate this parameter in favor of a new approch. |
|
Ah, yes, I forgot about that other PR. Either way, the concept of "keep the pod" will still be a thing. |
|
Would love a status update on this PR as the kubernetesPodOperator is basically unuseable with istio at this point, unless someone can point me to a workaround? Was as about to dig into the code for this PR to implent myself. |
I saw there're conflicts after #30718 was merged ytd. I'll work on resolving it in coming few days |
|
@eladkal @jedcunningham Can I ask for your review for this PR again ? Thanks |
|
I could use this merged with what I am doing as well. Thank you. |
| def container_is_completed(pod: V1Pod, container_name: str) -> bool: | ||
| """ | ||
| Examines V1Pod ``pod`` to determine whether ``container_name`` is completed. | ||
| If that container is present and completed, returns True. Returns False otherwise. | ||
| """ | ||
| container_status = get_container_status(pod, container_name) | ||
| if not container_status: | ||
| return False | ||
| return container_status.state.terminated is not None | ||
|
|
||
|
|
||
| def container_is_succeeded(pod: V1Pod, container_name: str) -> bool: | ||
| """ | ||
| Examines V1Pod ``pod`` to determine whether ``container_name`` is completed and succeeded. | ||
| If that container is present and completed and succeeded, returns True. Returns False otherwise. | ||
| """ | ||
| if not container_is_completed(pod, container_name): | ||
| return False | ||
|
|
||
| container_status = get_container_status(pod, container_name) | ||
| if not container_status: | ||
| return False | ||
| return container_status.state.terminated.exit_code == 0 | ||
|
|
||
|
|
There was a problem hiding this comment.
I think there is a problem with the rebase/merge, because these methods exist already in main branch
There was a problem hiding this comment.
I thought they had been merged already, just waiting on the new release so I don't have to build from source to use this. But still very eagerly waiting.
There was a problem hiding this comment.
Well. You have conflict now and you are essentially 271 commits behind. I suggest you rebase rather than merge and solve conflicts you have now - it's not clear whether your merge has been done well. When you rebase you will see only your changes on top of the current main branch.
And yes, for changes like that, it can happen that it will take long time to merge them, and that you will have to resolve conflicts and rebase few times, and that you might have conflicts with other changes.
I strongly suggest to rebase it (and solve conflicts).
There was a problem hiding this comment.
ahh... Sorry was too focused on making the code change & I performed merge instead of rebase. Will do rebase next time
| state, or the execution is interrupted. If True (default), delete the | ||
| pod; if False, leave the pod. | ||
| Deprecated - use `on_finish_action` instead. | ||
| :param istio_enabled: Whether istio is enabled in k8s cluster. False by default. |
There was a problem hiding this comment.
I wonder if we can avoid adding a new argument, by adding a new option in OnFinishAction, wdyt?
There was a problem hiding this comment.
Yes definitely. I've removed the extra param and added a new option in OnFinishAction. The logic for await_pod_completion & process_pod_deletion is updated also.
|
Thanks all for your comments. I'll perform rebase & address the comments asap |
* Makes pools respect deferrable tasks (with extra setting) See apache#21243 This commit makes pools consider deferred tasks if the `include_deferred` flag is set. By default a pool will not consider deferred tasks as occupied slots, but still show the number of deferred tasks in its stats. --------- Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
The `airflow-github` tooling had a check if PR has been already merged, but the PR did not work well when there was already a PR cherry-picked that referred the PR in question in their log message. The check was done via `git log --grep=#PR` essentially and so it happenes that `--grep` matches not only the first line of the message but the whole log message. So if the log message contained reference to the cherry-picked PR it was seen as merged. This change adds ( ) around and $ to the regexp to match not only the PR number but also parentheses around.
|
There're failing helm tests with different executors...I'll try to fix them in coming few days |
Continuing from joshua's work to integrate istio support into KubernetesPodOperator
Fix failing spell check
Add a new system test dag with istio enabled
…/Owen-CH-Leung/airflow into add-istio-support-to-k8sOperator
Owen-CH-Leung
requested review from
XD-DENG,
ashb,
bbovenzi,
eladkal,
ephraimbuddy,
kaxil,
o-nikolas,
pierrejeambrun and
ryanahamilton
as code owners
|
@Owen-CH-Leung looks like a bad rebase? |
|
Apologize all. I think I've done sth wrong when rebasing / merging and this branch is too hard to continue to work on . I'll open a new branch to continue my work |
|
@jedcunningham @hussein-awala @potiuk @eladkal I've created another PR as a continuation: |
11 participants
This PR extends the contributions of @joshuayeung, advancing the effort to integrate Istio support into the
KubernetesPodOperator. Previous PR here