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

Percona 5.7 docker image updates #2113

Merged
merged 7 commits into from Oct 5, 2016

Conversation

rnavarro
Copy link
Contributor

@rnavarro rnavarro commented Oct 5, 2016

  • Add a version to the percona 5.7 image for updates
  • Update documentation to reference percona 5.7

@michael-berlin michael-berlin self-assigned this Oct 5, 2016
@@ -11,7 +11,7 @@ RUN apt-key adv --keyserver ha.pool.sks-keyservers.net \
} | debconf-set-selections && \
apt-get update && \
apt-get install -y --no-install-recommends \
percona-server-server-5.7 libperconaserverclient18.1-dev && \
percona-server-server-5.7=5.7.14* libperconaserverclient18.1-dev && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you want to pin it to a specific minor version?

Wouldn't it be better if we don't pin it by default? This would be less surprising: a rebuild of the image would always get you the latest version.

We regularly rebuild and test the images. Therefore, I'm not concerned about regressions between minor versions.

Copy link
Contributor Author

@rnavarro rnavarro Oct 5, 2016

Choose a reason for hiding this comment

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

This was actually done as a cache busting mechanism.

I noticed on our local builds, when following the instructions, we were just using the cached versions of percona, which were old.

I added this so that we could pin to the latest stable and force an update when it changes.

I agree that no pinning should be necessary, but I'm not entirely sure how to get around the caching issues more effectively.

Additional workflow advice would be most helpful here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given what you're trying to achieve I'm not in favor of this change.

I would prefer if we can fix the original problem which is that always the latest version will be installed when you run docker build.

In the past, when I used Debian, the following steps always worked to update a package:

apt-get update
apt-get install <package>

In general, this still seems to be true. But here's a report that it doesn't work with external repositories: http://askubuntu.com/a/525686

But it looks to me like this happens only when there are multiple repositories which provide the package. In that case, the priority of the repository defines which one is used.

Given that I would still think that a plain apt-get install should update your package in our case. After all, there's only one repository which provides the percona packages?

Can you think of a way to reproduce the problem?

As far as I can tell, there's currently no way to reproduce it? I pushed newer images earlier this morning and they have already the 5.7.14-8-1.jessie package installed i.e. I cannot recreate the situation where older packages are installed and docker build should update the image.

btw: I'm in favor of the other changes in the PR. Just not the version pinning in the Dockerfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moby/moby#3313

That's the issue I was referencing, it's an issue with docker not the apt package system.

But, hidden in the weeds is to do:

Issue #3375 has been fixed and Docker 0.7.5 doesn't have that problem.

As for the rest of the problems described in this issue, using --no-cache during docker build is the right solution.

I'll back out this change and use the --no-cache when doing our builds to prevent this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, now I get it:

When the RUN command does not change, Docker does not rebuild the image for that particular command. That makes sense. If I understand it correctly, your change would have worked once because the changed RUN command triggers a rebuild. When a newer version comes out, you would have to change the RUN command again.

Given that, I agree that --no-cache is the way to go here. In fact, our script docker/bootstrap/build.sh uses that option as well. And that explains why the images I rebuilt today have the latest version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I didn't see the --no-cache in the docker/bootstrap/build.sh, good catch!.

Should we add that option to the docker/lite/build.sh script on lines 45 and 47? I think that would solve the problem once and for all.

We build our images with make docker_lite_percona57

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, looks like it.

Can you please create a separate PR for that and we can ask @enisoc for his opinion on that?

The lite images are used within Kubernetes for example. I think it makes sense that we fully rebuild them with the latest MySQL version when we update the Vitess binaries in there.

@michael-berlin
Copy link
Contributor

michael-berlin commented Oct 5, 2016

LGTM

Approved with PullApprove

@michael-berlin michael-berlin merged commit 3ade15b into vitessio:master Oct 5, 2016
@rnavarro rnavarro deleted the docker-image-updates branch October 6, 2016 00:02
@rnavarro rnavarro mentioned this pull request Oct 6, 2016
frouioui pushed a commit to planetscale/vitess that referenced this pull request Nov 21, 2023
Signed-off-by: Manan Gupta <manan@planetscale.com>
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.

None yet

3 participants