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

Expose Airflow Webserver Port in Production Docker Image #8228

Merged

Conversation

feluelle
Copy link
Member

@feluelle feluelle commented Apr 9, 2020

The production docker image currently does not expose a port which is needed for Airflow's webserver to be accessible from the host. This PR exposes a port and the breeze script will pass the port configured in breeze env.


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


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.

@feluelle
Copy link
Member Author

feluelle commented Apr 9, 2020

@potiuk Is it correct that we should map the port like I did in every backend compose file?

scripts/ci/_utils.sh Outdated Show resolved Hide resolved
scripts/ci/_utils.sh Outdated Show resolved Hide resolved
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Few comments :)

Dockerfile Outdated Show resolved Hide resolved
scripts/ci/_utils.sh Outdated Show resolved Hide resolved
scripts/ci/_utils.sh Outdated Show resolved Hide resolved
scripts/ci/docker-compose/backend-mysql.yml Outdated Show resolved Hide resolved
@feluelle feluelle force-pushed the feature/productionimage-expose-webserver-port branch from 74f861a to 1cffa75 Compare April 11, 2020 09:26
@feluelle feluelle requested review from ashb and dimberman April 11, 2020 13:40
@feluelle feluelle force-pushed the feature/productionimage-expose-webserver-port branch from 1cffa75 to 3ecca89 Compare April 14, 2020 08:05
@feluelle
Copy link
Member Author

Ready for re-review @potiuk

@potiuk
Copy link
Member

potiuk commented Apr 14, 2020

Looks Great!

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Hey @feluelle -> i think that was the reason from #8229

@feluelle
Copy link
Member Author

feluelle commented Apr 14, 2020

I also had an issue with volumes being mounted GETting on /home:

Traceback (most recent call last):
  File "/home/airflow/.local/lib/python3.7/site-packages/flask/app.py", line 2446, in wsgi_app
    response = self.full_dispatch_request()
  File "/home/airflow/.local/lib/python3.7/site-packages/flask/app.py", line 1951, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/home/airflow/.local/lib/python3.7/site-packages/flask/app.py", line 1820, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/home/airflow/.local/lib/python3.7/site-packages/flask/_compat.py", line 39, in reraise
    raise value
  File "/home/airflow/.local/lib/python3.7/site-packages/flask/app.py", line 1949, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/airflow/.local/lib/python3.7/site-packages/flask/app.py", line 1935, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/airflow/.local/lib/python3.7/site-packages/flask_appbuilder/security/decorators.py", line 109, in wraps
    return f(self, *args, **kwargs)
  File "/opt/airflow/airflow/www/views.py", line 355, in index
    status_count_paused=status_count_paused)
  File "/opt/airflow/airflow/www/views.py", line 190, in render_template
    **kwargs
  File "/home/airflow/.local/lib/python3.7/site-packages/flask_appbuilder/baseviews.py", line 281, in render_template
    template, **dict(list(kwargs.items()) + list(self.extra_args.items()))
  File "/home/airflow/.local/lib/python3.7/site-packages/flask/templating.py", line 140, in render_template
    ctx.app,
  File "/home/airflow/.local/lib/python3.7/site-packages/flask/templating.py", line 120, in _render
    rv = template.render(context)
  File "/home/airflow/.local/lib/python3.7/site-packages/jinja2/asyncsupport.py", line 76, in render
    return original_render(self, *args, **kwargs)
  File "/home/airflow/.local/lib/python3.7/site-packages/jinja2/environment.py", line 1008, in render
    return self.environment.handle_exception(exc_info, True)
  File "/home/airflow/.local/lib/python3.7/site-packages/jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/home/airflow/.local/lib/python3.7/site-packages/jinja2/_compat.py", line 37, in reraise
    raise value.with_traceback(tb)
  File "/opt/airflow/airflow/www/templates/airflow/dags.html", line 20, in top-level template code
    {% extends base_template %}
  File "/home/airflow/.local/lib/python3.7/site-packages/jinja2/environment.py", line 1005, in render
    return concat(self.root_render_func(self.new_context(vars)))
  File "/opt/airflow/airflow/www/templates/airflow/dags.html", line 17, in root
    under the License.
  File "/opt/airflow/airflow/www/templates/airflow/master.html", line 16, in root
    specific language governing permissions and limitations
  File "/home/airflow/.local/lib/python3.7/site-packages/flask_appbuilder/templates/appbuilder/baselayout.html", line 17, in root
    {% include 'appbuilder/flash.html' %}
  File "/home/airflow/.local/lib/python3.7/site-packages/flask_appbuilder/templates/appbuilder/init.html", line 32, in root
    <link href="{{url_for('appbuilder.static',filename='select2/select2.css')}}" rel="stylesheet">
  File "/home/airflow/.local/lib/python3.7/site-packages/flask_appbuilder/templates/appbuilder/baselayout.html", line 37, in block_body
  File "/opt/airflow/airflow/www/templates/airflow/dags.html", line 106, in block_content
    </td>
