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

cache the curl #49

Merged

Conversation

misterbisson
Copy link
Contributor

Caches the curl -s --fail localhost/nginx-health so that we can do consistent math on the values in it.

Fixes #48

Copy link

@jwreagor jwreagor left a comment

Choose a reason for hiding this comment

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

LGTM, this was easy enough to review and caching would be better than firing two separate requests.

Update containerpilot, consul, consul-template

# Add some stuff via apt-get
RUN apt-get update \
&& apt-get install -y --no-install-recommends \
bc \
ca-certificates \
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 appears to be required for nginx:1.13

@@ -1,18 +1,19 @@
# A minimal Nginx container including ContainerPilot
FROM nginx:1.11
FROM nginx:1.13
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old 1.11 was too old for me to feel comfortable pushing a new image from.

Choose a reason for hiding this comment

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

Good call, 1.11 came out a year ago.

curl \
unzip \
&& rm -rf /var/lib/apt/lists/*

# Install Consul
# Releases at https://releases.hashicorp.com/consul
RUN export CONSUL_VERSION=0.7.3 \
&& export CONSUL_CHECKSUM=901a3796b645c3ce3853d5160080217a10ad8d9bd8356d0b73fcd6bc078b7f82 \
RUN export CONSUL_VERSION=0.7.5 \
Copy link
Contributor Author

@misterbisson misterbisson May 16, 2017

Choose a reason for hiding this comment

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

The next version should probably jump to Consul 0.8.x, but I'm staying on the 0.7 branch because of some bugs in the first releases of 0.8.

bin/sensor Outdated
@@ -9,8 +9,9 @@ help() {

# Cummulative number of dropped connections
unhandled() {
local accepts=$(curl -s --fail localhost/nginx-health | awk 'FNR == 3 {print $1}')
local handled=$(curl -s --fail localhost/nginx-health | awk 'FNR == 3 {print $2}')
local scraped=$(curl -s --fail localhost/nginx-health)
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 is the most important change in this PR.

makefile Outdated
@@ -0,0 +1,59 @@
# Makefile for building, shipping, and testing the container.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was no makefile, and building the old way suddenly felt dirty.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a Makefile in test/

Copy link
Contributor

Choose a reason for hiding this comment

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

Which yes, is not a great place to put it but this component blueprint can't really be tested without an example backend, so the whole building and testing outside of a simple docker build has been done in test/ and examples/. Probably could use some improvements, which I was planning on tackling this Q as part of the general set of work to improve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I missed the /test

unhandled() {
local accepts=$(curl -s --fail localhost/nginx-health | awk 'FNR == 3 {print $1}')
local handled=$(curl -s --fail localhost/nginx-health | awk 'FNR == 3 {print $2}')
local scraped=$(curl -s --fail localhost/nginx-health)
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 is the key change

echo $(expr ${accepts} - ${handled})
}

# ratio of connections-in-use to available workers
connections_load() {
local scraped=$(curl -s --fail localhost/nginx-health)
local active=$(echo ${scraped} | awk '/Active connections/{print $3}')
local waiting=$(echo ${scraped} | awk '/Reading/{print $6}')
local active=$(echo "${scraped}" | awk '/Active connections/{print $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.

The result from curl -s --fail localhost/nginx-health is a multi-line string, so we need to quote it when echoing it.

@misterbisson
Copy link
Contributor Author

I tested this in the WordPress blueprint (see autopilotpattern/wordpress#48), as well as an internal production blueprint.

@misterbisson misterbisson merged commit 57689a6 into master May 18, 2017
@misterbisson misterbisson deleted the 48-nginx_connections_unhandled_total-appears-incorrect branch May 18, 2017 02:54
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