-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
[EDGE]Enable edge worker maintenance mode #45958
base: main
Are you sure you want to change the base?
[EDGE]Enable edge worker maintenance mode #45958
Conversation
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.
Copilot reviewed 5 out of 15 changed files in this pull request and generated no comments.
Files not reviewed (10)
- providers/edge/README.rst: Language not supported
- providers/edge/docs/changelog.rst: Language not supported
- providers/edge/src/airflow/providers/edge/openapi/edge_worker_api_v1.yaml: Language not supported
- providers/edge/src/airflow/providers/edge/plugins/templates/edge_worker_hosts.html: Language not supported
- providers/edge/src/airflow/providers/edge/cli/api_client.py: Evaluated as low risk
- providers/edge/src/airflow/providers/edge/get_provider_info.py: Evaluated as low risk
- providers/edge/provider.yaml: Evaluated as low risk
- providers/edge/tests/provider_tests/edge/cli/test_edge_command.py: Evaluated as low risk
- providers/edge/src/airflow/providers/edge/init.py: Evaluated as low risk
- providers/edge/pyproject.toml: Evaluated as low risk
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.
Cool! some review comments (please don't be afraid) - looks good in general and the feature is really something that is improving Edge.
providers/edge/src/airflow/providers/edge/models/edge_worker.py
Outdated
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/models/edge_worker.py
Outdated
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/models/edge_worker.py
Outdated
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/models/edge_worker.py
Outdated
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/plugins/templates/edge_worker_hosts.html
Outdated
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/worker_api/routes/worker.py
Outdated
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/worker_api/routes/worker.py
Outdated
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/plugins/templates/edge_worker_hosts.html
Outdated
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/plugins/templates/edge_worker_hosts.html
Outdated
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/plugins/edge_executor_plugin.py
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/plugins/templates/edge_worker_hosts.html
Outdated
Show resolved
Hide resolved
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.
A failure in static checks - needs a bit of ruff re-formatting. Please run pre-commit and then it should turn green.
Can you please double check on the comments, smells like logical errors. If fixed, then I would make a final test and then I think we are close to merge...
@@ -395,9 +397,11 @@ def heartbeat(self) -> None: | |||
if _EdgeWorkerCli.maintenance_mode: | |||
logger.info("Exit Maintenance mode requested!") | |||
_EdgeWorkerCli.maintenance_mode = False | |||
worker_state_changed = worker_info.state == state |
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.
Are you sure this is the right check? Why has the state changed when the server response equals the reported state?
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.
I am not sure whether the changes on API return value (list->Pydantic Model) will work with Airflow 2.10 - the OpenAPI Manifest needs to be adjusted? Or is the change working without? Because only the new ENUM values are added here.
Maintenance mode is enabled for the edge worker. In maintenance mode, the worker is alive, but cannot consume any jobs.
The maintenance mode can be triggered by a button from the edge worker status page. It writes the state "maintenance request" directly to the database as worker state. Then the worker will go to maintenance pending if there are running jobs, and maintenance mode if all jobs have finished.
When exiting maintenance mode, maintenance exit is written to the database. Then the worker will switch to running state if it was in state maintenance pending, and to idle if it was in maintenance mode.
Why do we need the state maintenance exit?
If the user requested maintenance, so the maintenance request is in the database, and the user wants to exit maintenance immidiately e.g. for misclick, then we will not know if we should write running or idle to the database.
^ 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.