Conversation
0455300 to
7e47779
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds deferrable execution support to the WinRMOperator to address issues where long-running remote Windows processes were causing TaskInstances to be prematurely killed by the scheduler. The refactoring introduces asynchronous polling via a new trigger, freeing workers from blocking on command completion.
Changes:
- Introduced
WinRMCommandOutputTriggerfor asynchronous polling of WinRM command output - Refactored
WinRMOperatorto support deferrable mode with newexecute_completecallback - Restructured
WinRMHookto separate command execution from output polling
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/winrm.py |
New trigger implementation for async command output polling |
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/operators/winrm.py |
Added deferrable mode support and refactored execute logic |
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/hooks/winrm.py |
Extracted command execution and output polling into separate methods |
providers/microsoft/winrm/tests/unit/microsoft/winrm/triggers/test_winrm.py |
Unit tests for the new trigger |
providers/microsoft/winrm/tests/unit/microsoft/winrm/operators/test_winrm.py |
Updated operator tests with deferrable mode coverage |
providers/microsoft/winrm/tests/unit/microsoft/winrm/hooks/test_winrm.py |
Simplified import statement |
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/get_provider_info.py |
Registered the new trigger module |
providers/microsoft/winrm/provider.yaml |
Added trigger configuration |
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/__init__.py |
New init file for triggers module |
providers/microsoft/winrm/tests/unit/microsoft/winrm/triggers/__init__.py |
New init file for trigger tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/hooks/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/operators/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/hooks/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/operators/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/winrm.py
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/operators/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/operators/winrm.py
Outdated
Show resolved
Hide resolved
|
@kaxil just have to check one more thing with triggerer |
24c92bf to
6566908
Compare
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/hooks/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/hooks/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/hooks/winrm.py
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/operators/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/hooks/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/hooks/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/operators/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/winrm.py
Outdated
Show resolved
Hide resolved
providers/microsoft/winrm/tests/unit/microsoft/winrm/operators/test_winrm.py
Outdated
Show resolved
Hide resolved
kaxil
left a comment
There was a problem hiding this comment.
Could you also squash your commits in one (or <5) please :) Currently t is 128 commits :D
Will do that once CI/CD is green again, thx for reviewing Kaxil :-) |
0bdc9b3 to
c0467c9
Compare
…m/triggers/winrm.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* refactor: Added deferrable support to WinRMOperator * Update providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/winrm.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* refactor: Added deferrable support to WinRMOperator * Update providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/winrm.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* refactor: Added deferrable support to WinRMOperator * Update providers/microsoft/winrm/src/airflow/providers/microsoft/winrm/triggers/winrm.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Was generative AI tooling used to co-author this PR?
Motivation
Recently, we encountered an issue where TaskInstances were being prematurely killed by the scheduler.
We initially tried the fix proposed in PR #60330 made by @ephraimbuddy, but unfortunately this did not help in our case. After further investigation, we discovered that this behaviour only occurred in DAGs using the WinRMOperator.
Problem
We use the WinRMOperator to launch remote processes on Windows servers. Some of these processes can take a significant amount of time to complete.
The root cause is that the WinRMOperator currently performs polling synchronously inside the worker, via the run method of WinRMHook. This has several drawbacks:
Overall, this is not an efficient or scalable execution model in Airflow.
Solution
This PR refactors the WinRMOperator to support deferrable execution.
When deferrable=True:
This aligns the WinRMOperator with Airflow’s recommended architecture for long-running or polling-based operations.
Benefits
Workers are no longer blocked by long-running WinRM commands.
Example Usage
Conclussion
With this refactoring in place, we no longer experience TaskInstances being prematurely killed. Polling is handled asynchronously by the triggerer, which is the preferred and more robust approach for this type of workload in Airflow and we don't block workers for polling unnecessarily.
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.