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 --image-repository flag so that users can select an alternative repository mirror #3714

Merged
merged 9 commits into from Mar 22, 2019

Conversation

@laozc
Copy link
Contributor

laozc commented Feb 18, 2019

This PR is aiming to support a new command line argument --image-repository for some users which has difficulty accessing gcr.io.

Closes #3166

laozc added 3 commits Feb 17, 2019
Some users (especially for those in mainland China) may have issue
accessing the default image repository. This patchset allows users
to override the default image repository gcr.io to a different
repository by specifying --image-repository option in the command
line as a simple workaround. Images will be pulled from the
specified image repository instead of the default ones.

Example (using mirror by Aliyun):
minikube start ...
   --image-repository
   registry.cn-hangzhou.aliyuncs.com/google_containers
When the user overrides image repository the images will be pulled
from the overrided one instead of the official repositories.
@minikube-bot

This comment has been minimized.

Copy link
Collaborator

minikube-bot commented Feb 18, 2019

Can one of the admins verify this patch?

@laozc laozc force-pushed the laozc:custom-image-repository branch from 851ee5f to e753a90 Feb 18, 2019
@afbjorklund afbjorklund requested a review from tstromberg Feb 18, 2019
@tstromberg

This comment has been minimized.

Copy link
Contributor

tstromberg commented Feb 19, 2019

Thank you so much for writing this. This PR could potentially make minikube work much better in China, as well as environments with limited internet connectivity. I'll give this a closer look tomorrow, but at least initially this seems to be doing the right things.

Do you have a mirror or two to recommend that I could test this PR against?

One potential concern to eventually address is cache-poisoning, which will require minikube addressing images with hashes instead of versions -- but I won't let "perfect be the enemy of good".

A potential eventual enhancement would be to have an automated fall-back mechanism between a set of known-good image repositories (similar to "apt" or other systems), so that minikube in users in China would not have to manually specify a --image-repository URL unless they wish to.

Thanks again!

@tstromberg

This comment has been minimized.

Copy link
Contributor

tstromberg commented Feb 19, 2019

@minikube-bot OK to test

@laozc

This comment has been minimized.

Copy link
Contributor Author

laozc commented Feb 20, 2019

You may start with registry.cn-hangzhou.aliyuncs.com/google_containers.
This is one of the well known mirrors for gcr.io maintained by Aliyun guys.
I verified with this mirror so it should work as expected (v1.13.3).

We may use --image-repository as an ultimate workaround
if none of mirrors provided is accessible in user's environment.
I agree that maintaining a mirror list as like the APT / pacman world also helps,
or even trying to guess a reliable mirror by IP address during the setup.
I can work on that later.

Let me know if you have any other concerns.

@tstromberg

This comment has been minimized.

Copy link
Contributor

tstromberg commented Feb 20, 2019

How does this new --image-repository flag interact with the existing --registry-mirror flag?

--registry-mirror - Registry mirrors to pass to the Docker daemon

I'll admit, I didn't know about this flag up until today. I haven't looked into it's implementation yet to see if it's compatible with non-docker container runtimes, which your flag seems to allow for.

@laozc

This comment has been minimized.

Copy link
Contributor Author

laozc commented Feb 21, 2019

I believe --registry-mirror only impacts those images with no repository prefix - images that come from the Docker official registry.
It won't work on images from private registries, which is the case like gcr.io/kube-proxy. For private images, docker will still go to the private registry and fetch them.

If you set up minikube with --registry-mirror, it could work for pods/deployments that use ubuntu/18.04, but not for gcr.io/ - you will need to wipe the "gcr.io/" out from the references to make it look like an image from the official registry. For the latter case, it could be achieved using --registry-mirror https://private_server --image-registry [private_server/]google_containers

@afbjorklund

This comment has been minimized.

Copy link
Contributor

afbjorklund commented Feb 23, 2019

For instance crio has a --registryflag, but that only works for images without prefix ("unqualified"):

   --registry value              registry to be prepended when pulling unqualified images, can be specified multiple times

The same goes for the global /etc/containers/registries.conf, which lists the default registries:

[registries.search]
registries = ['docker.io']

[registries.insecure]
registries = []

And /etc/crio/crio.conf with CRI-O's:

# insecure_registries is used to skip TLS verification when pulling images.
insecure_registries = [
]

# registries is used to specify a comma separated list of registries to be used
# when pulling an unqualified image (e.g. fedora:rawhide).
registries = [
        "docker.io"
]

As stated above, this would not help for the k8s.gcr.io images.

@tstromberg tstromberg changed the title Custom image repository Add --image-repository flag so that users can select an alternative repository mirror Feb 25, 2019
@tstromberg

This comment has been minimized.

Copy link
Contributor

tstromberg commented Feb 28, 2019

/approve

Thank you for the clarification and excellent work here. I've added a few nitpicks, but it's good enough as is. This is really going to help minikube users around the world.

We're going to want to add an integration test for this, but until #3767 lands it's rather painful to add start arguments. We can do that in a follow-up PR.

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved
@tstromberg

This comment has been minimized.

Copy link
Contributor

tstromberg commented Mar 6, 2019

@minikube-bot OK to test

1 similar comment
@tstromberg

This comment has been minimized.

Copy link
Contributor

tstromberg commented Mar 13, 2019

@minikube-bot OK to test

@laozc

This comment has been minimized.

Copy link
Contributor Author

laozc commented Mar 18, 2019

Have some other work based on this PR.
Any chance to have it merged sooner?
Let me know if you have any concerns.

@tstromberg

This comment has been minimized.

Copy link
Contributor

tstromberg commented Mar 21, 2019

@minikube-bot OK to test

@laozc laozc force-pushed the laozc:custom-image-repository branch from 9a3d6b4 to 859bf1c Mar 21, 2019
@laozc laozc force-pushed the laozc:custom-image-repository branch from 859bf1c to daeb238 Mar 21, 2019
@tstromberg

This comment has been minimized.

Copy link
Contributor

tstromberg commented Mar 21, 2019

I think it may have been my attempt at merging to master, but it appears that "minikube dashboard" is broken with this PR at the moment.

@tstromberg

This comment has been minimized.

Copy link
Contributor

tstromberg commented Mar 22, 2019

Do you mind remerging this PR? I'd like to also confirm that minikube dashboard still works.

@laozc

This comment has been minimized.

Copy link
Contributor Author

laozc commented Mar 22, 2019

I'm checking the case you describe and looking into the PR.

@laozc laozc force-pushed the laozc:custom-image-repository branch 2 times, most recently from b69e061 to 481f9b5 Mar 22, 2019
@laozc laozc force-pushed the laozc:custom-image-repository branch from 481f9b5 to daec030 Mar 22, 2019
@tstromberg

This comment has been minimized.

Copy link
Contributor

tstromberg commented Mar 22, 2019

Thanks for making the changes!

/lgtm /approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 22, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: laozc, tstromberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tstromberg tstromberg merged commit 702d471 into kubernetes:master Mar 22, 2019
5 of 10 checks passed
5 of 10 checks passed
Linux-KVM Jenkins
Details
Linux-None Jenkins
Details
Linux-Virtualbox Jenkins
Details
OSX-Hyperkit Jenkins
Details
tide Not mergeable. Needs lgtm label.
Details
Jenkins Cross Build Build finished. No test results found.
Details
Linux-VirtualBox Jenkins
Details
OSX-Virtualbox Jenkins
Details
cla/linuxfoundation laozc authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laozc

This comment has been minimized.

Copy link
Contributor Author

laozc commented Jul 23, 2019

@AdnanHodzic

This comment has been minimized.

Copy link

AdnanHodzic commented Jul 23, 2019

@laozc got it, thank you for the clarification!

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

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.