Extend Edge Worker CLI commands operate on remote edge workers #49915
Conversation
boring-cyborg
bot
added
area:providers
provider:edge
jscheffl
left a comment
jscheffl
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
Looks like a good idea as extension until the UI for Airflow 3 is fully restored as feature.
Some comments during review. Already late here, after (potential) changes added I'd need a proper review.
One thing that I think about is that the UI has a state model today by rendering only the buttons possible. But the EdgeWorker model itself does not define valid state transitions. So if the user uses the "wrong" CLI command now it could potentially set the worker(s) into an inconsistent state. E.g. remove the worker while still a job is running. I think a check for the current vs. desired state should be added... and maybe best is doing it on the EdgeWorker model class and not (redundant to UI) in the CLI.
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py
Outdated
Show resolved
Hide resolved
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py
Outdated
Show resolved
Hide resolved
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py
Outdated
Show resolved
Hide resolved
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py
Outdated
Show resolved
Hide resolved
jscheffl
left a comment
jscheffl
left a comment
There was a problem hiding this comment.
Did a second pass review. Looks all well. Please don't be afraid about the comments, I assume the missing details are not complex. I like this very much!
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py
Outdated
Show resolved
Hide resolved
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py
Outdated
Show resolved
Hide resolved
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py
Outdated
Show resolved
Hide resolved
jscheffl
left a comment
jscheffl
left a comment
There was a problem hiding this comment.
Passing over the code just two thoughts - maybe not blocking. But if you have time to maybe have a thought?
As unfortunately I still was not able to add a system deployment test with edge I'd like to test manually - but it is late here today, promise to make it tomorrow. Then I hope I can approve and merge.
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py
Outdated
Show resolved
Hide resolved
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py
Outdated
Show resolved
Hide resolved
jscheffl
left a comment
jscheffl
left a comment
There was a problem hiding this comment.
I hope this is the last round of rework I will request. I think it is almost good, with just two things:
- The DB existence check - as in the other comment. Preventing large stack traces of error when calling on the wrong machine
- Maybe pushing all DB related to logic to EdgeWorker class would be great - because the CLI code is already 800+LoC and reducing just to CLI would be good. Second is optional, if you do not lick I'll make this in a follow-up PR.
All testing looks good by the way!
jscheffl
left a comment
jscheffl
left a comment
There was a problem hiding this comment.
Thanks for the contribution, the patience from review comments and fixing and the effort invested! Really like this!
…e#49915) * Extend Edge Worker CLI commands operate on remote edge workers * Fix static-checks - ruff linting/spell checks * TYPE_CHECK Session and doc string massaging * Fix more ruff spell-check and linting * Consolidate _valid_hosts to _fetch_from_db. Make the query smarter * Jens Suggestions on 04/29 * Move db related logic to edge_worker model
2 participants
EdgeExecutor is missing EdgeWorkerHosts html pages in Airflow 3. This PR attempts to bridge the gap by providing those utilities via CLI options. These options are intended to run on the server side ( or have access to the server and db). They aim to control the remote edge workers.
This PR enables the following functionality: