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

docker-compose: fix the issue where celery broker was unable to push cfg #283

Closed
wants to merge 1 commit into from

Conversation

mtetreault
Copy link

Those changes should allow one to get started.

With those change, everything should work out of the box at the exception of the admin/site/sites thing that still needs to be configured.

The links have been removed because it's deprecated and is not required.
https://docs.docker.com/compose/compose-file/compose-file-v2/#links

@CLAassistant
Copy link

CLAassistant commented Jun 4, 2020

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

Codecov Report

Merging #283 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #283   +/-   ##
=======================================
  Coverage   46.76%   46.76%           
=======================================
  Files          58       58           
  Lines        2677     2677           
=======================================
  Hits         1252     1252           
  Misses       1425     1425           

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 c86bce2...d0691ad. Read the comment docs.

@mtetreault mtetreault closed this Jun 4, 2020
@mtetreault mtetreault reopened this Jun 4, 2020
@kfdm kfdm self-assigned this Jun 4, 2020
@kfdm kfdm self-requested a review June 4, 2020 23:51
Copy link
Collaborator

@kfdm kfdm left a comment

Choose a reason for hiding this comment

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

Interesting about links being depricated. I do not actually use docker-compose directly (this is meant more as a demo) so I have not followed the specific details of this request.

I made comments on a few things that I'm curious about, but it may be Monday before I'm able to test this for myself (It is currently Friday afternoon in Japan right now)

@@ -14,30 +14,28 @@ services:

web:
extends: base
command: web -b 0.0.0.0:8000
command: web -b 0.0.0.0:8000 --reload
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general --reload is really more for a development server so this should probably be removed.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, this will be removed.

- prometheus
- mysql
- redis
command: worker -l info --queues localhost,celery,prometheus,alertmanager
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typically the worker only needs to listen to 2 queues at max.
celery is the default queue for almost everything (notifications, etc) and there should be a single named queue for each Prometheus worker that matches is host name. In the case of the docker container it might just be prometheus so perhaps this command should be --queues prometheus,celery

Copy link
Author

Choose a reason for hiding this comment

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

Alright, it seems to work properly with:

command: worker -l info --queus prometheus,celery

links:
- alertmanager
- blackbox
user: "root"
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, I think it's probably best to run as a non-root user

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you that it should be runned as a non-root user.

The original issue is that prometheus couldn't create the required files&folders.

prometheus_1    | level=error ts=2020-06-05T13:30:24.569Z caller=query_logger.go:87 component=activeQueryTracker msg="Error opening query log file" file=/prometheus/queries.active err="open /prometheus/queries.active: permission denied"
prometheus_1    | panic: Unable to create mmap-ed active query log

This is due to the fact that by default the folders were created with the following user:groups

drwxr-xr-x 6 systemd-coredump root       4096 Jun  5 09:30 mysql_data
drwxr-xr-x 2 root             root       4096 Jun  5 09:30 prom_data

It looks like prometheus uses the nobody user https://github.com/prometheus/prometheus/blob/master/Dockerfile#L19

What do you think about using the user nobody as prometheus does?

The folder creation could then be done by an entrypoint script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using the same prometheus user (nobody) should be fine.

Comment on lines +33 to +38
command:
- --config.file=/etc/prometheus/prometheus.yml
- --storage.tsdb.path=/prometheus
- --web.console.libraries=/usr/share/prometheus/console_libraries
- --web.console.templates=/usr/share/prometheus/consoles
- --web.enable-lifecycle
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think all the defaults should be ok, so I'm unsure the reason to add them here.

Copy link
Author

Choose a reason for hiding this comment

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

The command option is overridden to add the --web.enable-lifecycle which is required to allow prometheus to reload its configuration.

Prometheus can reload its configuration at runtime. If the new configuration is not well-formed, the changes will not be applied. A configuration reload is triggered by sending a SIGHUP to the Prometheus process or sending a HTTP POST request to the /-/reload endpoint (when the --web.enable-lifecycle flag is enabled). This will also reload any configured rule files

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good catch then. I suppose I need to test more carefully then because I never noticed this before (in production I do not deploy with docker so I've only tested the web part in docker)

@@ -47,14 +45,14 @@ services:
blackbox:
image: prom/blackbox-exporter
network_mode: bridge
ports:
- "9115:9115"
Copy link
Collaborator

Choose a reason for hiding this comment

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

blackbox_exporter is typically not accessed outside of the cluster, so normally the ports wouldn't need to be exported.

Copy link
Author

Choose a reason for hiding this comment

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

I don't have a strong opinion on this point. I found it useful to have access to the page, mainly during testing, this can be removed.

@kfdm kfdm mentioned this pull request Aug 10, 2022
@kfdm
Copy link
Collaborator

kfdm commented Aug 10, 2022

Closing in favor of #402

@kfdm kfdm closed this Aug 10, 2022
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.

4 participants