Skip to content

WIP - Fix k8s executor when home path different#26449

Closed
dstandish wants to merge 1 commit intoapache:mainfrom
astronomer:fix-k8s-exec-diff-home-path
Closed

WIP - Fix k8s executor when home path different#26449
dstandish wants to merge 1 commit intoapache:mainfrom
astronomer:fix-k8s-exec-diff-home-path

Conversation

@dstandish
Copy link
Contributor

@dstandish dstandish commented Sep 17, 2022

Since #21877 we no longer parse the dag as part of --local task run.

As a consequence it reads dag file location from the database. But if the dag file is in a different path on the k8s executor worker, then this will be the wrong path. We can fix this by respecting the subdir from the top level command.

One problem is, at least with k8s executor (and maybe others too?), the way it builds the pod args, it sets subdir=DAGS_FOLDER which means that all dags in the folder get parsed, which, we should avoid somehow. So, have to work on this more and see whether we can improve that.

It may be easier to just store the relative loc in the DB.

Since apache#21877 we no longer parse the dag as part of --local task run.

As a consequence it reads dag file location from the database.  But if the dag file is in a different path on the k8s executor worker, then this will be the wrong path.  We can fix this by respecting the subdir from the top level command.

One problem is, k8s executor, the way it builds the pod args, it sets subdir=DAGS_FOLDER which means that all dags in the folder get parsed, which, we should avoid somehow.  So, have to work on this more and see whether we can improve that.

It may be easier to just store the relative loc in the DB.
@boring-cyborg boring-cyborg bot added area:CLI area:Scheduler including HA (high availability) scheduler labels Sep 17, 2022
@potiuk
Copy link
Member

potiuk commented Sep 19, 2022

@pingzh - maybe you can have better idea how to fix it ?

@dstandish
Copy link
Contributor Author

dstandish commented Sep 19, 2022

@pingzh - maybe you can have better idea how to fix it ?

@potiuk, since having a DAGS_FOLDER is required full stop, why don't we always store the relative fileloc? Then airflow can always reassemble the paths relative to DAGS_FOLDER and this is a non-issue....

@ashb any concerns with this idea?

It would seem possible that we could fix this as part of an upgrade in a patch release... (i.e. update all filelocs to be relative).

And yeah... with knowledge that the paths might be different on different services..... storing the full path in fileloc doesn't make a lot of sense.

@potiuk
Copy link
Member

potiuk commented Sep 19, 2022

@potiuk, since having a DAGS_FOLDER is required full stop, why don't we always store the relative fileloc? Then airflow can always reassemble the paths relative to DAGS_FOLDER and this is a non-issue....

Sounds good. Actually I thought we've already fixed those some time ago to be relative :)

@dstandish
Copy link
Contributor Author

@potiuk, since having a DAGS_FOLDER is required full stop, why don't we always store the relative fileloc? Then airflow can always reassemble the paths relative to DAGS_FOLDER and this is a non-issue....

Sounds good. Actually I thought we've already fixed those some time ago to be relative :)

i will work on it.

@dstandish
Copy link
Contributor Author

i'm closing this one in favor of #26509

@dstandish dstandish closed this Sep 20, 2022
@ashb ashb deleted the fix-k8s-exec-diff-home-path branch September 20, 2022 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:CLI area:Scheduler including HA (high availability) scheduler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants