Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-3126] Add option to specify additional K8s volumes #7423

Closed
wants to merge 1 commit into from

Conversation

brandonwillard
Copy link
Contributor

@brandonwillard brandonwillard commented Feb 15, 2020

This PR introduces a new config option, kubernetes.extra_volume_mounts, that allows users to specify multiple Kubernetes volumes to be mounted in each generated worker pod.


Issue link: AIRFLOW-3126

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


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.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added area:Scheduler Scheduler or dag parsing Issues k8s labels Feb 15, 2020
@boring-cyborg boring-cyborg bot added the provider:google Google (including GCP) related issues label Feb 15, 2020
@brandonwillard brandonwillard force-pushed the k8s-extra-mounts branch 4 times, most recently from 1bff31e to 9ffcd34 Compare February 15, 2020 07:59
@codecov-io
Copy link

codecov-io commented Feb 15, 2020

Codecov Report

Merging #7423 into master will decrease coverage by 22.22%.
The diff coverage is 4.34%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #7423       +/-   ##
===========================================
- Coverage   86.97%   64.75%   -22.23%     
===========================================
  Files         927      927               
  Lines       44963    45009       +46     
===========================================
- Hits        39108    29145     -9963     
- Misses       5855    15864    +10009     
Impacted Files Coverage Δ
airflow/kubernetes/worker_configuration.py 16.88% <0.00%> (-82.43%) ⬇️
airflow/executors/kubernetes_executor.py 16.27% <5.55%> (-40.60%) ⬇️
airflow/hooks/S3_hook.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/hooks/pig_hook.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/hooks/hdfs_hook.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/hooks/http_hook.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/hooks/jdbc_hook.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/hooks/druid_hook.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/hooks/hive_hooks.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/hooks/mssql_hook.py 0.00% <0.00%> (-100.00%) ⬇️
... and 495 more

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 1982c3f...2aa2c86. Read the comment docs.

@brandonwillard
Copy link
Contributor Author

brandonwillard commented Feb 15, 2020

All right, I just checked locally (with breeze) and the uncovered sections of those unrelated MySql files are definitely being called by their respective tests, which are also passing. As well, the Travis log messages for those tests say they passed, and the warnings on Travis look the same as the local ones, so I'm not seeing a difference between local and Travis testing.

In other words, I don't know why CodeCov is saying coverage decreased in those untouched and unrelated files.

@potiuk
Copy link
Member

potiuk commented Feb 15, 2020

coverage is guideline and it really only works 100% with master builds. we have some optimisations on building times for PR builds (for example no kubernwtes tests are run if no kuberneyes files are touched). so do not worry about coverage decrease in unrelated files

@davlum
Copy link
Contributor

davlum commented Feb 25, 2020

I've noticed you've added several PRs recently to add missing functionality to the airflow.cfg for configuring the KubernetesExecutor. There is also a new field which allows you to pass the YAML directly if that helps at all.

@brandonwillard
Copy link
Contributor Author

brandonwillard commented Feb 25, 2020

@davlum, I've tried it and It did not appear to help—specifically, for default worker pod creation under the K8s Executor. It required too much specificity in the template and seemed to deactivate most other configuration options. Can you give an example of how it can be used to add volumes in exactly this way without sacrificing any of the existing functionality/configurability of worker pod creation?

Also, I don't see how it could possibly help with #7405.

@davlum
Copy link
Contributor

davlum commented Feb 26, 2020

If that field is set it won't use the other fields as indicated by the comment. You're right in that it definitely won't help with #7405, but I don't see why what you're trying to accomplish in this PR can't be done with that field. Do you have some example YAML that you want to create and we can test with that? Ftr I'm not invalidating this PR, I was just wondering if you had considered the option.

@brandonwillard
Copy link
Contributor Author

Sorry, I have to move this to my personal fork: #8150.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler Scheduler or dag parsing Issues provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants