Skip to content

[AIRFLOW-1985] Changes to propagate config in subdags#2929

Closed
edgarRd wants to merge 1 commit into
apache:masterfrom
edgarRd:erod-subdag-impersonation
Closed

[AIRFLOW-1985] Changes to propagate config in subdags#2929
edgarRd wants to merge 1 commit into
apache:masterfrom
edgarRd:erod-subdag-impersonation

Conversation

@edgarRd
Copy link
Copy Markdown
Contributor

@edgarRd edgarRd commented Jan 10, 2018

Make sure you have checked all steps below.

JIRA

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Propagating configuration within subdags when using impersonation to
guarantee correct behavior of tasks.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Added relevant tests.

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":

    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"
  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

@aoen PTAL

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 10, 2018

Codecov Report

Merging #2929 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2929      +/-   ##
==========================================
+ Coverage   73.41%   73.44%   +0.03%     
==========================================
  Files         160      161       +1     
  Lines       12202    12222      +20     
==========================================
+ Hits         8958     8977      +19     
- Misses       3244     3245       +1
Impacted Files Coverage Δ
airflow/models.py 86.95% <ø> (-0.05%) ⬇️
airflow/executors/base_executor.py 95.16% <100%> (ø) ⬆️
airflow/utils/configuration.py 100% <100%> (ø)
airflow/jobs.py 79.51% <100%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 804710f...4ac32e7. Read the comment docs.

Comment thread airflow/operators/subdag_operator.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to put this in execute itself? That way you can open the file using a "with" instead which is a bit safer in terms of automatic cleanup.

Comment thread airflow/executors/base_executor.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add an explanation why cfg_path is needed and a JIRA for the appropriate fix as discussed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

Comment thread airflow/operators/subdag_operator.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why copy a subset instead of the whole thing?

@edgarRd edgarRd force-pushed the erod-subdag-impersonation branch from 9feea7c to 01340ea Compare January 11, 2018 00:59
@edgarRd
Copy link
Copy Markdown
Contributor Author

edgarRd commented Jan 11, 2018

Addressed comments, @aoen PTAL

@edgarRd edgarRd force-pushed the erod-subdag-impersonation branch 2 times, most recently from 6c65fd6 to 57e1a0a Compare January 11, 2018 02:08
@aoen
Copy link
Copy Markdown
Contributor

aoen commented Jan 11, 2018

@edgarRd there was one more comment, Why copy a subset of the config instead of the whole thing (trying to reduce the complexity of this change since it's a work-around)?

@edgarRd edgarRd force-pushed the erod-subdag-impersonation branch 3 times, most recently from 71f5728 to ce862eb Compare January 11, 2018 09:27
@edgarRd
Copy link
Copy Markdown
Contributor Author

edgarRd commented Jan 11, 2018

I tried initially copying the whole configuration settings but it's not possible given that sometimes configuration elements come from env variables, such as testsection when running tests, and these can't be serialized since they could be arbitrary. At the end, you still need some sort of logic to rule these parts out.

I did reduce the complexity of this change by always copying the configuration sections when running within the context of the subdag operator (using a BackfillJob) - the generated configuration files are always removed by the airflow run in cli.py#L343.

Propagating configuration within subdags when using impersonation to
guarantee correct behavior of tasks.
@edgarRd edgarRd force-pushed the erod-subdag-impersonation branch from ce862eb to 4ac32e7 Compare January 11, 2018 18:08
@edgarRd
Copy link
Copy Markdown
Contributor Author

edgarRd commented Feb 9, 2018

This changes were already merged in #2991. Closing.

@edgarRd edgarRd closed this Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants