Fix `email_on_failure` with `render_template_as_native_obj` by jedcunningham · Pull Request #22770 · apache/airflow

jedcunningham · 2022-04-06T01:05:15Z

Co-authored-by: andyhuang [email protected]

Closes: #22152
Alternative of #22218, refactored to only change behavior when generating emails and including test coverage.

cc @andyfcx

uranusjr

Some nitpicks otherwise lgtm.

github-actions · 2022-04-06T05:55:08Z

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.

Co-authored-by: andyhuang <[email protected]>

Co-authored-by: Tzu-ping Chung <[email protected]>

dstandish · 2022-04-06T19:01:03Z


        else:
-            jinja_env = self.task.get_template_env()
+            jinja_env = self.task.get_template_env(force_sandboxed=True)


i think you may need to update this operator to handle kwargs

airflow/airflow/providers/microsoft/psrp/operators/psrp.py

Line 158 in be0a4e4

def get_template_env(self):

also, this illustrates a backcompat issue if a user implemented this method (since it's a signature change).

if we wanted to be real careful we could inspect the signature before calling....

wdyt?

Oh ugh that's going to make this approach hard/basically impossible. Nice catch @dstandish

I think I lean toward this being "internal" to the task, but I'm not sure how others view it.

We could inspect, but emails would still fail even then. I wonder if we should instead release a new psrp provider that has a minimum core of 2.3.0. I guess the most 'flexible', ignoring the email issue, is to inspect in both so new providers still work on old core and vice versa.

We could use dag.get_template_env(force_sandbox) instead of self in side the email rendering?

Oh, good idea, it uses dag anyways! Let me try that.

Yeah, that works 🍺

blag

Besides @dstandish's questions, LGTM.

Co-authored-by: Daniel Standish <[email protected]>

jedcunningham · 2022-04-06T21:13:54Z

        return self.task_id

-    def get_template_env(self) -> "jinja2.Environment":
+    def get_template_env(self, *, force_sandboxed: bool = False) -> "jinja2.Environment":


Think we should remove support for force_sandboxed here so we arent changing the signature of the method psrp is overriding?

Doesn't do any harm since its an optional arg, but for the sake of a smaller diff, yes we could remove it.

+1 to removing it since it’d make more obvious why we call the DAG’s get_template_env instead (so we can pass this option). It’d be nice to have a comment there describing why we don’t call the task’s get_template_env there.

I pushed a commit for this (plus a small tweak for Mypy).

Also added comments on why we use the DAG level implementation instead of the task level one.

…2770) Co-authored-by: andyhuang <[email protected]> Co-authored-by: Tzu-ping Chung <[email protected]>

vandanthaker · 2022-06-20T09:53:09Z

@jedcunningham
Any alternative to fix this on v2.2.5 without upgrading to 2.3.X?
Thanks in advance

potiuk · 2022-06-20T12:39:31Z

@jedcunningham Any alternative to fix this on v2.2.5 without upgrading to 2.3.X? Thanks in advance

As usual in open source - you can cherry-pick and apply it yourself. But we recommend to upgrade to 2.3 instead. Upgrading to latest released version is generally good thing to do.

potiuk · 2022-06-20T12:40:56Z

This is really the trade-off - either you take the burden of migration to what community maintain, or you take the maintenance burden yourself :). Choose one that is best for you.

vandanthaker · 2022-06-21T03:49:49Z

Agree :)

uranusjr · 2022-06-21T05:08:57Z

Note that you can always manually add the patch—this one is simple enough I believe a simple patch command should work.

dstandish · 2022-06-21T16:02:21Z

And here's an example of how we patched another PR in a dockerfile

RUN apt update && apt install -y patch patchutils

RUN set -ex; \
    curl -o /tmp/kpo.patch https://patch-diff.githubusercontent.com/raw/apache/airflow/pull/24117.patch; \
    cd /usr/local/lib/python3.9/site-packages/airflow; \
    filterdiff -p1 -i 'airflow*' /tmp/kpo.patch | patch -u -p 2; \
    rm /tmp/kpo.patch

A Dag with render_template_as_native_obj=True made email_on_failure and email_on_retry alerts fail to send: the alert subject and body were evaluated into Python objects, which the email backend cannot send, and the resulting error was swallowed by the notification error handler, so the alert vanished without any signal to the operator. This was fixed once in apache#22770 and regressed when the alert path moved to SmtpNotifier in apache#57354, which dropped the only caller of force_sandboxed. Only the subject and body are forced to strings; the recipient fields keep rendering the way the Dag asks for, so a templated address list still resolves to a real list.

jedcunningham requested review from XD-DENG, ashb and kaxil as code owners April 6, 2022 01:05

jedcunningham requested review from dstandish and uranusjr April 6, 2022 01:05

uranusjr approved these changes Apr 6, 2022

View reviewed changes

Comment thread airflow/models/abstractoperator.py Outdated

Comment thread airflow/models/dag.py Outdated

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

jedcunningham and others added 2 commits April 6, 2022 08:26

Fix email_on_failure with render_template_as_native_obj

9b145b9

Co-authored-by: andyhuang <[email protected]>

Apply suggestions from code review

073220f

Co-authored-by: Tzu-ping Chung <[email protected]>

jedcunningham force-pushed the template_native_obj branch from 9a1f125 to 073220f Compare April 6, 2022 14:26

jedcunningham closed this Apr 6, 2022

jedcunningham reopened this Apr 6, 2022

dstandish reviewed Apr 6, 2022

View reviewed changes

blag reviewed Apr 6, 2022

View reviewed changes

jedcunningham and others added 2 commits April 6, 2022 14:44

Update tests/models/test_dag.py

535df6b

Co-authored-by: Daniel Standish <[email protected]>

Use DAGs get_template_env instead of the tasks

91a52bd

ashb approved these changes Apr 6, 2022

View reviewed changes

jedcunningham commented Apr 6, 2022

View reviewed changes

dstandish approved these changes Apr 7, 2022

View reviewed changes

Fix DAG access error

905f171

Also added comments on why we use the DAG level implementation instead of the task level one.

ashb approved these changes Apr 7, 2022

View reviewed changes

ashb merged commit d80d52a into apache:main Apr 7, 2022

ashb deleted the template_native_obj branch April 7, 2022 08:48

ashb mentioned this pull request Apr 7, 2022

Do not use native obj Jinja2 env to render error emails #22218

Closed

ashb added this to the Airflow 2.3.0 milestone Apr 7, 2022

ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Apr 7, 2022

blag mentioned this pull request Apr 12, 2022

v2.2.5 Pre-release astronomer/airflow#1493

Merged

blag pushed a commit to astronomer/airflow that referenced this pull request Apr 13, 2022

Fix email_on_failure with render_template_as_native_obj (apache#2…

a37b057

…2770) Co-authored-by: andyhuang <[email protected]> Co-authored-by: Tzu-ping Chung <[email protected]>

jedcunningham added a commit to astronomer/airflow that referenced this pull request Apr 13, 2022

Fix email_on_failure with render_template_as_native_obj (apache#2…

06d8dfb

…2770) Co-authored-by: andyhuang <[email protected]> Co-authored-by: Tzu-ping Chung <[email protected]>

Eason09053360 mentioned this pull request Sep 16, 2026

Fix missing task alert emails on Dags using native template rendering #73228

Closed

1 task done

Conversation

jedcunningham commented Apr 6, 2022

Uh oh!

uranusjr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 6, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vandanthaker commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Jun 20, 2022

Uh oh!

potiuk commented Jun 20, 2022

Uh oh!

vandanthaker commented Jun 21, 2022

Uh oh!

uranusjr commented Jun 21, 2022

Uh oh!

dstandish commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vandanthaker commented Jun 20, 2022 •

edited

Loading