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

Update Dockerfile & docker-compose for faster, usable builds #2942

Merged
merged 4 commits into from
Jan 24, 2022

Conversation

grahamalama
Copy link
Contributor

This PR:

  • Converts the Dockerfile to a multistage build

This is so that we can cache the layers responsible for building kinto-admin and installing the dependencies kinto relies on to build itself. This also leads to an overall smaller image by the end of the build.

Before:

  • Build time (no cache): ~5m 30s
  • Build time after minimal code change: ~5m 30s
  • Image size: 638MB

After

  • Build time (no cache): ~2m 30s
  • Build time after minimal code change: ~8s
  • Image size: 256MB

It also:

  • Upgrades Node from version 10 (which was breaking the build) to version 16 (LTS)
  • Modifies the docker-compose file to be usable again, since services must be nested under a top-level services key

@@ -7,7 +7,7 @@ jsonpatch==1.32
jsonschema==4.4.0
logging-color-formatter==1.0.2
newrelic==7.2.4.171
psycopg2-binary==2.9.3
psycopg2==2.9.3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is because kinto[postgresql] requires psycopg2 (not psycopg2-binary)

Copy link
Member

@Natim Natim Jan 21, 2022

Choose a reason for hiding this comment

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

psycopg2-binary is psycopg2 precompiled, we should keep the binary version IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

psycopg2-binary is psycopg2 precompiled, we should keep the binary version IMHO.

I would prefer that as well, but I think in order to do this, we'll have to change psycopg2 to psycopg2-binary in setup.cfg. Would that be acceptable? I didn't know if there was a good reason we needed to build psycopg2 when installing kinto.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to align the dependencies names. Shall we try psycopg2-binary everywhere?

Copy link
Contributor Author

@grahamalama grahamalama Jan 24, 2022

Choose a reason for hiding this comment

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

Shall we try psycopg2-binary everywhere?

Yep, we certainly can. I was just ensuring there wasn't a good reason to build psycopg2. From their docs:

If you are the maintainer of a published package depending on psycopg2 you shouldn’t use psycopg2-binary as a module dependency. For production use you are advised to use the source distribution.

and

Warning
The psycopg2 wheel package comes packaged, among the others, with its own libssl binary. This may create conflicts with other extension modules binding with libssl as well, for instance with the Python ssl module: in some cases, under concurrency, the interaction between the two libraries may result in a segfault. In case of doubts you are advised to use a package built from source.

Copy link
Contributor Author

@grahamalama grahamalama Jan 24, 2022

Choose a reason for hiding this comment

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

@Natim I'm going to merge this PR as-is, then open a follow-up issue where we can further discuss using psycopg2 vs psycopg2-binary

EDIT:
Follow-up issue #2943

@@ -7,7 +7,7 @@ jsonpatch==1.32
jsonschema==4.4.0
logging-color-formatter==1.0.2
newrelic==7.2.4.171
psycopg2-binary==2.9.3
psycopg2==2.9.3
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to align the dependencies names. Shall we try psycopg2-binary everywhere?

@grahamalama grahamalama merged commit 46c84bb into master Jan 24, 2022
@grahamalama grahamalama deleted the docker-updates branch January 24, 2022 13:44
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.

3 participants