Allow Airflow UI to create worker pod via Clear > Run

[kimyen]

Problem:

Clear and Run task instance via UI cause mushroom cloud error.

Repro steps:

• Using Helm 1.1.0 to deploy Airflow 1.10.15
• From the UI, navigate to a DAG tree view
• Select an existing task (small square on tree view) > Clear > Run

Error:

-------------------------------------------------------------------------------
Node: airflow-webserver-67bf9669bc-x6wcc
-------------------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/flask/app.py", line 2446, in wsgi_app
    response = self.full_dispatch_request()
  ...
  File "/usr/local/lib/python3.7/site-packages/airflow/www/views.py", line 1196, in run
    executor.start()
  File "/usr/local/lib/python3.7/site-packages/airflow/executors/kubernetes_executor.py", line 784, in start
    self.kube_client, self.worker_uuid
  File "/usr/local/lib/python3.7/site-packages/airflow/executors/kubernetes_executor.py", line 396, in __init__
    self.worker_configuration_pod = WorkerConfiguration(kube_config=self.kube_config).as_pod()
  File "/usr/local/lib/python3.7/site-packages/airflow/kubernetes/worker_configuration.py", line 437, in as_pod
    return PodGenerator(pod_template_file=self.kube_config.pod_template_file).gen_pod()
  File "/usr/local/lib/python3.7/site-packages/airflow/kubernetes/pod_generator.py", line 308, in gen_pod
    result.metadata.name = self.make_unique_pod_id(result.metadata.name)
AttributeError: 'NoneType' object has no attribute 'name'

How does this PR fix the problem above:

• From the stack trace above, this line here shows that the name is read from the kube_config.pod_template_file:

  airflow/airflow/kubernetes/worker_configuration.py

  Line 437 in db72074

  return PodGenerator(pod_template_file=self.kube_config.pod_template_file).gen_pod()

• In the Airflow Helm chart 1.1.0, only the scheduler has the pod_template_file mounted. So when this action was triggered, the webserver does not have the pod_template file mounted and couldn't fill in the values here.
• This PR patches the webserver K8s template to also mount the pod_template_file the same way the schedule does here:

  airflow/chart/templates/scheduler/scheduler-deployment.yaml

  Line 141 in daa725b

  mountPath: {{ include "airflow_pod_template_file" . }}/pod_template_file.yaml

  so that when the code path is exercised, name is filled in from the value read in the pod template file.

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

[boring-cyborg]

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

• Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
• In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
• Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
  Apache Airflow is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: dev@airflow.apache.org
  Slack: https://s.apache.org/airflow-slack

[kimyen]

One of the unit test failed:

==== Helm sqlite: 2 failures ====

chart/tests/test_webserver.py::WebserverDeploymentTest::test_logs_persistence_adds_volume_and_mount_1: AssertionError: assert {'mountPath':...name': 'logs'} == {'mountPath':...'airflow.cfg'}
  Differing items:
  {'name': 'logs'} != {'name': 'config'}
  {'mountPath': '/opt/airflow/logs'} != {'mountPath': '/opt/airflow/airflow.cfg'}
  Right contains 2 more items:
  {'readOnly': True, 'subPath': 'airflow.cfg'}
  Use -v to get the full diff
chart/tests/test_webserver.py::WebserverDeploymentTest::test_logs_persistence_adds_volume_and_mount_2: AssertionError: assert {'mountPath':...name': 'logs'} == {'mountPath':...'airflow.cfg'}
  Differing items:
  {'name': 'logs'} != {'name': 'config'}
  {'mountPath': '/opt/airflow/logs'} != {'mountPath': '/opt/airflow/airflow.cfg'}
  Right contains 2 more items:
  {'readOnly': True, 'subPath': 'airflow.cfg'}
  Use -v to get the full diff
Error: Process completed with exit code 1.

I want to challenge the value of this test. It doesn't seem to catch anything that would break the webserver deployment, but rather being picky about the order of the volume. 😞

I would expect to break a test that has a set of volumes that the webserver deployment should have, and with this PR, extend the test to add the new volume. I don't know enough to write such a test. What do you think about the proposal? Does such test already exist (if so, it didn't fail)? Can I get some pointers on how to write the test I propose?

[jedcunningham]

@kimyen, that test is overly fragile as written. Easy fix though: #18332

I don't think that test exists yet, and I agree it would be a valuable test. The broken test is a reasonable example to follow, just check the default volumes and volumeMounts instead. More than 1 way to approach this, but one would be to use parameterized.expand like so:

@parameterized.expand([
    ("1.10.10", False),
    ("1.10.12", True),
    ("2.1.0", True),
])
def test_default_volumes_and_mounts(self, af_version, pod_template_file_expected):
    # assert "always there" volumes/mounts
    if pod_template_file_expected:
        # assert pod_template_file mount   

[kimyen]

@jedcunningham I would love your feedback on the test that I added. Other volumes depends on other values, so I limit the scope of my test on default and optional config volumes.

I don't have an option to add tags to this PR, but would like to mark it as Ready for Review.

[jedcunningham]

@kimyen I'd also suggest that you set up the pre-commit hooks, they help catch these static check issues locally 👍.

[kimyen]

[potiuk]

REopened to rebuild @kimyen -> Next time you can simply rebase + "--force-push-with-lease" if something like that happens (which might be because of various CI issues)

[kimyen]

Thanks @potiuk !

The docker image build still failed. But the error is not related to this change:

  ERROR: Cannot install apache-airflow[devel-ci]==2.2.0.dev0 because these package versions have conflicting dependencies.

It looks like the fix is in package dependencies for 2.2.0.dev0.

[potiuk]

Looks like all tests passed now!

[kimyen]

@jedcunningham would you mind reviewing this again? Thank you!

[github-actions]

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

[jedcunningham]

(Closing/reopening to rerun CI as it's been a little while)

[boring-cyborg]

Awesome work, congrats on your first merged pull request!

[jedcunningham]

@kimyen, congrats on your first commit 🎉! (sorry about the delay in getting this merged)