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

honor --image-repository even if --image-mirror-country is set #8249

Merged
merged 1 commit into from
May 29, 2020

Conversation

SataQiu
Copy link
Member

@SataQiu SataQiu commented May 22, 2020

Currently, if I specify both --image-mirror-country and --image-repository, then --image-repository will be ignored. I think in this case, we still need to respect the user's choice about image repository.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SataQiu
To complete the pull request process, please assign sharifelgamal
You can assign the PR to them by writing /assign @sharifelgamal in a comment when ready.

The full list of commands accepted by this bot can be found 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

@codecov-commenter
Copy link

Codecov Report

Merging #8249 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8249   +/-   ##
=======================================
  Coverage   34.46%   34.46%           
=======================================
  Files         147      147           
  Lines        9428     9426    -2     
=======================================
  Hits         3249     3249           
+ Misses       5780     5779    -1     
+ Partials      399      398    -1     
Impacted Files Coverage Δ
cmd/minikube/cmd/start_flags.go 50.15% <100.00%> (+0.31%) ⬆️

Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

this look like doing the right thing, do you mind verifying that there might be a related code in
updateExistingConfigFromFlags

that would be if user already started a cluster with a config, and then if they only do
"minikube start" without any args, we should respect the flags that was used in first start

@SataQiu
Copy link
Member Author

SataQiu commented May 24, 2020

I checked the logic about updateExistingConfigFromFlags in https://github.com/kubernetes/minikube/blob/master/cmd/minikube/cmd/start_flags.go#L544-L546. It only deals with --image-repository flag update, but does not deal with --image-mirror-country flag.

	if cmd.Flags().Changed(imageRepository) {
		cc.KubernetesConfig.ImageRepository = viper.GetString(imageRepository)
	}

I think it is fine, we should not change the image-mirror-country of an existing cluster.

If the user specifies a new --image-repository flag, minikube will use the new flag value.
But if they only do "minikube start" without any args, we will respect the flags that were used in the first start.

Here is my test:

$ ./minikube start --image-repository registry.aliyuncs.com/google_containers --image-mirror-country=cn
😄  Darwin 10.15.4 上的 minikube v1.10.1
✨  Automatically selected the hyperkit driver. Other choices: docker, parallels
💾  正在下载驱动 docker-machine-driver-hyperkit:
    > docker-machine-driver-hyperkit.sha256: 65 B / 65 B [---] 100.00% ? p/s 0s
    > docker-machine-driver-hyperkit: 10.90 MiB / 10.90 MiB  100.00% 68.58 KiB 
🔑  The 'hyperkit' driver requires elevated permissions. The following commands will be executed:

    $ sudo chown root:wheel /Users/shida/.minikube/bin/docker-machine-driver-hyperkit 
    $ sudo chmod u+s /Users/shida/.minikube/bin/docker-machine-driver-hyperkit 


Password:
✅  正在使用镜像存储库 registry.aliyuncs.com/google_containers
💿  正在下载 VM boot image...
    > minikube-v1.10.0.iso.sha256: 65 B / 65 B [-------------] 100.00% ? p/s 0s
    > minikube-v1.10.0.iso: 174.99 MiB / 174.99 MiB  100.00% 1.84 MiB p/s 1m35s
👍  Starting control plane node minikube in cluster minikube
🔥  Creating hyperkit VM (CPUs=2, Memory=4000MB, Disk=20000MB) ...
🐳  正在 Docker 19.03.8 中准备 Kubernetes v1.18.2…
    > kubelet.sha256: 65 B / 65 B [--------------------------] 100.00% ? p/s 0s
    > kubeadm.sha256: 65 B / 65 B [--------------------------] 100.00% ? p/s 0s
    > kubectl.sha256: 65 B / 65 B [--------------------------] 100.00% ? p/s 0s
    > kubectl: 41.99 MiB / 41.99 MiB [---------------] 100.00% 2.35 MiB p/s 18s
    > kubeadm: 37.97 MiB / 37.97 MiB [---------------] 100.00% 1.71 MiB p/s 23s
    > kubelet: 108.03 MiB / 108.03 MiB [-------------] 100.00% 2.34 MiB p/s 47s
🔎  Verifying Kubernetes components...
🌟  Enabled addons: default-storageclass, storage-provisioner
🏄  完成!kubectl 已经配置至 "minikube"

