Add dag-processor cli command by mhenc · Pull Request #22305 · apache/airflow

mhenc · 2022-03-16T09:01:27Z

This change introduces new cli command: 'dag-processor' which runs DagProcessorManager as a standalone process.

Part of AIP-43
https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-43+DAG+Processor+separation

mhenc · 2022-03-16T09:01:40Z

potiuk

Looks good. But I sense some simplification possibility.

I understand why _standalone_mode is set as field in the manager, but it is in fact unnecessary and requires to make some mental correlation between "standalone_mode" and "signal_conn". I think the code will be a little cleaner if there is only "signal_conn" used.

Maybe name it differently to make it more obvious: direct_scheduler_conn or something ? This way the if's will be more obvious:

if not self._direct_scheduler_conn:

or

if self._direct_scheduler_conn:

potiuk

Looks nice. I think we need to add some documentation update (in the architecture mostly) to explain it but It can/should be done as separate PR.

potiuk · 2022-03-24T12:33:14Z

Since this is a core part of scheduler behaviour we definitely need another commiter's review. Anyone?

github-actions · 2022-03-24T12:33:17Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2022-03-24T12:33:51Z

BTW. It's been a bit easier than I thought it will be it seems :)

mhenc · 2022-03-25T08:59:52Z

I will update documentation after we run more tests - of course I ran my own tests, but I hope someone else verify it as well, before we add it to the public docs.

potiuk · 2022-03-29T14:19:44Z

Any more reviews anyone:)?

jedcunningham · 2022-03-30T14:28:28Z

Have we considered dag-parser instead of dag-processor? Or maybe even dags parser (like we have celery worker)?

mhenc · 2022-03-31T07:47:26Z

Not really, we proposed dag-processor as it just use DagProcessor* class so it shows the connection there.
E.g. the current documentation calls the component within the Scheduler as "Dag File Processor"
https://airflow.apache.org/docs/apache-airflow/stable/concepts/scheduler.html#dag-file-processing

but of course we can change it if we believe that fits better

cc: @potiuk thoughts?

potiuk · 2022-03-31T08:50:21Z

I think dags command is conceptualy different than celery as celery is really there to manage "celery workers". Dag is there mostly to manage "dags" and do some actions around it. Somehow dags parser or dags processor does not feel like belonging to the same group as dags list or dags test. Those are kind of dfferent "lifecycle".

I think the main reason "worker" is under "celery" is that the whole "celery" group is disabled when celery executor is not used. I'd say "dags processor/parser" is a "core" component - similar as scheduler, webserver, triggerer - especially when we go into more isolation mode, it deserves "top-level" entry.

I also thing "dags-parser" is worse than "dags-processor". One - because it introduces a new name, we already have DagProcessor and what it does is conceptually the same as the separated command. And Two - DagsProcessor does more than parsing - it Parses, Serializes, Stores the DAGS in the database but also executes Dag Callbacks. So by all means "dags-parser" does not do the justice to what tthis component does. The dags-processor is much more accurate.

I would think what's better "dags-processor" or "dag-processor" and I am not sure about plural vs. singular though.

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

ephraimbuddy

LGTM. +1 for dags-processor. I think it should standalone instead of being part of airflow dags.

Do we want to have this in 2.3?

potiuk · 2022-04-01T09:21:42Z

Do we want to have this in 2.3?

I think so. This is already an improvement in security and isolation. While it is still far from the the "true" multi-tenancy, it already gives some features of multitenancy.

This is still optional and without the DB isolation it does not give a lot of "security". but it already gives users interesting possibilities - for example if they have really a lot of DAGs to parse, but the do not want to run multiple schedulers - they could have a single or two schedulers and for example 10 dags-processors.

The additional security you get here is important. By employing DAG processor as separate process running in separate machine (or even separate security zone) you finally reach something that even some of scenarios where DAG writers could exercise too much power on scheduler. By adding DAG processor separation those scenarios are impossible and DAG writer is not able to execute any code on scheduler any more.

Additionally that will give the users an opportunity to isolate code executed by different groups of people. It does not secure a database access yet, but if you run separate DAG processors for separate subdirs with different "write" access, that already gives the user an option to run separate DAG processor in separate sub-folder you won't be able to execute a code on the same machine as "other group" of yours. Surely - you will be able to do in tasks, but if you use Kubernetes Executor, those are executed in separate PODs, so you will be able to already achive "complete code execution" isolation for multiple groups of people.

I think - Google wants to get it deployed anyway, so they might want to take alpha/beta/RC candidates of 2.3 and hammer it to make it really robust and tested, and the sooner that feature gets into hands of the users, the more prepared we will be for next steps of multi-tenancy introduction. It will give us a chance to iron out all the wrinkles in case we find them before we go deeper into "full" multi-tenancy.

potiuk · 2022-04-01T09:25:12Z

Of course we need a bit more documentation (but we can very quickly add the docs describing the use cases and architecture and benefits of it). I am happy to prepare some of that actually if we need to do it quickly - even for Alpha of 2.3.0.

Also looking at the "size" and complexity of this overal change, I think the risks connected with this change are rather small.

mik-laj · 2022-04-02T04:03:00Z

+            )
+            with ctx:
+                try:
+                    manager.register_exit_signals()


Should we also call.manager.start?

Right, I prepared a fix #22720
Thanks!

mik-laj · 2022-04-02T04:05:38Z

                self.waitables.pop(sentinel)
                self._processors.pop(processor.file_path)

+            if conf.getboolean("scheduler", "standalone_dag_processor"):


We should read configuration option values out of the loop because they are not cached.

Also addressed in #22720

potiuk · 2022-04-04T09:43:50Z

Good eyes @mik-laj !

+ move reading [scheduler]standalone_dag_processor outside of the loop See #22305 (comment)

+ move reading [scheduler]standalone_dag_processor outside of the loop See apache/airflow#22305 (comment) GitOrigin-RevId: 215993b75d0b3a568b01a29e063e5dcdb3b963e1

marclamberti · 2022-08-01T11:21:44Z

I love this feature!
Can we already run different dag-processors in different folders or not yet?
I tried to do that, but my dags keep getting replaced by the ones parsed by the latest dag-processor running

potiuk · 2022-08-01T18:52:21Z

I love this feature! Can we already run different dag-processors in different folders or not yet? I tried to do that, but my dags keep getting replaced by the ones parsed by the latest dag-processor running

Not yet. This is something @mhenc is plannig to add to complete AIP-43.

+ move reading [scheduler]standalone_dag_processor outside of the loop See apache/airflow#22305 (comment) GitOrigin-RevId: 215993b75d0b3a568b01a29e063e5dcdb3b963e1

mhenc requested review from XD-DENG, ashb, ephraimbuddy, jedcunningham and kaxil as code owners March 16, 2022 09:01

boring-cyborg Bot added area:CLI area:Scheduler including HA (high availability) scheduler labels Mar 16, 2022

potiuk reviewed Mar 22, 2022

View reviewed changes

mhenc requested a review from potiuk March 22, 2022 20:56

potiuk approved these changes Mar 24, 2022

View reviewed changes

github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 24, 2022

jedcunningham reviewed Mar 30, 2022

View reviewed changes

Comment thread airflow/dag_processing/manager.py Outdated

mhenc requested a review from jedcunningham March 30, 2022 06:56

mhenc and others added 5 commits April 1, 2022 09:30

Add dag-processor cli command

fbc87ab

Update airflow/jobs/scheduler_job.py

2b814f4

Rename DagProcessor _signal_conn field to _direct_scheduler_conn

07819de

fixes for scheduler_job.py

7549d2a

Update airflow/dag_processing/manager.py

362e773

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

ephraimbuddy reviewed Apr 1, 2022

View reviewed changes

ephraimbuddy approved these changes Apr 1, 2022

View reviewed changes

potiuk merged commit f5f11ae into apache:main Apr 1, 2022

mik-laj reviewed Apr 2, 2022

View reviewed changes

mhenc mentioned this pull request Apr 4, 2022

Fix starting standalone dag processor when running as a daemon #22720

Merged

mik-laj pushed a commit that referenced this pull request Apr 4, 2022

Fix starting dag processor when running as a daemon (#22720)

215993b

+ move reading [scheduler]standalone_dag_processor outside of the loop See #22305 (comment)

ephraimbuddy added the type:new-feature Changelog: New Features label Apr 8, 2022

mhenc deleted the standalone_dp branch July 4, 2022 12:04

ashb added this to the Airflow 2.3.0 milestone Aug 5, 2022

adwk67 mentioned this pull request Sep 29, 2022

[NEW VERSION] 2.4.1 stackabletech/airflow-operator#86

Closed

5 tasks

Uh oh!

Conversation

mhenc commented Mar 16, 2022

Uh oh!

mhenc commented Mar 16, 2022

Uh oh!

potiuk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk commented Mar 24, 2022

Uh oh!

github-actions Bot commented Mar 24, 2022

Uh oh!

potiuk commented Mar 24, 2022

Uh oh!

mhenc commented Mar 25, 2022

Uh oh!

potiuk commented Mar 29, 2022

Uh oh!

Uh oh!

jedcunningham commented Mar 30, 2022

Uh oh!

mhenc commented Mar 31, 2022

Uh oh!

potiuk commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ephraimbuddy left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk commented Apr 1, 2022

Uh oh!

potiuk commented Apr 1, 2022

Uh oh!

mik-laj Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

mhenc Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

mik-laj Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

mhenc Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

potiuk commented Apr 4, 2022

Uh oh!

marclamberti commented Aug 1, 2022

Uh oh!

potiuk commented Aug 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

potiuk left a comment •

edited

Loading

potiuk commented Mar 31, 2022 •

edited

Loading