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 RPM packaging for 18.09.x #131

Merged
merged 11 commits into from
Aug 16, 2018
Merged

Conversation

seemethere
Copy link
Contributor

@seemethere seemethere commented Aug 6, 2018

Splits out the docker-ce package and docker-ce-cli package into their
own things.

Includes functionality for running dockerd as a host process that's managed by containerd.

Dependencies include:

Still TODO: (as in stuff for other PR's)

  • Offline installation support (will need a way to dump engine images to oci compliant tar balls)
  • Not hardcoding the engine image in the dockerd.json
  • Not pointing to Seemethere's org (engine image)

Still TODO: need to have a cleanup on the dependencies for the
Dockerfiles
This is done

NOTE: this relies on functionality being added here: docker/cli#1260 There's no dependency on this anymore

Signed-off-by: Eli Uriegas [email protected]

@seemethere seemethere requested review from dhiltgen and a team August 6, 2018 18:17
lightweight container.

Docker containers are both hardware-agnostic and platform-agnostic. This means
they can run anywhere, from your laptop to the largest EC2 compute instance and

Choose a reason for hiding this comment

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

I feel like we have a reworded version of this somewhere.
I could be mistaken, but I think there are things like the largest EC2 instance now says something like largest cloud instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably did, that can be part of a follow up PR by the docs team to align this with our current EE packaging. This PR is meant to get RPM packaging up so we can start testing it with our internal pipelines.

Copy link
Contributor

Choose a reason for hiding this comment

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

I could be mistaken, but I think there are things like the largest EC2 instance now says something like largest cloud instance.

As a data point, I noticed that specific wording change ("largest EC2 instance" → "largest cloud instance") as well when looking at diffs between .spec file versions a few days ago. From my point of view, it definitely makes sense. 😄

Copy link

@jose-bigio jose-bigio left a comment

Choose a reason for hiding this comment

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

Is docker-ce.spec what will be used for the meta package?

For the cli package did you essentially remove all the engine related steps?

If so can we remove some of the packages that are installed in the Dockerfile?

jose-bigio
jose-bigio previously approved these changes Aug 6, 2018
Copy link

@jose-bigio jose-bigio left a comment

Choose a reason for hiding this comment

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

LGTM

Talked to Eli there is a tracking issue to remove the unnecessary dependencies from the Dockerfile.

@justinclift
Copy link
Contributor

justinclift commented Aug 7, 2018

Hmmm, trying this out on my CentOS 7 x64 desktop, the make centos command is outright
failing at the very start with fresh clones of docker and cli repo:

$ export ENGINE_DIR=~/go/src/github.com/docker/engine
$ export CLI_DIR=~/go/src/github.com/docker/cli
$ make centos
mkdir -p rpmbuild/SOURCES
docker run --rm -i -w /v \
        -v /home/jc/git_repos/docker-ce-packaging/rpm/../../cli:/cli \
        -v /home/jc/git_repos/docker-ce-packaging/rpm/rpmbuild/SOURCES:/v \
        alpine \
        tar -C / -c -z -f /v/cli.tgz --exclude .git cli
Unable to find image 'alpine:latest' locally
latest: Pulling from library/alpine
8e3ba11ec2a2: Pull complete 
Digest: sha256:7043076348bf5040220df6ad703798fd8593a0918d06d3ce30c6c93be117e430
Status: Downloaded newer image for alpine:latest
Unknown option: -C
usage: git [--version] [--help] [-c name=value]
           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
           [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
           <command> [<args>]
Unknown option: -C
usage: git [--version] [--help] [-c name=value]
           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
           [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
           <command> [<args>]
Unknown option: -C
usage: git [--version] [--help] [-c name=value]
           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
           [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
           <command> [<args>]
date: invalid date '@'
[etc]

Haven't investigated yet, but will do so shortly.

@justinclift
Copy link
Contributor

On a side note, the previous .spec file included the docker.service file. Neither the new cli nor engine ones seem too now. Not sure if that's on purpose or not. 😉

@seemethere
Copy link
Contributor Author

Yeah that'd be due to the gen-rpm-ver script since the version of git included in CentOS 7 is 6 months behind the patch that actually included git -C. :/

@justinclift
Copy link
Contributor

Thanks @seemethere. I'll try it all in a Fedora 28 VM instead then. That's likely to bundle a much newer git.

@justinclift
Copy link
Contributor

justinclift commented Aug 7, 2018

Ahhh, it looks like newer Git is available in the Red Hat software collections:

    https://www.softwarecollections.org/en/scls/rhscl/rh-git29/

That works. 😄

@justinclift
Copy link
Contributor

Ahhh, grokking this PR now. Looks like a work in progress, with initial focus on getting the cli bits into decent shape and the engine piece can come later.

For that purpose, this seems ok so far. 😄

I'll have a go at building in a minimal CentOS 7 box shortly, to work out the correct BuildRequires dependencies too.

@seemethere seemethere changed the title Add RPM package building for docker-ce-cli WIP Add RPM package building for docker-ce-cli Aug 7, 2018
@justinclift
Copy link
Contributor

For anyone else interested in testing on CentOS 7, these seem to be the essential steps for initial setup from a basic "Minimal" CentOS 7 install:

1. Install newer git than default CentOS provided one

$ sudo yum -y install centos-release-scl yum-utils
$ sudo yum-config-manager --enable rhel-server-rhscl-7-rpms
$ sudo yum -y install rh-git29
$ scl enable rh-git29 bash

2. Install Docker CE

$ sudo yum-config-manager --add-repo=https://download.docker.com/linux/centos/docker-ce.repo
$ sudo yum -y install docker-ce
$ sudo systemctl start docker
$ sudo systemctl enable docker  <-- Only needed if you're planning on rebooting

3. Grab the required Docker source pieces

$ mkdir -p ~/go/src/github.com/docker
$ cd ~/go/src/github.com/docker
$ git clone https://github.com/docker/engine
$ git clone https://github.com/docker/cli
$ git clone https://github.com/docker/seemethere/docker-ce-packaging

That last repo (seemethere/docker-ce-packaging) is only for this PR. For the main docker packaging repo it'll be docker/docker-ce-packaging.

4. Build the rpm

$ cd ~/go/src/github.com/docker/docker-ce-packaging/rpm
$ git checkout rpm_new <-- Only needed for this PR
$ make centos

This info would likely be useful in a wiki section for this repo. Is there any chance of getting the wiki enabled here?

@seemethere seemethere force-pushed the rpm_new branch 2 times, most recently from 8e5ccb0 to f6e7b85 Compare August 13, 2018 23:17
Splits out the docker-ce package and docker-ce-cli package into their
own things.

Still TODO: need to have a cleanup on the dependencies for the
Dockerfiles

Signed-off-by: Eli Uriegas <[email protected]>
Also split off containerd.mk into it's own thing since most of this
stuff will be re-used with debian packaging anyways.

Signed-off-by: Eli Uriegas <[email protected]>
rpmlint checks can be re-added later

Signed-off-by: Eli Uriegas <[email protected]>
@seemethere seemethere force-pushed the rpm_new branch 4 times, most recently from a4a4c21 to 94a1918 Compare August 15, 2018 21:22
Why didn't we do this the whole time? `¯\_(ツ)_/¯`

Signed-off-by: Eli Uriegas <[email protected]>
@seemethere seemethere changed the title WIP Add RPM package building for docker-ce-cli Add RPM package building for docker-ce-cli Aug 16, 2018
@seemethere seemethere changed the title Add RPM package building for docker-ce-cli Add RPM packaging for 18.09.x Aug 16, 2018
Old versions of things on CentOS 7 strike again!

infinity is not a thing for TimeoutSec on systemd < 229

Signed-off-by: Eli Uriegas <[email protected]>
@jose-bigio jose-bigio dismissed their stale review August 16, 2018 16:06

Review is outdated

@@ -0,0 +1,11 @@
{
"image": "docker.io/seemethere/engine-community:0.0.0-20180814124044-678d4b3a6d.x86_64",

Choose a reason for hiding this comment

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

This probably shouldn't be pointing to seemethere's hub org.

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 currently just for development purposes. I'll have a follow up PR after I finish up Deb packaging as well to make this configurable.

SPECS/docker-ce.spec SPECS/docker-ce-cli.spec

SOURCE_FILES=containerd-proxy.tgz cli.tgz containerd-shim-process.tar docker.service dockerd.json
SOURCES=$(addprefix rpmbuild/SOURCES/, $(SOURCE_FILES))

Choose a reason for hiding this comment

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

Pretty cool

Copy link

@jose-bigio jose-bigio left a comment

Choose a reason for hiding this comment

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

Confused about the docker-ce.spec logic.

@@ -45,49 +45,49 @@ debian: debian-stretch debian-jessie ## build all debian deb packages
raspbian: raspbian-stretch debian-jessie ## build all raspbian deb packages

.PHONY: ubuntu-xenial
ubuntu-xenial: engine-$(ARCH).tar ## build ubuntu xenial deb packages

Choose a reason for hiding this comment

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

Ideally these changes would be in a separate request.
(These aren't RPM changes)

# clear any old state
rm -f %{_localstatedir}/lib/rpm-state/docker-is-active > /dev/null 2>&1 || :

# check if docker service is running

Choose a reason for hiding this comment

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

Can you provide more context to your comments?
Like stop docker service if it is running.

# check if docker service is running
if systemctl is-active docker > /dev/null 2>&1; then
systemctl stop docker > /dev/null 2>&1 || :
touch %{_localstatedir}/lib/rpm-state/docker-is-active > /dev/null 2>&1 || :

Choose a reason for hiding this comment

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

Why is docker being stopped but the docker-is-active file is being touched?

# check if docker was running before upgrade
if [ -f %{_localstatedir}/lib/rpm-state/docker-is-active ]; then
systemctl start docker > /dev/null 2>&1 || :
rm -f %{_localstatedir}/lib/rpm-state/docker-is-active > /dev/null 2>&1 || :

Choose a reason for hiding this comment

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

Why is the is active file being removed?
Isn't docker active after systemctl start docker?

Copy link

@jose-bigio jose-bigio left a comment

Choose a reason for hiding this comment

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

Talked with Eli changes that I were confused about have been part of spec file since before these changes.

LGTM!

Copy link
Contributor

@corbin-coleman corbin-coleman left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants