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

Add official images for TICK stack #1583

Merged
merged 1 commit into from
May 12, 2016

Conversation

nathanielc
Copy link

Telegraf
InfluxDB
Chronograf
Kapacitor

@FelixWeis
Copy link

Very nice, I've been waiting to get this into the library.

Small request: All containers in the TICK stack should all use the same base image, e.g. right now influxdb uses debian:jessie and telegraf uses ubuntu:trusty.

It would be nice if the base images would all be at least debian:jessie.

Even better: The baseimage buildpack-deps:jessie-curl is part of the docker-library and basicly debian:jessie + curl, wget and ca-certificates and is used by many other containers instead of base debian. Since all the 4 images share the first RUN command to get those dependencies, using that image would save even more space/layers. Savings around 4x44 MB.

@FelixWeis
Copy link

I created 4 pull requests in chronograf, influxdb, kapacitor, telegraf to change to base image.

@jsternberg
Copy link
Contributor

We can't right now for telegraf because the packaging of the current version uses a tool available in ubuntu but not available in debian (deb-systemd-invoke). We changed it to use systemctl like it should have done originally, but we weren't planning on having a patch release for just that change and were just going to let that roll out with the next version.

Is that ok? I'll check the PR's for the other suggestion you made.

@@ -0,0 +1,8 @@
maintainer: Shubhra Kar <[email protected]> (@ShubhraKar)

Choose a reason for hiding this comment

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

maintainer line is a comment, not an instruction.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've got a few PR's to fix this. One of us will update this PR when we fix this.

@e-dard
Copy link
Contributor

e-dard commented Apr 4, 2016

@FelixWeis I've updated the revisions based on feedback here, and some improvements on our end. As @jsternberg says, we can't currently use buildpack-deps:jessie-curl for Telegraf, but I suggest we use buildpack-deps:trusty-curl for Telegraf instead.

When the next Telegraf release lands I'll move it over to buildpack-deps:jessie-curl.

@tianon
Copy link
Member

tianon commented Apr 6, 2016

Did an initial review pass, and just a couple minor comments to start with:

  1. what's the purpose of set -- "$@" in the entrypoint.sh scripts? That's essentially saying "modify $@ to match the contents of $@", so it's a little confusing what that's attempting to accomplish 😄

  2. gpg --verify should be switched to use gpg --batch --verify (Fix suggested "gpg" usage to stop relying on deprecated and insecure behavior #1420)

  3. influxdb.key should be coming off the keyservers, or at least be verified by the full fingerprint
    ie: (added after the wget && import && rm)

    gpg -k 05CE15085FC09D18E99EFB22684A14CF2582E0C5
    

    or better yet: (replacing the wget ... bit completely)

    gpg --keyserver ha.pool.sks-keyservers.net --recv-keys 05CE15085FC09D18E99EFB22684A14CF2582E0C5
    

@tianon
Copy link
Member

tianon commented Apr 6, 2016

I'd add that these look generally OK otherwise (although I caveat that I haven't really dug in very deep, just a quick surface inspection to get a feel for what's here), and that you should start considering your content for a PR against https://github.com/docker-library/docs if you haven't started thinking about that already. 👍

@yosifkit
Copy link
Member

yosifkit commented Apr 6, 2016

All of it seems pretty good to me pending gpg updates and a PR to the docs.

Two suggestions:

  • use ha.pool.sks-keyservers.net since we have had issues with pgp.mit being unavailable
  • add scripts to help you generate these library files like mongo and the other images maintained within docker-library have.

This second suggestion is not strictly necessary but more helpful for you to manage the commit ids.

@jsternberg
Copy link
Contributor

@yosifkit for the docs PR, what needs to be in it? I've been having trouble with creating a PR for that since it seems like some of the files are generated and depend on this PR being merged first.

WIP PR here: influxdata/docker-docs#1

@jsternberg
Copy link
Contributor

I've updated all of the images with the feedback from this thread. I just need some guidance on what to do with the docs and then I can get a PR for that. Do we need this to be merged first?

@yosifkit
Copy link
Member

Thanks, Dockerfiles look good to me. We usually merge the PRs at about the same time, so that you repos get a description soon after being pushed to the Docker Hub.

As for what is required, you'll need a content.md, README-short.txt, and a logo.png for each of your 4 products. The README.md and tag-details.md files get generated automatically (you can run update.sh repo-name to see what it will create for the readme file). You'll want to put %%LOGO%% in content.md where you want the image to appear. The "Supported tags and Dockerfile links" section is generated from the content of the files here in library/, so that will only appear correctly once this PR merges. The "User Feedback" section is also generated, but can be overridden by supplying a user-feedback.md. A license.md is useful to let users know the license of the contained software and will be inserted into the resulting README.md.

You probably want to run mardownfmt.sh -d repo-name/ to check the formatting of your markdown files. The -d option will print out a diff of what needs changing (travis runs this test on the PR).

@yosifkit
Copy link
Member

As far as your current docs go, just a couple changes:

  • remove Supported Docker versions section, this one is generated too 😄
  • don't break to 80 column lines, keep paragraphs one long line.
  • use fenced code blocks with language like console, this will improve highlighting a little bit on the Docker Hub (and in github too)
  • lists shoule be -{tab}item (ie, most indentation is a tab)

@yosifkit
Copy link
Member

If you want to keep any unfenced code blocks, they'll need to be tab indented for the block and you are then free to use spaces within the code. markdownfmt.sh -d should be able to show you many of these. 😃

@psftw
Copy link
Contributor

psftw commented Apr 12, 2016

Having trouble building chronograf:0.12 -- looks like this file is missing https://s3.amazonaws.com/get.influxdb.org/chronograf/chronograf_0.12.0_amd64.deb.asc

@jsternberg
Copy link
Contributor

@psftw it's fixed now. You should be able to build chronograf:0.12.

@psftw
Copy link
Contributor

psftw commented Apr 12, 2016

Thanks, got it building, and now taking a closer look. Note: The Official Repository should only contain tags for the latest version of each supported branch, and possibly release candidates (though we discourage "nightly" or CI-style builds). If you plan on supporting the 0.10 and 0.11 branches with additional releases, then no worries, otherwise we should take them out. Other projects tend to put deprecated and/or CI testing releases in a separate organization/project repository.

@yosifkit
Copy link
Member

Also, if 0.10 and 0.11 are going to be unsupported, you could keep them on this first PR as a backfill of old versions and then follow this with a new PR to drop them. The images would still be pull-able from the Docker Hub, but the list generated in "Supported tags and respective Dockerfile links" on the Hub description would be limited by what is in these library/ files. No need to drop them if you want to keep them for an 0.10.x release.

Your docs over on influxdata/docker-docs#1 look pretty good to me. Want to make it a PR to the official-images/docs?

@jsternberg
Copy link
Contributor

That would be really good if we could allow 0.10 and 0.11 on the Docker Hub as unsupported, but still on there for people to pull them. The 0.12 version is still rather new.

I'm just waiting on a review from a co-worker for the documentation. Someone should review it before tomorrow and I'll make a PR for it referencing this issue so we can get all of them merged.

@jsternberg
Copy link
Contributor

I have created the PR for the documentation in docker-library/docs#548.

@jsternberg
Copy link
Contributor

Hey @yosifkit is there anything more that we have to do before this gets merged?

@psftw
Copy link
Contributor

psftw commented Apr 15, 2016

I don't see any major issues. I'm also going to be sending over a bunch of tweaks to the doc PR soon as I'm working through it. Overall, really excited about this! 👍

@psftw
Copy link
Contributor

psftw commented Apr 15, 2016

I should also add that I've only looked at the 0.12 images, so I can't comment on 0.10 or 0.11.

@jsternberg
Copy link
Contributor

All of them should be the same. After the initial merge, I'm also going to remove the 0.10 and 0.11 images since I don't think we're maintaining those versions anymore, but we would like the images up there for those who may still be using those versions.

The person who works on Kapacitor is working on updating the documentation. We'll have that soon.

@jsternberg
Copy link
Contributor

We've updated the documentation for Kapacitor. For the questions about environment variables, I'm not sure I know the answers to those questions. Is it possible to have this merged so the images are available for people to use and then we can always update the documentation if that's implemented?

For the environment variables part, is there a better way to set default values for some of the environment variables? The purpose is to provide a default if a configuration file isn't provided.

@yosifkit
Copy link
Member

If the server automatically uses the specified environment variables over a given config file, I would just use different env names and only export the envs to the ones used by the server when there is no config file. Or you could generate a config file from them if it does not exist.

@jsternberg jsternberg force-pushed the initial-release branch 2 times, most recently from f9d5c4b to 3981873 Compare April 22, 2016 21:56
@jsternberg
Copy link
Contributor

I've modified all of the images to start using config files completely instead of environment variables, but still let environment variables be used to override the defaults.

The applications don't have a way of loading a default configuration file yet, so right now, the -config /etc/influxdb/influxdb.conf line is needed and I included it in the default command. For 0.13, we're working on a change to each product where it will load a default file so this works better in the future. So right now, this will not use the config file correctly:

$ docker run --rm influxdb influxd

While this will:

$ docker run --rm influxdb

We want both to work out of the box, but that will be in 0.13 when those are released and I can update the Docker images. Is that OK?

@tianon
Copy link
Member

tianon commented Apr 22, 2016

Just sent a few PRs that emulate what you've described 0.13 will be doing with some simple entrypoint.sh code for now. 😄

As an aside, it's a little bit tedious (for you, I mean) that you've got four repos here -- if it'd simplify your life, feel free to combine all these into a single Git repo (bonus points for using Git sub-tree merging to keep the history of each directory -- happy to help there if you're interested) with directories like repo/kapacitor/0.12, etc.

Since they're all sooo similar (and inter-related), I don't see any harm in keeping them all lumped together in one repo.

(But it's not strictly necessary for you to do this for this to be accepted -- just offering the suggestion in case it makes your life easier. 😄)

@jsternberg
Copy link
Contributor

I updated the images with each of the PR's you sent. For the single repository, we talked about that when starting out and decided to keep them as separate repositories. I think in the future we would be able to update them to use a single repository if we decide that's an easier way to manage the repositories. It should be possible to merge them at a later time by just making a new repository and sending a PR with the updated paths.

@yosifkit
Copy link
Member

@jsternberg, it sounds reasonable to merge them to one git repository later if needed.

LGTM, Build test of #1583; d012b74 (chronograf, influxdb, kapacitor, telegraf):

$ bashbrew build "chronograf"
Cloning chronograf (git://github.com/influxdata/chronograf-docker) ...
Processing chronograf:0.12 ...
Processing chronograf:0.12.0 ...
Processing chronograf:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing chronograf:0.12
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
$ bashbrew build "influxdb"
Cloning influxdb (git://github.com/influxdata/influxdb-docker) ...
Processing influxdb:0.12 ...
Processing influxdb:0.12.2 ...
Processing influxdb:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing influxdb:0.12
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
$ bashbrew build "kapacitor"
Cloning kapacitor (git://github.com/influxdata/kapacitor-docker) ...
Processing kapacitor:0.12 ...
Processing kapacitor:0.12.0 ...
Processing kapacitor:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing kapacitor:0.12
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
$ bashbrew build "telegraf"
Cloning telegraf (git://github.com/influxdata/telegraf-docker) ...
Processing telegraf:0.12 ...
Processing telegraf:0.12.0 ...
Processing telegraf:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing telegraf:0.12
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed

@psftw
Copy link
Contributor

psftw commented Apr 25, 2016

I just took another pass at the docs based on the latest image changes. Overall I don't have any blocking issues with the images themselves, but have concerns about the docs, mostly just nitpicks on kapacitor, but also some needed changes based on the recent image changes. It would go a long way to see a compose file with the whole stack, and bonus points for utilizing the telegraf docker stats plugin. 💯 🏆

@nathanielc
Copy link
Author

@psftw We have an open PR here https://github.com/influxdata/TICK-docker/pull/2 for a docker compose file that uses the whole TICK stack. It hasn't been updated for all the recent changes to the images. I have been waiting until the official images are out to finalize any changes but the general idea is there.

@jsternberg jsternberg force-pushed the initial-release branch 3 times, most recently from 2603e29 to 23d22d3 Compare May 4, 2016 16:43
@tianon
Copy link
Member

tianon commented May 5, 2016

Changes in 23d22d3: https://github.com/influxdata/influxdb-docker/compare/566e8eab4f048676e330e4a538bfec81b6acdfc1...f71504677c2cddc9f11add55d8ddb968eef8e2d4, https://github.com/influxdata/kapacitor-docker/compare/e7458927e5ddd82abe4e91d0c20ec46b9d44dc71...6e2b35432afa15244e8c982b9615b6eb9f5e795d

Images LGTM; @psftw are you happy with where the docs are now (docker-library/docs#548), or do you have more comments to make over there?

@jsternberg
Copy link
Contributor

@psftw is there anything else we need to do with the documentation or the image for this to be merged? Thanks.

Telegraf
InfluxDB
Chronograf
Kapacitor
@psftw
Copy link
Contributor

psftw commented May 12, 2016

Latest documentation changes look good on the surface. I won't have time to close the loop on testing the whole TICK stack together this week as I had originally planned, but also don't want to hold things up over my initial troubles with Kapacitor. Thanks!

@jsternberg
Copy link
Contributor

Is it possible to get these merged before next week? If there needs to be further work to improve the documentation or images, we'll be around to help with addressing any issues. We want to start pointing our users to being able to use these images for Docker though as soon as we can.

@tianon
Copy link
Member

tianon commented May 12, 2016

Changes in a5d27ef: https://github.com/influxdata/influxdb-docker/compare/f71504677c2cddc9f11add55d8ddb968eef8e2d4...2db51e1d86672d2eb76636b17adbea441e767745, https://github.com/influxdata/kapacitor-docker/compare/6e2b35432afa15244e8c982b9615b6eb9f5e795d...9ee9a425da8453b91d9c4773df781391960968b8

LGTM

Build test of #1583; a5d27ef (chronograf, influxdb, kapacitor, telegraf):

$ bashbrew build "chronograf"
Cloning chronograf (git://github.com/influxdata/chronograf-docker) ...
Processing chronograf:0.12 ...
Processing chronograf:0.12.0 ...
Processing chronograf:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing chronograf:0.12
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
$ bashbrew build "influxdb"
Cloning influxdb (git://github.com/influxdata/influxdb-docker) ...
Processing influxdb:0.12 ...
Processing influxdb:0.12.2 ...
Processing influxdb:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing influxdb:0.12
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
$ bashbrew build "kapacitor"
Cloning kapacitor (git://github.com/influxdata/kapacitor-docker) ...
Processing kapacitor:0.12 ...
Processing kapacitor:0.12.0 ...
Processing kapacitor:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing kapacitor:0.12
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
$ bashbrew build "telegraf"
Cloning telegraf (git://github.com/influxdata/telegraf-docker) ...
Processing telegraf:0.12 ...
Processing telegraf:0.12.0 ...
Processing telegraf:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing telegraf:0.12
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed

@yosifkit
Copy link
Member

LGTM

@yosifkit yosifkit merged commit 0856b7c into docker-library:master May 12, 2016
@jsternberg jsternberg deleted the initial-release branch May 12, 2016 16:42
@psftw
Copy link
Contributor

psftw commented May 14, 2016

🎉 belated welcome and thanks for putting up with me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants