Ubuntu Docker Images

Code review comment for ~ubuntu-docker-images/ubuntu-docker-images/+git/telegraf:20.04-resync-with-21.04-dockerfile

Git
lp:~ubuntu-docker-images/ubuntu-docker-images/+git/telegraf
20.04-resync-with-21.04-dockerfile
Merge into edge

Revision history for this message

Bryce Harrington (bryce) wrote on 2021-03-25:

On Wed, Mar 24, 2021 at 10:47:12PM -0000, Sergio Durigan Junior wrote:
> Review: Approve
>
> Thanks for the MP, Bryce.
>
> I am leaving a few comments here and there for small nits I found. I built and tested the image, and everything seems OK (modulo the manifest thing; see below).
>
> For 21.04, since we have telegraf in the archive, the build process is much simpler because it's just a matter of apt-get installing the package. For 20.04, the situation is a bit more complex. We are still working with the SRU team in order to include telegraf into Focal, but it's not there yet. However, we do have the (yet to be) Focal telegraf package in a repository, so that's what the Dockerfile uses to build things (we could perhaps maintain this in a PPA, but I decided to just build everything from source). That's why you see the differences between the two releases.
>
> For the manifest generation, as I mentioned below, I can look into it if you'd like. It's not entirely trivial and I already did it for 21.04, so it will be faster, I think. You can leave the existing hunk that generates the manifest from dpkg, because that will be useful as well.
>

Thanks, that'd be great. I pushed it as is, you can fix up at your leisure.

> I'm approving this, so after you fix the issues I pointed, feel free to merge it. Thanks.
>
> Diff comments:
>
> > diff --git a/Dockerfile b/Dockerfile
> > index 97b39b7..b99f9b1 100644
> > --- a/Dockerfile
> > +++ b/Dockerfile
> > @@ -1,37 +1,47 @@
> > -FROM ubuntu:focal AS builder
> > +FROM ubuntu:focal AS manifest-builder
>
> The name should remain "builder", since it's being used to build the full package, not only the manifest.

Done

> > COPY . .
> >
> > WORKDIR /src/telegraf
> >
> > -RUN apt-get update \
> > - && DEBIAN_FRONTEND=noninteractive apt-get upgrade -y \
> > - && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends git dpkg-dev ca-certificates pristine-tar \
> > - && git clone https://git.launchpad.net/~ubuntu-server/ubuntu/+source/telegraf \
> > - && cd telegraf \
> > - && git branch pristine-tar origin/pristine-tar \
> > - && git checkout focal \
> > - && cp -a /adjust-telegraf-conf.patch debian/patches \
> > - && echo "adjust-telegraf-conf.patch" >> debian/patches/series \
> > - && sed -i 's@^EXCLUDE_TESTS.*@EXCLUDE_TESTS = github.com/influxdata/telegraf/plugins/inputs/http_response \
> > +RUN set -eux; \
> > + apt-get update; \
> > + DEBIAN_FRONTEND=noninteractive apt-get full-upgrade -y; \
> > + DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
> > + dpkg-dev git ca-certificates pristine-tar; \
> > + git clone https://git.launchpad.net/~ubuntu-server/ubuntu/+source/telegraf; \
> > + cd telegraf; \
> > + git branch pristine-tar origin/pristine-tar; \
> > + git checkout focal; \
> > + cp -a /adjust-telegraf-conf.patch debian/patches/; \
> > + echo "adjust-telegraf-conf.patch" >> debian/patches/series; \
> > + sed -i 's@^EXCLUDE_TESTS.*@EXCLUDE_TESTS = github.com/influxdata/telegraf/plugins/inputs/http_response \
> > github.com/influxdata/telegraf/plugins/inputs/zipkin \
> > -github.com/influxdata/telegraf/plugins/outputs/prometheus_client \\@' debian/rules \
> > - && pristine-tar checkout ../telegraf_$(dpkg-parsechangelog -SVersion | sed 's/$.*$-.ubuntu.*/\1/').orig.tar.xz \
> > - && DEBIAN_FRONTEND=noninteractive apt-get build-dep . -y \
> > - && dpkg-buildpackage -us -uc
> > +github.com/influxdata/telegraf/plugins/outputs/prometheus_client \\@' debian/rules; \
> > + pristine-tar checkout ../telegraf_$(dpkg-parsechangelog -SVersion | sed 's/$.*$-.ubuntu.*/\1/').orig.tar.xz; \
> > + DEBIAN_FRONTEND=noninteractive apt-get build-dep . -y; \
> > + dpkg-buildpackage -us -uc
> >
> > FROM ubuntu:focal
> >
> > ENV TZ UTC
> >
> > -COPY --from=builder /src/telegraf/telegraf*.deb /pkg/
> > +COPY --from=manifest-builder /src/telegraf/telegraf*.deb /pkg/
>
> Just a reminder: when you revert the renaming of the container on line 1, you will have to update here as well.

Done

> > -RUN apt-get update \
> > - && DEBIAN_FRONTEND=noninteractive apt-get upgrade -y \
> > - && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends /pkg/telegraf*.deb tzdata \
> > +RUN set -eux; \
> > + apt-get update; \
> > + DEBIAN_FRONTEND=noninteractive apt-get full-upgrade -y; \
> > + DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
> > + tzdata; \
> > + dpkg -i /pkg/telegraf*.deb; \
>
> Huh, I could not reproduce the issue with "apt-get install /pkg/telegraf*.deb": everything worked fine here. If you look at the logs for the previous 20.04 builds, you can check that doing "apt-get install /pkg/telegraf*.deb" works fine in the LP build machines as well. I wonder why you're seeing this error.
>

I backed out this change and it built fine.
I think I may have triggered the issue with an incorrect refactoring,
that I then fixed separately after adding the dpkg code.

apt-get uses dpkg -i internally, so the install should be equivalent.
However, apt-get also does some dependency resolution that dpkg -i does
not do. So the apt-get form is better.

> > # We don't need to keep the .deb packages around
> > - && rm -rf /pkg \
> > - && rm -rf /var/lib/apt/lists/*
> > + rm -rf /pkg; \
> > + rm -rf /var/lib/apt/lists/*; \
> > +# smoke test
> > + telegraf --version; \
>
> Different indentation (spaces instead of TABs) here and below.

Got it.

> > +# create manifest
> > + mkdir -p /usr/share/rocks; \
> > + (echo "# os-release" && cat /etc/os-release && echo "# dpkg-query" && dpkg-query -f '${db:Status-Abbrev},${binary:Package},${Version},${source:Package},${Source:Version}\n' -W) > /usr/share/rocks/dpkg.query
>
> Hm, this part is going to be a bit more complicated. We will need to use the golang-manifest-builder script here as well. The idea is to generate the manifest files in the "builder" container (after the package has built), and then copy the files over here, just like we do in the 21.04 image.
>

Thanks for handling that part.
Bryce

On Wed, Mar 24, 2021 at 10:47:12PM -0000, Sergio Durigan Junior wrote:
> Review: Approve
> 
> Thanks for the MP, Bryce.
> 
> I am leaving a few comments here and there for small nits I found.  I built and tested the image, and everything seems OK (modulo the manifest thing; see below).
> 
> For 21.04, since we have telegraf in the archive, the build process is much simpler because it's just a matter of apt-get installing the package.  For 20.04, the situation is a bit more complex.  We are still working with the SRU team in order to include telegraf into Focal, but it's not there yet.  However, we do have the (yet to be) Focal telegraf package in a repository, so that's what the Dockerfile uses to build things (we could perhaps maintain this in a PPA, but I decided to just build everything from source).  That's why you see the differences between the two releases.
> 
> For the manifest generation, as I mentioned below, I can look into it if you'd like.  It's not entirely trivial and I already did it for 21.04, so it will be faster, I think.  You can leave the existing hunk that generates the manifest from dpkg, because that will be useful as well.
>

Thanks, that'd be great.  I pushed it as is, you can fix up at your leisure.

> I'm approving this, so after you fix the issues I pointed, feel free to merge it.  Thanks.
> 
> Diff comments:
> 
> > diff --git a/Dockerfile b/Dockerfile
> > index 97b39b7..b99f9b1 100644
> > --- a/Dockerfile
> > +++ b/Dockerfile
> > @@ -1,37 +1,47 @@
> > -FROM ubuntu:focal AS builder
> > +FROM ubuntu:focal AS manifest-builder
> 
> The name should remain "builder", since it's being used to build the full package, not only the manifest.

Done

> >  COPY . .
> >  
> >  WORKDIR /src/telegraf
> >  
> > -RUN apt-get update \
> > -	&& DEBIAN_FRONTEND=noninteractive apt-get upgrade -y \
> > -	&& DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends git dpkg-dev ca-certificates pristine-tar \
> > -	&& git clone https://git.launchpad.net/~ubuntu-server/ubuntu/+source/telegraf \
> > -	&& cd telegraf \
> > -	&& git branch pristine-tar origin/pristine-tar \
> > -	&& git checkout focal \
> > -	&& cp -a /adjust-telegraf-conf.patch debian/patches \
> > -	&& echo "adjust-telegraf-conf.patch" >> debian/patches/series \
> > -	&& sed -i 's@^EXCLUDE_TESTS.*@EXCLUDE_TESTS = github.com/influxdata/telegraf/plugins/inputs/http_response \
> > +RUN set -eux; \
> > +	apt-get update; \
> > +	DEBIAN_FRONTEND=noninteractive apt-get full-upgrade -y; \
> > +	DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
> > +		dpkg-dev git ca-certificates pristine-tar; \
> > +	git clone https://git.launchpad.net/~ubuntu-server/ubuntu/+source/telegraf; \
> > +	cd telegraf; \
> > +	git branch pristine-tar origin/pristine-tar; \
> > +	git checkout focal; \
> > +	cp -a /adjust-telegraf-conf.patch debian/patches/; \
> > +	echo "adjust-telegraf-conf.patch" >> debian/patches/series; \
> > +	sed -i 's@^EXCLUDE_TESTS.*@EXCLUDE_TESTS = github.com/influxdata/telegraf/plugins/inputs/http_response \
> >  github.com/influxdata/telegraf/plugins/inputs/zipkin \
> > -github.com/influxdata/telegraf/plugins/outputs/prometheus_client \\@' debian/rules \
> > -	&& pristine-tar checkout ../telegraf_$(dpkg-parsechangelog -SVersion | sed 's/$.*$-.ubuntu.*/\1/').orig.tar.xz \
> > -	&& DEBIAN_FRONTEND=noninteractive apt-get build-dep . -y \
> > -	&& dpkg-buildpackage -us -uc
> > +github.com/influxdata/telegraf/plugins/outputs/prometheus_client \\@' debian/rules; \
> > +	pristine-tar checkout ../telegraf_$(dpkg-parsechangelog -SVersion | sed 's/$.*$-.ubuntu.*/\1/').orig.tar.xz; \
> > +	DEBIAN_FRONTEND=noninteractive apt-get build-dep . -y; \
> > +	dpkg-buildpackage -us -uc
> >  
> >  FROM ubuntu:focal
> >  
> >  ENV TZ UTC
> >  
> > -COPY --from=builder /src/telegraf/telegraf*.deb /pkg/
> > +COPY --from=manifest-builder /src/telegraf/telegraf*.deb /pkg/
> 
> Just a reminder: when you revert the renaming of the container on line 1, you will have to update here as well.

Done

> > -RUN apt-get update \
> > -	&& DEBIAN_FRONTEND=noninteractive apt-get upgrade -y \
> > -	&& DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends /pkg/telegraf*.deb tzdata \
> > +RUN set -eux; \
> > +	apt-get update; \
> > +	DEBIAN_FRONTEND=noninteractive apt-get full-upgrade -y; \
> > +	DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
> > +		tzdata; \
> > +	dpkg -i /pkg/telegraf*.deb; \
> 
> Huh, I could not reproduce the issue with "apt-get install /pkg/telegraf*.deb": everything worked fine here.  If you look at the logs for the previous 20.04 builds, you can check that doing "apt-get install /pkg/telegraf*.deb" works fine in the LP build machines as well.  I wonder why you're seeing this error.
>

I backed out this change and it built fine.
I think I may have triggered the issue with an incorrect refactoring,
that I then fixed separately after adding the dpkg code.

apt-get uses dpkg -i internally, so the install should be equivalent.
However, apt-get also does some dependency resolution that dpkg -i does
not do.  So the apt-get form is better.

> >  # We don't need to keep the .deb packages around
> > -	&& rm -rf /pkg \
> > -	&& rm -rf /var/lib/apt/lists/*
> > +	rm -rf /pkg; \
> > +	rm -rf /var/lib/apt/lists/*; \
> > +# smoke test
> > +    telegraf --version; \
> 
> Different indentation (spaces instead of TABs) here and below.

Got it.

> > +# create manifest
> > +    mkdir -p /usr/share/rocks; \
> > +    (echo "# os-release" && cat /etc/os-release && echo "# dpkg-query" && dpkg-query -f '${db:Status-Abbrev},${binary:Package},${Version},${source:Package},${Source:Version}\n' -W) > /usr/share/rocks/dpkg.query
> 
> Hm, this part is going to be a bit more complicated.  We will need to use the golang-manifest-builder script here as well.  The idea is to generate the manifest files in the "builder" container (after the package has built), and then copy the files over here, just like we do in the 21.04 image.
>

Thanks for handling that part.
Bryce

« Back to merge proposal