TypeError: can only concatenate str (not "NoneType") to str

@feluelle feluelle force-pushed the feature/productionimage-expose-webserver-port branch from 3ecca89 to 0ad4ac4 Compare April 14, 2020 11:34
@feluelle
Copy link
Member Author

feluelle commented Apr 14, 2020

we should not map the: "airflow, common, dags, requirements, scripts" in production image. They might be totally misleading and can have side effects if /opt/airflow is on the PYTHONPATH

Yes, without it - it works. /version and /home 👍

potiuk
potiuk previously approved these changes Apr 14, 2020
@feluelle
Copy link
Member Author

But the skip mounting flag is misleading when we also map ports, isn't it? You won't think that when you do not mount you will also disable port mapping in prod image.

@potiuk
Copy link
Member

potiuk commented Apr 14, 2020

Indeed. I think we should move ports to "base.yaml". I can't think of any reason why it would be harm.

@feluelle
Copy link
Member Author

feluelle commented Apr 14, 2020

But then you won't be able to test prod image locally while having airflow ci running, because both are using the same port WEBSERVER_HOST_PORT

EDIT:

Error response from daemon: driver failed programming external connectivity on endpoint docker-compose_airflow_run_c577066264a5 (c0f164193ce289b5af71856fca35aef987f96e7d34f2109ec9426d4ed480c052): Bind for 0.0.0.0:28080 failed: port is already allocated

@potiuk
Copy link
Member

potiuk commented Apr 14, 2020

I think there are more reasons why you cannot run both prod/ci in parallel - they use the same docker-compose configuration and they are both named "airflow" so I would not worry about that. I think you should always stop the CI Breeze before starting Prod one. We might add some better diagnosis/explanation of the case rather than "port listening" error.

@feluelle
Copy link
Member Author

feluelle commented Apr 14, 2020

Yes that is true. It is not only that - both are using the same database if for example both (by default) have backend mysql or postgres. (which is configured in the compose file :D)

@feluelle
Copy link
Member Author

Should I add docs regarding that in this PR? Just a note about what you said not using both at the same time?

@potiuk
Copy link
Member

potiuk commented Apr 14, 2020

Should I add docs regarding that in this PR? Just a note about what you said not using both at the same time?

Absolutely :)

@feluelle feluelle force-pushed the feature/productionimage-expose-webserver-port branch from 0ad4ac4 to 116279a Compare April 14, 2020 12:26
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Just one comment for the example - it's not valid right now as you might use different constraint requirements than the ones matching the version you want to build.

@codecov-io
Copy link

codecov-io commented Apr 14, 2020

Codecov Report

Merging #8228 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8228      +/-   ##
==========================================
- Coverage   88.43%   88.42%   -0.01%     
==========================================
  Files         940      940              
  Lines       45353    45355       +2     
==========================================
- Hits        40108    40106       -2     
- Misses       5245     5249       +4     
Impacted Files Coverage Δ
airflow/jobs/backfill_job.py 90.52% <0.00%> (-1.12%) ⬇️
...oviders/google/cloud/utils/credentials_provider.py 94.11% <0.00%> (+0.14%) ⬆️

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 eee4eba...b4ecb42. Read the comment docs.

@potiuk potiuk dismissed their stale review April 14, 2020 13:58

The example change needs to be reverted

@feluelle feluelle force-pushed the feature/productionimage-expose-webserver-port branch from 116279a to b4ecb42 Compare April 14, 2020 14:31
@feluelle
Copy link
Member Author

@potiuk can you check again, please?

@potiuk
Copy link
Member

potiuk commented Apr 14, 2020

Perfect! Thanks @feluelle ! Let's wait for Travis and I am super happy to merge it :)

@feluelle
Copy link
Member Author

Are we good to go @potiuk ? :)

@potiuk potiuk merged commit cf6c254 into apache:master Apr 15, 2020
@potiuk
Copy link
Member

potiuk commented Apr 15, 2020

Go! Thanks @feluelle !

@feluelle feluelle deleted the feature/productionimage-expose-webserver-port branch April 30, 2020 07:08
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants