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

Complement QoL changes #2663

Merged
merged 8 commits into from
Aug 23, 2022
Merged

Complement QoL changes #2663

merged 8 commits into from
Aug 23, 2022

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Aug 22, 2022

This PR does the following:

  • adds a keysize parameter to generate-keys, so we can use lower sized keys when running in CI
  • updates the Complement docker files to use BuildKit (requires Docker >18.09)
  • uses exec when executing dendrite-monotlith-server, making it PID 1 inside docker, which results in Dendrite actually receiving the SIGTERM signal send by Docker. (Making it faster when running tests with Complement, as we don't take 10 seconds to timeout)

CC: @MadLittleMods I think you were complaining about longer build/execution times, iirc?

@S7evinK S7evinK requested a review from a team as a code owner August 22, 2022 14:52
exec ./dendrite-monolith-server --really-enable-open-registration --tls-cert server.crt --tls-key server.key --config dendrite.yaml -api=${API:-0}
Copy link
Contributor

Choose a reason for hiding this comment

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

CC: @MadLittleMods I think you were complaining about longer build/execution times, iirc?

Yes! Especially with Synapse, matrix-org/synapse#13204

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like Synapse is already PID 1 inside the container and receives the SIGTERM request, so this is not the issue. :/

Copy link
Contributor

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

LGTM, although might be best to run it by @kegsay too.

Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

It would be nice to see some actual timings before/after.

./generate-config -server $SERVER_NAME --ci > dendrite.yaml && \
cp /complement/ca/ca.crt /usr/local/share/ca-certificates/ && update-ca-certificates && \
./dendrite-monolith-server --really-enable-open-registration --tls-cert server.crt --tls-key server.key --config dendrite.yaml -api=${API:-0}
exec ./dendrite-monolith-server --really-enable-open-registration --tls-cert server.crt --tls-key server.key --config dendrite.yaml -api=${API:-0}
Copy link
Member

Choose a reason for hiding this comment

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

What difference does this make in practice?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seeming 5m 49s on this PR, vs 7m 5s on the last main run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For Postgres it's almost 5m faster (8m 19s vs 12m 49s)

@S7evinK S7evinK merged commit 95a5097 into main Aug 23, 2022
@S7evinK S7evinK deleted the s7evink/complement branch August 23, 2022 11:10
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