-
Notifications
You must be signed in to change notification settings - Fork 11
provide consistent environement to test SSL libraries with 3rd party … #49
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
Conversation
|
Is it worth codifying these scripts as a container, so we can just test the pre-built images, and updated them on a schedule rather than having to rebuild them every time? |
I've deliberately opted to write this in script form (ksh) to avoid containers so things can be tested on any *nix OS where ksh and tool chains are available. The build on ubuntu virtual machine in our data center (16vCPUs 8GB ram) takes ~20 minutes. the container image is tempting, but it's too big hammer in my opinion. I prefer a lightweight script with better portability. the container is kind of platform on its own. |
bob-beck
left a comment
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.
Yes please, land ASAP and then please add a README.md to this directory, even if initially perfunctory, to say what and how they are and how you use them.
bench-scripts/apache_bench.sh
Outdated
| @@ -0,0 +1,633 @@ | |||
| #!/bin/ksh -x | |||
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.
TIL about ksh. Maybe #!/usr/bin/env ksh instead?
bench-scripts/apache_bench.sh
Outdated
| # 4.0M Sep 12 14:49 test_16.txt | ||
| # | ||
|
|
||
| INSTALL_ROOT=${BENCH_INSTALL_ROOT:-"$HOME/work.openssl/bench.binaries"} |
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.
Hmm always installing it here might be unexpected. Not saying we should change it, just thinking that maybe it can go to /tmp or current directory instead
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 makes sense to change default variant to /tmp/bench.binaries
| # make sure master is up-to date just in | ||
| # case we build a master version | ||
| # | ||
| git checkout master || exit 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.
Any reason for not just using set -e instead of adding || exit 1 everywhere?
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.
just personal preference as I prefer to be explicit. do you mind if I keep the script the way it is now?
bench-scripts/nginx_bench.sh
Outdated
| if [[ -n "${VERSION}" ]] ; then | ||
| git checkout -b stable-${VERSION} origin/stable-${VERSION} || exit 1 | ||
| fi |
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 can be removed as above
| --enable-apachehttpd \ | ||
| --enable-postauth || exit 1 | ||
|
|
||
| make ${MAKE_OPTS} || exit 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.
adding -j will have better speed on multicore envs. And all the other occurrences in the PR
| make ${MAKE_OPTS} || exit 1 | |
| make ${MAKE_OPTS} -j || exit 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.
no please, -j make option on bsd always requires number of jobs.
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 see; would $(nproc) work?
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.
unfortunately not
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.
That's unfortunate. The build takes too much time in the whole script. Do you see a way to optimize it somehow?
| rm -f "${WORKSPACE_ROOT}/${TEST_FILE}" | ||
| } | ||
|
|
||
| function cleanup { |
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.
Is this ever called? Maybe you can set a bash exit trap to call this.
| fi | ||
|
|
||
| typeset TEST_FILE=".test_file.$$" | ||
| mkdir -p "${WORKSPACE_ROOT}" |
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.
Should we check that "${WORKSPACE_ROOT}" and "${INSTALL_ROOT}" don't exist/are empty before creating 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.
The script does not require install/workspace directories to be empty. The only requirement is that directories must exist (ensured by mkdir -p) and be writable (the write test happens few lines below).
jogme
left a comment
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.
LGTM; thank you!
jogme
left a comment
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.
Looking good, thank you!
…tool the idea is to have a script (set of scripts) which build and install environement such we can evaluate SSL performance of different SSL implementations. The initial version supports httpd from apache. The idea is to build desired SSL library and install it to custom location (INSTALL ROOT). Then we build apache and siege client and install it to INSTALL ROOT. The proposed approach uses shell script as it is very lightweight and portable between various *NIX flavours.
1438b63 to
cc4b43d
Compare
…tool the idea is to have a script (set of scripts) which build and install environement such we can evaluate SSL performance of different SSL implementations. The initial version supports httpd from apache. The idea is to build desired SSL library and install it to custom location (INSTALL ROOT). Then we build apache and siege client and install it to INSTALL ROOT. The proposed approach uses shell script as it is very lightweight and portable between various *NIX flavours. Reviewed-by: Neil Horman <[email protected]> (Merged from #49)
|
it got merged in via #e3201 |
…tool the idea is to have a script (set of scripts) which build and install environement such we can evaluate SSL performance of different SSL implementations. The initial version supports httpd from apache. The idea is to build desired SSL library and install it to custom location (INSTALL ROOT). Then we build apache and siege client and install it to INSTALL ROOT. The proposed approach uses shell script as it is very lightweight and portable between various *NIX flavours. Reviewed-by: Neil Horman <[email protected]> (Merged from openssl#49)
…tool
the idea is to have a script (set of scripts) which build and install environement such we can evaluate SSL performance of different SSL implementations.
The initial version supports httpd from apache. The idea is to build desired SSL library and install it to custom location (INSTALL ROOT). Then we build apache and siege client and install it to INSTALL ROOT.
The proposed approach uses shell script as it is very lightweight and portable between various *NIX flavours.