❗  /usr/local/bin/kubectl is version 1.14.7, which may be incompatible with Kubernetes 1.18.2.
💡  You can also use 'minikube kubectl -- get pods' to invoke a matching version
$ kubectl get pod -A
NAMESPACE     NAME                               READY   STATUS    RESTARTS   AGE
kube-system   coredns-7ff77c879f-n964v           1/1     Running   0          8s
kube-system   coredns-7ff77c879f-rpgk9           1/1     Running   0          8s
kube-system   etcd-minikube                      1/1     Running   0          14s
kube-system   kube-apiserver-minikube            1/1     Running   0          14s
kube-system   kube-controller-manager-minikube   1/1     Running   0          14s
kube-system   kube-proxy-ljhnz                   1/1     Running   0          9s
kube-system   kube-scheduler-minikube            1/1     Running   0          14s
kube-system   storage-provisioner                1/1     Running   1          14s
$ ./minikube stop
✋  Stopping "minikube" in hyperkit ...
🛑  Node "minikube" stopped.
$ ./minikube start
😄  Darwin 10.15.4 上的 minikube v1.10.1
✨  根据现有的配置文件使用 hyperkit 驱动程序
👍  Starting control plane node minikube in cluster minikube
🔄  Restarting existing hyperkit VM for "minikube" ...
🐳  正在 Docker 19.03.8 中准备 Kubernetes v1.18.2…
🔎  Verifying Kubernetes components...
🌟  Enabled addons: default-storageclass, storage-provisioner
🏄  完成!kubectl 已经配置至 "minikube"

❗  /usr/local/bin/kubectl is version 1.14.7, which may be incompatible with Kubernetes 1.18.2.
💡  You can also use 'minikube kubectl -- get pods' to invoke a matching version
$ kubectl get pod -A
NAMESPACE     NAME                               READY   STATUS             RESTARTS   AGE
kube-system   coredns-7ff77c879f-n964v           1/1     Running            1          91s
kube-system   coredns-7ff77c879f-rpgk9           1/1     Running            1          91s
kube-system   etcd-minikube                      1/1     Running            1          97s
kube-system   kube-apiserver-minikube            1/1     Running            1          97s
kube-system   kube-controller-manager-minikube   1/1     Running            1          97s
kube-system   kube-proxy-ljhnz                   1/1     Running            1          92s
kube-system   kube-scheduler-minikube            1/1     Running            1          97s
kube-system   storage-provisioner                0/1     CrashLoopBackOff   2          97s
$ ./minikube ssh
                         _             _            
            _         _ ( )           ( )           
  ___ ___  (_)  ___  (_)| |/')  _   _ | |_      __  
/' _ ` _ `\| |/' _ `\| || , <  ( ) ( )| '_`\  /'__`\
| ( ) ( ) || || ( ) || || |\`\ | (_) || |_) )(  ___/
(_) (_) (_)(_)(_) (_)(_)(_) (_)`\___/'(_,__/'`\____)

$ 
$ 
$ 
$ docker images
REPOSITORY                                                        TAG                 IMAGE ID            CREATED             SIZE
registry.aliyuncs.com/google_containers/dashboard                 v2.0.0              8b32422733b3        4 weeks ago         222MB
registry.aliyuncs.com/google_containers/kube-proxy                v1.18.2             0d40868643c6        5 weeks ago         117MB
registry.aliyuncs.com/google_containers/kube-scheduler            v1.18.2             a3099161e137        5 weeks ago         95.3MB
registry.aliyuncs.com/google_containers/kube-controller-manager   v1.18.2             ace0a8c17ba9        5 weeks ago         162MB
registry.aliyuncs.com/google_containers/kube-apiserver            v1.18.2             6ed75ad404bd        5 weeks ago         173MB
registry.aliyuncs.com/google_containers/pause                     3.2                 80d28bedfe5d        3 months ago        683kB
registry.aliyuncs.com/google_containers/coredns                   1.6.7               67da37a9a360        3 months ago        43.8MB
registry.aliyuncs.com/google_containers/etcd                      3.4.3-0             303ce5db0e90        7 months ago        288MB
registry.aliyuncs.com/google_containers/metrics-scraper           v1.0.2              3b08661dc379        7 months ago        40.1MB
registry.aliyuncs.com/google_containers/storage-provisioner       v1.8.1              4689081edb10        2 years ago         80.8MB
$ ./minikube stop
✋  Stopping "minikube" in hyperkit ...
🛑  Node "minikube" stopped.
$ ./minikube start --image-repository registry.cn-hangzhou.aliyuncs.com/google_containers
😄  Darwin 10.15.4 上的 minikube v1.10.1
✨  根据现有的配置文件使用 hyperkit 驱动程序
👍  Starting control plane node minikube in cluster minikube
🔄  Restarting existing hyperkit VM for "minikube" ...
🐳  正在 Docker 19.03.8 中准备 Kubernetes v1.18.2…
🔎  Verifying Kubernetes components...
🌟  Enabled addons: default-storageclass, storage-provisioner
🏄  完成!kubectl 已经配置至 "minikube"

❗  /usr/local/bin/kubectl is version 1.14.7, which may be incompatible with Kubernetes 1.18.2.
💡  You can also use 'minikube kubectl -- get pods' to invoke a matching version
$ ./minikube ssh
                         _             _            
            _         _ ( )           ( )           
  ___ ___  (_)  ___  (_)| |/')  _   _ | |_      __  
/' _ ` _ `\| |/' _ `\| || , <  ( ) ( )| '_`\  /'__`\
| ( ) ( ) || || ( ) || || |\`\ | (_) || |_) )(  ___/
(_) (_) (_)(_)(_) (_)(_)(_) (_)`\___/'(_,__/'`\____)

$ docker images
REPOSITORY                                                                    TAG                 IMAGE ID            CREATED             SIZE
registry.aliyuncs.com/google_containers/dashboard                             v2.0.0              8b32422733b3        4 weeks ago         222MB
registry.cn-hangzhou.aliyuncs.com/google_containers/dashboard                 v2.0.0              8b32422733b3        4 weeks ago         222MB
registry.aliyuncs.com/google_containers/kube-proxy                            v1.18.2             0d40868643c6        5 weeks ago         117MB
registry.cn-hangzhou.aliyuncs.com/google_containers/kube-proxy                v1.18.2             0d40868643c6        5 weeks ago         117MB
registry.cn-hangzhou.aliyuncs.com/google_containers/kube-controller-manager   v1.18.2             ace0a8c17ba9        5 weeks ago         162MB
registry.aliyuncs.com/google_containers/kube-controller-manager               v1.18.2             ace0a8c17ba9        5 weeks ago         162MB
registry.aliyuncs.com/google_containers/kube-apiserver                        v1.18.2             6ed75ad404bd        5 weeks ago         173MB
registry.cn-hangzhou.aliyuncs.com/google_containers/kube-apiserver            v1.18.2             6ed75ad404bd        5 weeks ago         173MB
registry.aliyuncs.com/google_containers/kube-scheduler                        v1.18.2             a3099161e137        5 weeks ago         95.3MB
registry.cn-hangzhou.aliyuncs.com/google_containers/kube-scheduler            v1.18.2             a3099161e137        5 weeks ago         95.3MB
registry.aliyuncs.com/google_containers/pause                                 3.2                 80d28bedfe5d        3 months ago        683kB
registry.cn-hangzhou.aliyuncs.com/google_containers/pause                     3.2                 80d28bedfe5d        3 months ago        683kB
registry.aliyuncs.com/google_containers/coredns                               1.6.7               67da37a9a360        3 months ago        43.8MB
registry.cn-hangzhou.aliyuncs.com/google_containers/coredns                   1.6.7               67da37a9a360        3 months ago        43.8MB
registry.aliyuncs.com/google_containers/etcd                                  3.4.3-0             303ce5db0e90        7 months ago        288MB
registry.cn-hangzhou.aliyuncs.com/google_containers/etcd                      3.4.3-0             303ce5db0e90        7 months ago        288MB
registry.aliyuncs.com/google_containers/metrics-scraper                       v1.0.2              3b08661dc379        7 months ago        40.1MB
registry.cn-hangzhou.aliyuncs.com/google_containers/metrics-scraper           v1.0.2              3b08661dc379        7 months ago        40.1MB
registry.aliyuncs.com/google_containers/storage-provisioner                   v1.8.1              4689081edb10        2 years ago         80.8MB
registry.cn-hangzhou.aliyuncs.com/google_containers/storage-provisioner       v1.8.1              4689081edb10        2 years ago         80.8MB

@SataQiu
Copy link
Member Author

SataQiu commented May 28, 2020

/assign @sharifelgamal

@medyagh
Copy link
Member

medyagh commented May 28, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 28, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
docker Driver

@tstromberg tstromberg merged commit a9359c0 into kubernetes:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants