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 varnish #4404

Closed
wants to merge 1 commit into from
Closed

Conversation

teohhanhui
Copy link
Contributor

@teohhanhui teohhanhui commented May 29, 2018

Reviving #1294.

Varnish Cache is a widely used caching HTTP reverse proxy.

As we've seen in #1294 (which remained open for a long time) upstream has not shown any interest.

I believe all the best practices have been sufficiently adhered to, including the Official Images Test Suite, update.sh and generate-stackbrew-library.sh.

Docs PR: docker-library/docs#1241

Checklist for Review

NOTE: This checklist is intended for the use of the Official Images maintainers both to track the status of your PR and to help inform you and others of where we're at. As such, please leave the "checking" of items to the repository maintainers. If there is a point below for which you would like to provide additional information or note completion, please do so by commenting on the PR. Thanks! (and thanks for staying patient with us ❤️)

  • associated with or contacted upstream?
  • does it fit into one of the common categories? ("service", "language stack", "base distribution")
  • is it reasonably popular, or does it solve a particular use case well?
  • does a documentation PR exist? (should be reviewed and merged at roughly the same time so that we don't have an empty image page on the Hub for very long)
  • dockerization review for best practices and cache gotchas/improvements (ala the official review guidelines)?
  • 2+ dockerization review?
  • existing official images have been considered as a base? (ie, if foobar needs Node.js, has FROM node:... instead of grabbing node via other means been considered?)
  • if FROM scratch, tarballs only exist in a single commit within the associated history?
  • passes current tests? any simple new tests that might be appropriate to add? (https://github.com/docker-library/official-images/tree/master/test)

@teohhanhui
Copy link
Contributor Author

teohhanhui commented May 31, 2018

ping @tianon @yosifkit

@teohhanhui
Copy link
Contributor Author

Tagging @kevin1024 @avoinea @andrerom from the previous PR.

@teohhanhui
Copy link
Contributor Author

My new attempt to contact upstream:
https://twitter.com/gquintard_/status/1011691328353292288

/cc @gquintard who wrote this blog post: https://info.varnish-software.com/blog/varnish-docker

@gquintard
Copy link
Contributor

Hi, thanks for tagging me.

I had a look and only have a couple of questions/objections:

@teohhanhui
Copy link
Contributor Author

why compile from source

It seems to be the Docker way. And yes, it allows anyone to easily customize the build.

I'll look into the tmpfs.

99.9999999999%

Point taken. But seeing that the source .tgz is only ~3.0 MB, I don't think that's a major issue.

@gquintard
Copy link
Contributor

My point about the vmods using the source is really more about maintainability and being correct; people will read your instructions and think that vmods still need the source, even though your instructions were already outdated when you wrote them. I mean, vmods today don't even know about VARNISHSRC anymore.

Plus, your argument about the source being small doesn't stand since you take care of removing the vmod's tgz in the Dockerfile, and that file is super small.

Other than than, no issues on the purely varnish sides of things

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Jun 28, 2018

Okay, I'll remove the Varnish source after compilation. 😄

@andrerom
Copy link
Contributor

Biased, but would also like to see https://github.com/varnish/varnish-modules part of the image, some of the modules are becoming quite broadly used (xkey, variable, sessions, ..).

It's quite easy to install it on top of the varnish packages @gquintard recommends, example of that:
https://github.com/ezsystems/ezplatform/blob/master/doc/docker/Dockerfile-varnish

@teohhanhui
Copy link
Contributor Author

If it's easy to install, we don't need to include it. For example, the official PHP image only includes non-default extensions that are impossible or too hard to install.

But if there's a unified way to install most Varnish modules (I doubt so), we could certainly add a helper script.

@gquintard
Copy link
Contributor

gquintard commented Jun 29, 2018 via email

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Jul 13, 2018

@gquintard

Building requires the Varnish header files and uses pkg-config to find the necessary paths.

from https://github.com/varnishcache/libvmod-example

So we still need to keep the source around...

EDIT: Hmm, I guess I was mistaken... The header files are in /usr/local/include/varnish

@teohhanhui
Copy link
Contributor Author

All issues raised by @gquintard have been resolved. 🎉

@teohhanhui teohhanhui force-pushed the add/varnish branch 2 times, most recently from 501362a to fc99134 Compare July 17, 2018 16:20
@teohhanhui
Copy link
Contributor Author

@andrerom Alpine variant added. 🎉

Also, the debian variant is now 33% smaller.

@docker-library-bot
Copy link
Member

Hello! ✨

Thanks for your interest in contributing to the official images program. 💭

As you may have noticed, we've usually got a pretty decently sized queue of new images (not to mention image updates and maintenance of images under @docker-library which are maintained by the core official images team). As such, it may be some time before we get to reviewing this image (image updates get priority both because users expect them and because reviewing new images is a more involved process than reviewing updates), so we apologize in advance! Please be patient with us (and avoid poking us about your image via other communication means -- rest assured, we've seen your PR and it's in the queue). ❤️

We do try to proactively add and update the "new image checklist" on each PR, so if you haven't looked at it yet, that's a good use of time while you wait. ☔

Thanks! 💖 💙 💚 ❤️

@teohhanhui
Copy link
Contributor Author

and avoid poking us about your image via other communication means

Oops! Sorry...

@tianon
Copy link
Member

tianon commented Jul 26, 2018

Thanks @docker-library-bot ❤️

why compile from source

It seems to be the Docker way.

Just to be clear, compiling from source isn't "the Docker way" -- for all official images, we explicitly recommend following upstream's recommendations. For example, postgres installs the PostgreSQL packages built/provided by upstream (and only builds them from their source for architectures that upstream doesn't publish packages for). We do tend to have a lot of upstreams that don't publish their own recommended packages, so compiling from source is often the "recommended" path from the upstream.

A few other initial notes after taking a first pass over the Dockerization:

  1. are gcc, libc-dev, and libgcc needed at runtime for Varnish, or would they be more appropriate in VMOD_BUILD_DEPS? (are they used in VCC-compiler? I'm not terribly familiar with how Varnish works under the hood)

  2. is there a default example .vcl file that makes sense to ship? (it's OK if the answer is no, for example haproxy doesn't ship one because there isn't really any default that makes sense for even a minority of users and I understand it has a somewhat similar use to Varnish)

  3. are the patches in /varnish-alpine-patches/ temporary (as in, something upstream is considering including and/or handling), or are those expected to stay something specific to the Docker image?

  4. --without-jemalloc strikes me as odd -- if upstream defaults to using jemalloc, shouldn't the Docker image also?

  5. setting the default args via entrypoint only if there aren't any args is kind of odd -- that means that users who do docker run ... varnish will get a very different set of arguments from users who do docker run ... varnish -f ...; I'd recommend either:

    • if varnishd can accept a later argument that overwrites the value of any of -F, -a, -f, or -s (or any of those should just always be set for Docker use anyhow), just always add them before what the user specifies
    • put them in CMD [] instead so that users can use docker inspect to determine the default set of arguments (using environment variables to change arguments to the application is a bit odd, we don't have many official images currently which do so)

@teohhanhui
Copy link
Contributor Author

  1. Yes, they're runtime dependencies. At first I had the same thought too...

  2. I don't think so, as we'll have nothing to use as the default "backend"?

  3. All the patches are taken directly from Alpine. I think upstream has been working on improving musl libc support, but obviously it's still not perfect.

  4. Because compilation fails with jemalloc, and the Alpine guys haven't cracked that one yet.

@gquintard
Copy link
Contributor

gquintard commented Jul 26, 2018

  1. in master, all patches are now superfluous

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Aug 12, 2018

put them in CMD [] instead so that users can use docker inspect to determine the default set of arguments (using environment variables to change arguments to the application is a bit odd, we don't have many official images currently which do so)

But wouldn't that make for very bad DX?

EDIT: I've made the CMD as minimal as possible.

@teohhanhui
Copy link
Contributor Author

@tianon @gquintard Is it important to use upstream built packages in this case? I looked at the postgres Dockerfile, and it seems to add a lot of complexity...

@gquintard
Copy link
Contributor

it's not that it important, it's just that's it way faster, for rom, the Dockerfile is literally 5 lines long:

FROM centos:7

RUN yum install -y epel-release && \
    curl -s https://packagecloud.io/install/repositories/varnishcache/varnish60/script.rpm.sh | bash && \
    yum install -y varnish

You get the init scripts, libs are packaged in a devel package if you want to build vmods, I honestly don't see the complexity here

@gquintard
Copy link
Contributor

oh, and we have weekly packages too!

@teohhanhui
Copy link
Contributor Author

@gquintard Init scripts are not useful in a Docker container. Well, the complexity is due to the fact that official Docker images are built for multiarch. And as we can see in the postgres Dockerfile, it's not easy / obvious to see what's going on with all the detection logic.

@teohhanhui teohhanhui force-pushed the add/varnish branch 2 times, most recently from a6b94e8 to dd6883a Compare March 28, 2019 17:31
@teohhanhui
Copy link
Contributor Author

teohhanhui commented Mar 28, 2019

@gquintard @tianon A point against using the upstream packages:

https://packagecloud.io/varnishcache/varnish62 is still empty at time of writing, 2 weeks after Varnish 6.2 was released.

@teohhanhui
Copy link
Contributor Author

Also, it seems like Varnish 6.2.0 still requires patches on Alpine:

https://git.alpinelinux.org/aports/tree/main/varnish?id=d1ee09bdd4d8b028c154d428eabebaa10d9f9686

@gquintard
Copy link
Contributor

not sure those are still necessary, but I'll check

@teohhanhui
Copy link
Contributor Author

@tianon Do you see any outstanding issues? What do I need to do to get the Dockerization review moving?

@gquintard
Copy link
Contributor

https://packagecloud.io/varnishcache/varnish62 is still empty at time of writing, 2 weeks after Varnish 6.2 was released.

fixed

@teohhanhui
Copy link
Contributor Author

@gquintard Yeah, I've noticed. Is it automated now?

@gquintard
Copy link
Contributor

@teohhanhui, it was already automated, but something broke the pipeline and we didn't notice, this shouldn't happen anymore.

@teohhanhui
Copy link
Contributor Author

Do you think it's important to switch to the official package? I'll look into it if you think it's necessary.

@gquintard
Copy link
Contributor

It's really just that the official repo is:

  • official
  • dead simple to install

If there's any issue with it on Docker, I'd rather have the official Docker images use them so we know fast when something goes wrong.

Past that, it's your choice

@teohhanhui
Copy link
Contributor Author

@gquintard @tianon I've done it at coopTilleuls/docker-varnish#42

Would appreciate your review.

I have some concerns about the inconsistency this has caused between both variants of the image though... For example, /var/lib/varnish vs /usr/local/var/varnish

@gquintard
Copy link
Contributor

I'm going to declare myself incompetent on the matter, somehow coopTilleuls/docker-varnish#42 adds more code, and I have no intention of digging through it. The mere fact that the Dockerfile is creating a local APT repo is terrifying.

I have some concerns about the inconsistency this has caused between both variants of the image though... For example, /var/lib/varnish vs /usr/local/var/varnish

Fix the custom built packages to do the same thing as the prepackaged versions? Or just don't build from source?

@teohhanhui
Copy link
Contributor Author

The mere fact that the Dockerfile is creating a local APT repo is terrifying.

That's only for architectures other than amd64, where there is no prebuilt binary.

Or just don't build from source?

Sure, if you provide prebuilt binaries that work on Alpine. I'm not sure if we should install from the official Alpine repository, as chances are they'd be a bit behind.

@gquintard
Copy link
Contributor

gquintard commented May 3, 2019

Sure, if you provide prebuilt binaries that work on Alpine. I'm not sure if we should install from the official Alpine repository, as chances are they'd be a bit behind.

best way I found is to checkout the aports and use the APKBUILD. alpine won't hold onto old packages, but we can reuse their knowledge

please have a look at this: coopTilleuls/docker-varnish/pull/43 , it's certainly not perfect, but it should make things more manageable (I'm monitoring travis to see if I missed anything)

That's only for architectures other than amd64, where there is no prebuilt binary.

While I appreciate the will to cover all the architectures, Varnish was built (no pun intended) from the ground up for 64bits architectures, so the others architectures you can target are arm64, which is possibly a bit out of the ordinary. In any case, I'd rather go for good enough and try to add support for that one later, if there's demand for it.

@gquintard gquintard mentioned this pull request May 5, 2019
9 tasks
@yosifkit
Copy link
Member

yosifkit commented Jul 3, 2019

Closing in favor of #5855.

@yosifkit yosifkit closed this Jul 3, 2019
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.

7 participants