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

Add consul_config template and use it for docker deployment environments #284

Merged
merged 14 commits into from Jul 19, 2018
Merged

Add consul_config template and use it for docker deployment environments #284

merged 14 commits into from Jul 19, 2018

Conversation

@paramsingh
Copy link
Member

@paramsingh paramsingh commented Jul 13, 2018

Also, this PR moves back to the single config.py model.

We encountered many problems when using multiple config files in LB ranging from different config values in different parts of the code to race conditions and so on. We decided that moving to a single config file would be better overall.

@paramsingh
Copy link
Member Author

@paramsingh paramsingh commented Jul 13, 2018

@brainzbot retest this please

Loading

@paramsingh paramsingh changed the title Add consul_config template and use it for docker deployment environments (WIP): Add consul_config template and use it for docker deployment environments Jul 13, 2018
@paramsingh
Copy link
Member Author

@paramsingh paramsingh commented Jul 13, 2018

Jenkins is kinda fragile, I've been wrestling for a few hours with this, taking a break, I'll fix soon.

Loading

@paramsingh
Copy link
Member Author

@paramsingh paramsingh commented Jul 13, 2018

@brainzbot retest this please

Loading

1 similar comment
@paramsingh
Copy link
Member Author

@paramsingh paramsingh commented Jul 13, 2018

@brainzbot retest this please

Loading

@paramsingh paramsingh changed the title (WIP): Add consul_config template and use it for docker deployment environments Add consul_config template and use it for docker deployment environments Jul 13, 2018
@paramsingh paramsingh requested review from alastair and mayhem Jul 13, 2018
DEBUG = False
SECRET_KEY = '''{{template "KEY" "secret_key"}}'''

{{if service "pgbouncer-master"}}
Copy link
Member Author

@paramsingh paramsingh Jul 13, 2018

Choose a reason for hiding this comment

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

this will need to change to the new postgres service

Loading

@paramsingh
Copy link
Member Author

@paramsingh paramsingh commented Jul 14, 2018

Loading

Copy link
Member

@mayhem mayhem left a comment

On the right track!

Loading

{{end}}
{{end}}

{{if service "acousticbrainz-redis"}}
Copy link
Member

@mayhem mayhem Jul 18, 2018

Choose a reason for hiding this comment

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

AB's use of redis is not very great -- let's not duplicate this and use the main metabrainz redis since that is on track for no longer being a single point of failure.

Loading

Copy link
Member Author

@paramsingh paramsingh Jul 18, 2018

Choose a reason for hiding this comment

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

Loading

Copy link
Member

@mayhem mayhem Jul 18, 2018

Choose a reason for hiding this comment

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

LOL. The same one that LB uses, should be fine.

Loading

'custom_config.py'
), silent=True)
config_file = os.path.join(os.path.dirname(os.path.realpath(__file__)), "..", "config.py")
print(config_file)
Copy link
Member

@mayhem mayhem Jul 18, 2018

Choose a reason for hiding this comment

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

Not sure if we need this now since there is only one config file.

Loading

Copy link
Member

@mayhem mayhem Jul 18, 2018

Choose a reason for hiding this comment

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

For the MeB setup, I actually generate the consul config to consul_config.py and refuse to even look at config.py should it miraculously appear in the repo. (I've also added config.py to .dockerignore) This makes it extra difficult to get the wrong config file... Perhaps even impossible.

Loading

@@ -47,7 +64,7 @@ def create_app(debug=None, config_path=None):

# Logging
from webserver.loggers import init_loggers
init_loggers(app)
init_loggers(app) # TODO: use CustomFlask.init_loggers here instead
Copy link
Member

@mayhem mayhem Jul 18, 2018

Choose a reason for hiding this comment

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

At some point we should add support for setting the log level via the config file.

Loading

Copy link
Member Author

@paramsingh paramsingh Jul 19, 2018

Choose a reason for hiding this comment

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

Loading

README.md Outdated
@@ -13,7 +13,7 @@ Please report issues here: http://tickets.musicbrainz.org/browse/AB
You can use [docker](https://www.docker.com/) and [docker-compose](https://docs.docker.com/compose/) to run the AcousticBrainz server. Make sure docker and docker-compose are installed.
Then copy two config files:

1. `custom_config.py.example` to `custom_config.py` *(you don't need to modify this file)*
1. `config.py.example` to `config.py` *(you don't need to modify this file)*
Copy link
Contributor

@alastair alastair Jul 18, 2018

Choose a reason for hiding this comment

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

Now we do have to modify this file, right?

Loading

Copy link
Member Author

@paramsingh paramsingh Jul 19, 2018

Choose a reason for hiding this comment

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

Well, to be honest, we had to modify it earlier also. I'll fix it.

Loading

# Set to True if Less should be compiled in browser. Set to False if styling is pre-compiled.
COMPILE_LESS = False

LOG_FILE_ENABLED = True
Copy link
Contributor

@alastair alastair Jul 18, 2018

Choose a reason for hiding this comment

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

Does file logging make sense if we don't have it exposed as an external volme? Perhaps we should log to stdout instead - confirm with @zas

Loading

Copy link
Member

@mayhem mayhem Jul 18, 2018

Choose a reason for hiding this comment

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

That could also be cargo culting. :)

Loading

DATASET_DIR = '''{{template "KEY" "dataset_dir"}}'''
FILE_STORAGE_DIR = '''{{template "KEY" "file_storage_dir"}}'''

FEATURE_EVAL_LOCATION = False
Copy link
Contributor

@alastair alastair Jul 18, 2018

Choose a reason for hiding this comment

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

TODO?

Loading

Copy link
Contributor

@alastair alastair Jul 18, 2018

Choose a reason for hiding this comment

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

sorry, my mistake. the config option looks like it should be a path, based on its name, but it's a boolean

Loading

if not os.path.exists(config_file):
time.sleep(1)

if not os.path.exists(config_file):
Copy link
Contributor

@alastair alastair Jul 18, 2018

Choose a reason for hiding this comment

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

as I understand, this loop is to wait for the config file to be created by consul, right? Out of interest, is there a reason why we're not using something like dockerize to wait for the file before launching flask? (https://github.com/jwilder/dockerize#waiting-for-other-dependencies)
The advantage of this is that we don't have this weird deploy-level logic inside a configuration/setup method

Loading

Copy link
Member

@mayhem mayhem Jul 19, 2018

Choose a reason for hiding this comment

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

Upon sleeping on it, I prefer to keep this as python code, complete with access for easy Sentry reporting in case shit goes wrong. Dockerize is good for a few things, namely ensuring the correct start-up order of containers. But I feel that this situation needs more nuance and more reporting.

Loading

root_path,
'custom_config.py'
), silent=True)
config_file = os.path.join(os.path.dirname(os.path.realpath(__file__)), "..", "config.py")
Copy link
Contributor

@alastair alastair Jul 18, 2018

Choose a reason for hiding this comment

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

what's the config file in production, and how is it loaded? I don't see any app.config.from_* that loads consul_config.py, which I understand is the name of the production config file.

Loading

Copy link
Contributor

@alastair alastair Jul 18, 2018

Choose a reason for hiding this comment

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

got it, the consul config file reads from consul_config.py.ctmpl and writes to config.py

Loading

{{end}}
{{end}}

{{if service "listenbrainz-redis"}}
Copy link
Member Author

@paramsingh paramsingh Jul 19, 2018

Choose a reason for hiding this comment

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

should probably consider renaming this then :P

Loading

Copy link
Member

@mayhem mayhem Jul 19, 2018

Choose a reason for hiding this comment

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

Ah, yes. Well, it used to be standalone until we moved to production. We should've renamed it then...

Loading

Copy link
Member

@mayhem mayhem left a comment

A couple of minor tweaks and this looks good to go.

Loading

@@ -0,0 +1,5 @@
template {
source = "/code/acousticbrainz/consul_config.py.ctmpl"
destination = "/code/acousticbrainz/consul_config.py"
Copy link
Member

@mayhem mayhem Jul 19, 2018

Choose a reason for hiding this comment

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

👍

Loading

if not os.path.exists(config_file):
time.sleep(1)

if not os.path.exists(config_file):
Copy link
Member

@mayhem mayhem Jul 19, 2018

Choose a reason for hiding this comment

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

This should be consul_config, no?

Loading

app.config.from_pyfile(consul_config)
return app

config_file = os.path.join(os.path.dirname(os.path.realpath(__file__)), "..", "config.py")
Copy link
Member

@mayhem mayhem Jul 19, 2018

Choose a reason for hiding this comment

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

I think for completeness sake we should add another print statement logging which config file was read as part of this process.

Loading

mayhem
mayhem approved these changes Jul 19, 2018
Copy link
Member

@mayhem mayhem left a comment

⚙️ 🔨 😷 !!

Loading

@paramsingh
Copy link
Member Author

@paramsingh paramsingh commented Jul 19, 2018

don't merge yet, I'll do it, there's something going wrong still.

Loading

@mayhem
Copy link
Member

@mayhem mayhem commented Jul 19, 2018

These things need to be carefully merged, so I try to leave them to you to merge at the right time.

Loading

mayhem
mayhem approved these changes Jul 19, 2018
Copy link
Member

@mayhem mayhem left a comment

LOL

Loading

@paramsingh paramsingh merged commit f77223b into metabrainz:master Jul 19, 2018
1 check passed
Loading

if config_path:
app.config.from_pyfile(config_path)
app.config.from_pyfile(config_path, silent=True)
Copy link
Contributor

@alastair alastair Aug 23, 2018

Choose a reason for hiding this comment

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

This has some awkward behaviour which is causing me some problems when running tests:

  • the config_path argument was to specify a separate config file for testing. If we no longer call this method with the parameter then we should remove it
  • In ListenBrainz you rename consul_config.py.tmpl to config.py (https://github.com/metabrainz/listenbrainz-server/blob/8d12052fc15c6a0a9349b51a9f53e7f1020b4485/docker/consul-template.conf#L3), but in AcousticBrainz you rename it to consul_config.py, which is inconsistent
  • config.py is loaded unconditionally, which will probably cause a server startup error on production, as the config file is consul_config.py
  • If config.py does exist on production, this will overwrite the production config
  • Unconditionally prints unneeded messages, especially during development and testing. In tests, I see the entire config printed to screen every time I create an app (each test), instead of the normal pytest .....F..E.
  • It tires to pretty-print the config (for readability?), but it doesn't work - when running tests I see the config all on one line: <Config {'JSON_AS_ASCII': True, 'FEATURE_EVAL_LOCATION': False, 'SESSION_REFRESH_EACH_REQUEST': True, 'DATASET_DIR': '/data/datasets', 'SENTRY_DSN': '' ... [lots more chopped]...>

Loading

Copy link
Contributor

@alastair alastair Aug 23, 2018

Choose a reason for hiding this comment

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

Action points:

  • remove config_path argument
  • Remove create_app_with_configuration method, which is no longer used independently
  • standardaise on consul_config.py or config.py in both apps
  • only load config.py during development
  • only use print in production
  • correctly pretty-print config in production

Loading

Copy link
Member Author

@paramsingh paramsingh Nov 16, 2018

Choose a reason for hiding this comment

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

Commenting for posterity: #294, was reading this PR and this conversation looked unresolved.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants