Fix failing KubeExecutor tests due to #44710

[amoghrajesh]

Recent changes brought in by #44710 leads to KubeExecutor tests failing.
The reason is because for some reason, the default_executor ends up being: ':KubernetesExecutor:'. This might not be the best fix for it and would require internally debugging things, but it will unblock main CI for now.

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

[eladkal]

Lets also please amment the commit message and change the title as this is a fix that we will note in the change log so users needs to know what this is about.

[potiuk]

Somehow that fix (while necessaryf for now) seems to be just workaround cc: @o-nikolas -> was it deliberate change or side-effect ?

[potiuk]

And good job @amoghrajesh on quick-fixing.

[amoghrajesh]

Thanks! Yeah i didnt get enough time to actually debug what went wrong. So, workaround for now

[o-nikolas]

Somehow that fix (while necessaryf for now) seems to be just workaround cc: @o-nikolas -> was it deliberate change or side-effect ?
There is a lot of context to explain haha, here goes!:

This was a deliberate change. Until now we've had the executor name/module path and optionally an alias. If it is a custom executor (provided as a module path) the str repr is <alias>:<module_path>. With a "core executor" like k8s, we don't show the module path because users just provide the always existing "KubernetesExecutor" alias. There was a specific branch in the code to do that.
Now when I wrote the changes for team_id the new repr for executors is <team_id>:<alias>:<module> and I removed the specail custom branch (or updated really, it's still a little special because no module path is shown and you can't override the alias) for the "core executors". If it is a "core executor" like Kubernetes we don't show the module path (as mentioned before) and just show its alias, and no team id exists in that test, so the string repr ends up as :KubernetesExecutor: with blanks on both sides (kind of like an AWS resource ARN). If there was a team id, it'd be my_team:KubernetesExecutor:. This is why it changed.

FWIW if it was a custom module path executor, with an alias and a team it would be my_team:my_cool_exec:exec.module.path.CoolExec

I could go back and remove this update, but I really do like that the "core executors" are moving closer inline with other executors. The problem comes with how the code here is fetching an executor name (not the executor itself) and comparing it to the airflow.executors.executor_constants.KUBERNETES_EXECUTOR which are NOT the same thing.

Does that make sense? I know it's kind of nuanced and complicated. What do folks think?

I can look on Monday at updating this code, or what a long term fix might look like.

[potiuk]

yeah. makes perfect sense. I think we should not compare it to the name but determine the class name from the "default_executor" and compare it with K8SExecutor class.

[potiuk]

(and later when team_id is served when team_id is set for executor, it should only query for tasks with the team_id of course).

[amoghrajesh]

Thanks @o-nikolas! Yeah that makes good sense.

I could go back and remove this update, but I really do like that the "core executors" are moving closer inline with other executors. The problem comes with how the code here is fetching an executor name (not the executor itself) and comparing it to the airflow.executors.executor_constants.KUBERNETES_EXECUTOR which are NOT the same thing.

Yea as Jarek mentioned, we should just try to compare the class names or the repr values instead. The logic in clear_not_launched_queued_tasks and related places should probably be updated to consider the match with team id too

[o-nikolas]

Okay, sounds good, @potiuk @amoghrajesh

I'll take a look at a more permanent change today.

Also note, that I updated me previous comment with more code-formatted text, since it was hiding some of the text! It should be even more clear to read now. Sorry for that extra confusion 😅