-
Notifications
You must be signed in to change notification settings - Fork 310
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
[IMP] Add some more default configurations #62
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have conflicts.
Please @pedrobaeza review this. Do you think it's worth it, or is it better to have those predefined flavors we talked once?
We should have both: the possibility to define these environment variables (not for us, but for @lasley that needs them), and default sane values. Please rebase this PR and add these defaults. |
Dockerfile
Outdated
ONBUILD ARG ODOO_LIMIT_TIME_REAL_CRON=1 | ||
ONBUILD ARG ODOO_DB_FILTER= | ||
ONBUILD ARG ODOO_LIST_DB=1 | ||
ONBUILD ARG ODOO_LOG_LEVEL=info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrobaeza You can see the defaults he's adding here.
The problem is that for performance there are no sane defaults for everyone. If you apply a 32 core's sane defaults to a 4 core machine, it would get exhausted.
My original idea was to have some sane disabled defaults in config files that you could symlink in your custom/conf.d
directory.
Imagine something like /opt/odoo/common/disabled-conf.d/
with files like:
- 1vcpu.conf
- 2vcpu.conf
- 4vcpu.conf
- 8vcpu.conf (etc.)
- 4gbram.conf
- 8gbram.conf
- 32gbram.conf (etc.)
Similar to apache's sites-available
vs sites-enabled
.
So you can add those with ln -s /opt/odoo/common/disabled-conf.d/{8vcpu,32gbram}.conf custom/conf.d
and have a community-maintained sane default for your machine. Or we could use an env var for that if it seems more convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO because the variety of uses for this image, we shouldn't make too many assumptions about the deploy by default.
Basically all I did here was expose some more configuration options, using specifically the ones Odoo sets as default.
I think the better configuration would just be a part of the env files in scaffolds though:
odoo:
env_file:
- common.env
- 4vcpu.env
- 32gbram.env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the above obviously wouldn't work if the configuration is being generated during build, but this also highlights why it should be in the endpoint instead. It seems pretty weird to statically define infrastructure capacity in a Docker image IMO
Dockerfile
Outdated
ONBUILD ARG ODOO_LIMIT_MEMORY_HARD=805306368 | ||
ONBUILD ARG ODOO_LIMIT_MEMORY_SOFT=671088640 | ||
ONBUILD ARG ODOO_LIMIT_REQUEST=8192 | ||
ONBUILD ARG ODOO_LIMIT_TIME_REAL_CRON=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 second for crons? not much sane...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops this was -1
in the defaults. Guess I can't read!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still missing...
Dockerfile
Outdated
@@ -27,10 +28,38 @@ ONBUILD ARG PGUSER=odoo | |||
ONBUILD ARG PGPASSWORD=odoopassword | |||
ONBUILD ARG PGHOST=db | |||
ONBUILD ARG PGDATABASE=prod | |||
ONBUILD ENV PGUSER="$PGUSER" \ | |||
ONBUILD ARG ODOO_MAX_CRON_THREADS=1 | |||
ONBUILD ARG ODOO_WORKERS=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimum 2 workers are required for production. Defaults should go for production always.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explicitly aligned with the defaults of Odoo so as to not change the operation of anything from default. I feel that this is better than attempting to make some determination of what someone wants for this image.
2616095
to
567152f
Compare
Hmmm so the build is failing, but I don't see how this is something I could have caused with this change & I have absolutely no idea how to fix it. Ideas?
|
(note I rebuilt three or four times under the hope it was a random error that would pass. no luck) |
Please rebase |
1d0148c
to
875d00b
Compare
Rebase looks to have cleared it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there @lasley, thanks!
I'm suggesting some little changes on the defaults. They diverge from Odoo's, but seem more sensible under our experience. Maybe @pedrobaeza wants to add some further suggestions before merging?
The thing I'm not sure about with this PR is what happens for preexisting projects that have their custom/conf.d
configurations? Next builds might get 2 values for the same key, and which one would take preference? Or would there be an error? Could you add a test for that please?
Also, does this have any value above changing the CMD that boots each container, adding the settings in CLI?
Dockerfile
Outdated
ONBUILD ARG ODOO_LIMIT_MEMORY_HARD=805306368 | ||
ONBUILD ARG ODOO_LIMIT_MEMORY_SOFT=671088640 | ||
ONBUILD ARG ODOO_LIMIT_REQUEST=8192 | ||
ONBUILD ARG ODOO_LIMIT_TIME_REAL_CRON=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still missing...
Dockerfile
Outdated
@@ -28,6 +28,17 @@ ONBUILD ARG PGUSER=odoo | |||
ONBUILD ARG PGPASSWORD=odoopassword | |||
ONBUILD ARG PGHOST=db | |||
ONBUILD ARG PGDATABASE=prod | |||
ONBUILD ARG ODOO_MAX_CRON_THREADS=-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default to 1.
Dockerfile
Outdated
@@ -28,6 +28,17 @@ ONBUILD ARG PGUSER=odoo | |||
ONBUILD ARG PGPASSWORD=odoopassword | |||
ONBUILD ARG PGHOST=db | |||
ONBUILD ARG PGDATABASE=prod | |||
ONBUILD ARG ODOO_MAX_CRON_THREADS=-1 | |||
ONBUILD ARG ODOO_WORKERS=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default to 2 (minimal required workers to get a functional worker-enabled odoo)
Dockerfile
Outdated
ONBUILD ARG ODOO_MAX_CRON_THREADS=-1 | ||
ONBUILD ARG ODOO_WORKERS=0 | ||
ONBUILD ARG ODOO_LIMIT_TIME_REAL=120 | ||
ONBUILD ARG ODOO_LIMIT_TIME_CPU=60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our experience it's quite better to increase these 2 defaults. Simple standard operations such as downloading a db backup never get done with these values. At least 10x please.
conf.d/60-performance.conf
Outdated
limit_memory_hard=$ODOO_LIMIT_MEMORY_HARD | ||
limit_memory_soft=$ODOO_LIMIT_MEMORY_SOFT | ||
limit_request=$ODOO_LIMIT_REQUEST | ||
limit_time_real_cron=$ODOO_LIMIT_TIME_REAL_CRON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please surround =
signs with spaces like in the other config files.
conf.d/70-security.conf
Outdated
@@ -0,0 +1,3 @@ | |||
[options] | |||
dbfilter=$ODOO_DB_FILTER | |||
list_db=$ODOO_LIST_DB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spaces here too.
BTW, if you are naming this file "security", please move admin_passwd
here, to be consistent, please.
conf.d/80-logging.conf
Outdated
@@ -0,0 +1,2 @@ | |||
[options] | |||
log_level=$ODOO_LOG_LEVEL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space
I really don't like changing the Odoo defaults. Why should we make the users memorize a completely new set of defaults? I agree that the Odoo ones aren't sensible in some scenarios, but that is what they are & they don't actually hurt anything. IMO the scaffolds are where we should be making configuration decisions. We just need a way to expose them to the user so that they're not remaking all of these scripts. Furthermore, I don't agree with the recommended default change to increase the worker time 10x. Our deploys use isolated cron and service instances that allow for long running operations. Web facing instances should not allow this type of thing, which can be an avenue for DOS. This is another reason that I think we should just leave the defaults - all of these options are contextual & everyone will have different ones. |
Regarding the duplications in configs - the last configuration item defined takes precedence. Which order do the configs drop in? Common then custom? or is it Custom then Common like it was in build/entry? |
Can we have a test on that please? You can test it with a simple odoo shell script.
|
Ok so I think that this works out great then with the custom coming after common, it can be completely controlled externally. Will add test, although I'm not quite sure what you mean by a shell script? I'll submit what I'm thinking, but I don't think it aligns with what you are because there's no script. |
0c28b68
to
0977cb1
Compare
depends_on: | ||
- postgresql | ||
- postgresql:manual_db_host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW This is not a valid syntax, although you have to revert this anyway (see above)
@@ -10,9 +10,8 @@ services: | |||
environment: | |||
PGUSER: another_odoo | |||
PGPASSWORD: anotherodoopassword | |||
PGHOST: postgresql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some entrypoint script will fail if you set an invalid $PGHOST
anyway.
I think I will explain better the test I expected:
-
Add to
environment
:ODOO_LIMIT_REQUEST: 8000 ODOO_LIST_DB: 0
-
Add a
dotd/custom/conf.d/request_limit.conf
with:[options] limit_request = 9000
-
Add
dotd/custom/entrypoint.d/600-goodconf.py
:#!/usr/local/bin/python-odoo-shell from odoo.tools import config assert config.get("limit_request") == 9000 assert not config.get("list_db")
This way we make sure the env variables get into config, and that manual configs override them.
0977cb1
to
1bbbce9
Compare
It seems Travis keeps failing on the below & it seems to be across all my PRs. Any ideas?
|
Can you rebase please? |
1bbbce9
to
1be3608
Compare
Hmmm so do we have any idea what's causing the text file error? Should I just rebase until it passes? It totally seems like something intermittent that is spread across all PRs |
See #65, which I'll reopen now. |
dd2bfdd
to
daad1d0
Compare
635050e
to
e88faa0
Compare
Is there anything else I need to do here other than rebase? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I comment below some missing points.
Also please remember to add changes to all Dockerfiles. We have 11.0 now!
Besides, with the rebase, you should get #65 fixed.
conf.d/60-performance.conf
Outdated
@@ -0,0 +1,9 @@ | |||
[options] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to remove this here and in the other files. It fails in Odoo 11
Dockerfile
Outdated
@@ -29,6 +29,17 @@ ONBUILD ARG PGUSER=odoo | |||
ONBUILD ARG PGPASSWORD=odoopassword | |||
ONBUILD ARG PGHOST=db | |||
ONBUILD ARG PGDATABASE=prod | |||
ONBUILD ARG ODOO_MAX_CRON_THREADS=2 | |||
ONBUILD ARG ODOO_WORKERS=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the image should be production first, I guess we can put 4 here, or at least 2 (minimum required for odoo to work and print reports).
Dockerfile
Outdated
ONBUILD ARG ODOO_LIMIT_TIME_REAL_CRON=-1 | ||
ONBUILD ARG ODOO_DB_FILTER="" | ||
ONBUILD ARG ODOO_LIST_DB=1 | ||
ONBUILD ARG ODOO_LOG_LEVEL=info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can reuse image's LOG_LEVEL? Or do you prefer to keep them separate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it would be best to keep them separate. The image log to me is a build/boot thing, vs the Odoo log being runtime. While they may be coordinated, they're for different people to use (the builder vs the maintainer)
Any news with this? |
1be3608
to
3aeac3f
Compare
Updated. Sorry for the lag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Travis is ❌
- Do we really need to prefix all of these with
ODOO_
? I think that's implicit... - Some other comments to make this even more awesome out of the box.
- Great job! 😊
8.0.Dockerfile
Outdated
@@ -29,6 +29,17 @@ ONBUILD ARG PGUSER=odoo | |||
ONBUILD ARG PGPASSWORD=odoopassword | |||
ONBUILD ARG PGHOST=db | |||
ONBUILD ARG PGDATABASE=prod | |||
ONBUILD ARG ODOO_MAX_CRON_THREADS=2 | |||
ONBUILD ARG ODOO_WORKERS=2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 4 is a better default here. 2 should be the minimal amount of workers to get a prefork server up and running, but even servers with a single CPU should be able to handle 4 workers nowadays, and it seems a better production default. The devel and test environments override this default already:
8.0.Dockerfile
Outdated
@@ -29,6 +29,17 @@ ONBUILD ARG PGUSER=odoo | |||
ONBUILD ARG PGPASSWORD=odoopassword | |||
ONBUILD ARG PGHOST=db | |||
ONBUILD ARG PGDATABASE=prod | |||
ONBUILD ARG ODOO_MAX_CRON_THREADS=2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better default is 1. 2 crons can collide each other and produce more concurrency errors, and besides cron jobs are usually not that important that can't be delayed a little bit until the previous job finishes. It's better to default to 1 and let customizations increase that number.
8.0.Dockerfile
Outdated
ONBUILD ARG ODOO_MAX_CRON_THREADS=2 | ||
ONBUILD ARG ODOO_WORKERS=2 | ||
ONBUILD ARG ODOO_LIMIT_TIME_REAL=120 | ||
ONBUILD ARG ODOO_LIMIT_TIME_CPU=60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With these limits it's quite common that you cannot download a database backup. I'd increase both to 5 minutes minimum by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's best to leave these at Odoo default and let a developer decide. I run my productions with these limits for security purposes. There is no reason a database backup should be able to complete from a production instance using user-facing methods
@@ -0,0 +1,2 @@ | |||
[options] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line or it will fail in odoo v11
IIRC there were some collisions, so I thought it best for consistency purposes. There's a lot more than just Odoo variables in this env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the test fails in odoo 9+ because AFAICS you cannot run a shell script against a database that does not exist.
Maybe you can fix the CI with just a 500-createdb.sh
file such as:
#!/usr/bin/env bash
# Need a preexisting DB to run odoo shell scripts
psql -d postgres -c "CREATE DATABASE $PGDATABASE"
Done. /crosses_fingers |
@@ -0,0 +1,3 @@ | |||
#!/usr/bin/env bash | |||
# Need a preexisting DB to run odoo shell scripts | |||
psql -d postgres -c "CREATE DATABASE $PGDATABASE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this file. See below.
@@ -0,0 +1,4 @@ | |||
#!/usr/local/bin/python-odoo-shell | |||
from odoo.tools import config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a problem on putting these test files at the entrypoint: both get executed before each test in dotd
, and there are a lot.
Put this file in a separate script dotd/custom/extras/goodconf.py
. Then add a command to the list that executes this script. Make sure you add the command after another test that should have created a database, this way you don't have to worry about the DB creation thing.
@@ -0,0 +1,4 @@ | |||
#!/usr/local/bin/python-odoo-shell | |||
from odoo.tools import config | |||
assert config.get("limit_request") == 9000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid we will need some more magic to make this test pass, because on v11 you cannot have duplicate config entries, 😕 possibly a new limitation on Python 3 implementation.
Merging as is would break v11 production deployments, not nice really... (Blessed be tests!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I think we just need another configuration variable. I've now exhausted all the ones that exist in Odoo, so I'll just make one up I think
Well hmmmm- I put this command last in the stack in hopes that it would be after a DB creation, but that does not seem to be the case. I'm not sure I understand this testing architecture/flow TBH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much nicer!
Now we have 2 failures, outlined below.
ONBUILD ARG ODOO_LIMIT_TIME_REAL_CRON=-1 | ||
ONBUILD ARG ODOO_DB_FILTER="" | ||
ONBUILD ARG ODOO_LIST_DB=1 | ||
ONBUILD ARG ODOO_LOG_LEVEL=info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to translate these ARG
into ENV
, like in the v8 dockerfile. That's the cause of v11 failure.
tests/__init__.py
Outdated
@@ -192,6 +192,8 @@ def test_dotd(self): | |||
("odoo", "--version"), | |||
# Implicit ``odoo`` command also works | |||
("--version",), | |||
# Environment vars were converted to config | |||
( "custom/extras/goodconf.py", ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that odoo shell
does not autocreate a database like normal odoo commands do from v9+. Then you can change this for ("bash", "-c", "odoo --stop-after-init && custom/extras/goodconf.py")
or something similar that creates the DB before executing the shell script.
Yikes! Well, now we're getting into it. 😉 The Now that we need it to create a Odoo db, we need to make the required steps for it. It seems we're hitting 2 problems:
|
I'm not sure if problem 2 would be fixed with a rebase now... |
1e04e60
to
720592b
Compare
Hmmm so v8 is still failing due to no db, but I very clearly see the create database command success from Postgres. I'm taking a total stab in the dark, but I'm thinking there's a different PSQL container for each matrix iteration, so I just created the db in all versions instead 🤷♂️ |
@@ -0,0 +1,4 @@ | |||
#!/usr/local/bin/python-odoo-shell | |||
from odoo.tools import config | |||
assert config.get("test_config_setting") == "Over9000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It says line 3 fails, so this is possibly not doing what you expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure why I'm even created that file or tested this part. Totally unrelated to the task at hand. Fixed.
If tests pass though, I think this might mean there's an issue with config generation in the custom directories.
@@ -0,0 +1,4 @@ | |||
#!/usr/local/bin/python-odoo-shell | |||
from odoo.tools import config | |||
assert config.get("limit_request") == "8000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still fails on line 3.
I'm suspecting that duplicate keys cannot be correctly parsed, or that the parameter is converted to int
and thus this assertion fails.
Maybe could you add assertion messages that help us to debug further? Example:
assert config.get("limit_request") == 8000, "%r is not %r" % (config.get("limit_request"), 8000)
... and the same in line 4
Hi friend, I added a scaffolding just to test settings in 52ad126, and it always uses a database. It's only testing versions >= 9.0 because older did not have odoo shell. Most likely you can take advantage of that to do the tests in an easier way 😊 |
* Add some more configurations, use the defaults from https://github.com/odoo/odoo/blob/3ace806d399e9e3abc5ada84ba98d2de100388d3/odoo/tools/config.py#L277
7ab819e
to
5178916
Compare
ODOO_DB_FILTER="$ODOO_DB_FILTER" \ | ||
ODOO_LIST_DB="$ODOO_LIST_DB" \ | ||
ODOO_LOG_LEVEL="$ODOO_LOG_LEVEL" | ||
>>>>>>> [IMP] Add some more default configurations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops.. wrong rebase
@@ -140,6 +140,9 @@ def test_settings(self): | |||
("--stop-after-init",), | |||
# SMTP settings work | |||
("./custom/scripts/test_smtp_settings.py",), | |||
# Environment vars were converted to config | |||
("bash", "-c", | |||
"odoo --stop-after-init && custom/extras/goodconf.py"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --stop-after-init
is done above, just call goodconf
This is very outdated and stale. Closing. |
This reverts commit 1bf8a8f.
This is dependent on: