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

minikube start --image-repository will now accept URLs with port #11585

Merged
merged 6 commits into from
Jul 12, 2021

Conversation

dmpe
Copy link
Contributor

@dmpe dmpe commented Jun 5, 2021

fixes #11579

fixes:

  • misspellings -> rename variables and functions

  • --image-repository shall now accept URLs with ports

Notes:

  • my first attempt at go
  • my first (potential) contribution to kubernetes/minikube project (k8s for that matter either) 🌴

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 5, 2021
@k8s-ci-robot k8s-ci-robot requested review from afbjorklund and RA489 June 5, 2021 19:01
@k8s-ci-robot
Copy link
Contributor

Welcome @dmpe!

It looks like this is your first PR to kubernetes/minikube 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/minikube has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @dmpe. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 5, 2021
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@dmpe dmpe changed the title wip: fix(start): minikube start --image-repository now accepts URL with port [WIP]: fix(start): minikube start --image-repository now accepts URL with port Jun 5, 2021
@dmpe dmpe marked this pull request as draft June 5, 2021 19:03
@dmpe dmpe changed the title [WIP]: fix(start): minikube start --image-repository now accepts URL with port [WIP]: minikube start --image-repository will now accept URLs with port Jun 5, 2021
@dmpe dmpe changed the title [WIP]: minikube start --image-repository will now accept URLs with port minikube start --image-repository will now accept URLs with port Jun 8, 2021
@dmpe dmpe marked this pull request as ready for review June 8, 2021 17:47
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2021
@dmpe
Copy link
Contributor Author

dmpe commented Jun 11, 2021

@afbjorklund can you please ok'it to test - so that test can be run.

thanks.

@dmpe
Copy link
Contributor Author

dmpe commented Jun 19, 2021

@RA489 maybe you can also take a look? Thanks

@dmpe
Copy link
Contributor Author

dmpe commented Jul 1, 2021

@medyagh Hi, can you please take a look on this. I would like to get it merged :) Thanks.

@medyagh
Copy link
Member

medyagh commented Jul 5, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 5, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmpe, medyagh

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2021
@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11585) |
+----------------+----------+---------------------+
| minikube start | 48.9s    | 49.8s               |
| enable ingress | 34.7s    | 34.4s               |
+----------------+----------+---------------------+

Times for minikube (PR 11585) ingress: 33.8s 33.7s 36.8s 33.8s 33.8s
Times for minikube ingress: 33.8s 34.7s 34.4s 36.3s 34.3s

Times for minikube start: 51.1s 50.4s 48.0s 47.1s 47.9s
Times for minikube (PR 11585) start: 52.3s 49.2s 52.5s 47.9s 47.2s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11585) |
+----------------+----------+---------------------+
| minikube start | 22.1s    | 22.4s               |
| enable ingress | 31.2s    | 29.7s               |
+----------------+----------+---------------------+

Times for minikube start: 22.4s 22.0s 22.5s 22.7s 21.2s
Times for minikube (PR 11585) start: 21.7s 23.2s 22.5s 22.5s 21.9s

Times for minikube ingress: 29.0s 28.0s 29.5s 33.0s 36.5s
Times for minikube (PR 11585) ingress: 29.0s 32.6s 26.5s 30.5s 30.0s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11585) |
+----------------+----------+---------------------+
| minikube start | 44.9s    | 45.4s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube start: 43.5s 44.3s 43.9s 43.4s 49.3s
Times for minikube (PR 11585) start: 44.0s 43.5s 43.8s 47.9s 48.0s

@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11585) |
+----------------+----------+---------------------+
| minikube start | 47.4s    | 49.3s               |
| enable ingress | 36.3s    | 40.3s               |
+----------------+----------+---------------------+

Times for minikube start: 49.2s 46.5s 47.6s 46.4s 47.3s
Times for minikube (PR 11585) start: 47.5s 49.5s 46.7s 52.6s 50.0s

Times for minikube ingress: 33.7s 42.2s 34.7s 36.8s 34.2s
Times for minikube (PR 11585) ingress: 42.7s 37.2s 36.2s 42.2s 43.1s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11585) |
+----------------+----------+---------------------+
| minikube start | 21.8s    | 21.7s               |
| enable ingress | 35.2s    | 34.4s               |
+----------------+----------+---------------------+

Times for minikube start: 22.9s 21.3s 21.2s 22.1s 21.4s
Times for minikube (PR 11585) start: 21.4s 21.5s 23.1s 21.9s 20.7s

Times for minikube ingress: 31.9s 34.9s 38.0s 37.5s 33.5s
Times for minikube (PR 11585) ingress: 32.0s 36.5s 33.0s 32.0s 38.5s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11585) |
+----------------+----------+---------------------+
| minikube start | 42.2s    | 45.4s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube (PR 11585) start: 47.0s 42.8s 47.5s 42.5s 47.3s
Times for minikube start: 31.2s 44.1s 43.4s 48.8s 43.3s

@dmpe
Copy link
Contributor Author

dmpe commented Jul 6, 2021

Hi @medyagh, @RA489 @afbjorklund

Thanks for running tests on this. Besides them I have also just run my own testing with sonatype/nexus3 container.
I have started empty nexus3, configured 2 repos: k8s.gcr.io and gcr.io in order to proxy images locally (as we do at work) and then run:

sudo ./minikube-linux-amd64 start --driver=none --image-repository http://localhost:6668 (proxy to k8s.gcr.io)

Result:

😄  minikube v1.22.0-beta.0 on Ubuntu 21.04
✨  Using the none driver based on user configuration
    ▪ The --image-repository flag your provided contains Scheme: http, which will be removed automatically
✅  Using image repository localhost:6668
👍  Starting control plane node minikube in cluster minikube
🤹  Running on localhost (CPUs=4, Memory=4178MB, Disk=79614MB) ...
ℹ️  OS release is Ubuntu 21.04
❗  This bare metal machine is having trouble accessing https://localhost:6668
💡  To pull new external images, you may need to configure a proxy: https://minikube.sigs.k8s.io/docs/reference/networking/proxy/
🐳  Preparing Kubernetes v1.20.8 on Docker 20.10.7 ...
    ▪ kubelet.resolv-conf=/run/systemd/resolve/resolv.conf
    ▪ Generating certificates and keys ...
    ▪ Booting up control plane ...
    ▪ Configuring RBAC rules ...
🤹  Configuring local host environment ...

❗  The 'none' driver is designed for experts who need to integrate with an existing VM
💡  Most users should use the newer 'docker' driver instead, which does not require root!
📘  For more information, see: https://minikube.sigs.k8s.io/docs/reference/drivers/none/

❗  kubectl and minikube configuration will be stored in /root
❗  To use kubectl or minikube commands as your own user, you may need to relocate them. For example, to overwrite your own settings, run:

    ▪ sudo mv /root/.kube /root/.minikube $HOME
    ▪ sudo chown -R $USER $HOME/.kube $HOME/.minikube

💡  This can also be done automatically by setting the env var CHANGE_MINIKUBE_NONE_USER=true
🔎  Verifying Kubernetes components...
    ▪ Using image localhost:6668/k8s-minikube/storage-provisioner:v5 (global image repository)
🌟  Enabled addons: default-storageclass, storage-provisioner
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

I.e. it is now working and using nexus3 docker images, instead of going outside. This is also validated by the fact, that nexus3 contains all those docker images.

2 Points:

The --image-repository flag your provided contains Scheme: http, which will be removed automatically

In order to use minikube with nexus3 ports, you will need to specify scheme, even though I am not sure it is necessary. Certainly docker pull localhost:6669/k8s-minikube/storage-provisioner:v5 is working, while docker pull http://localhost:6669/k8s-minikube/storage-provisioner:v5 is correctly not working. So the message applies only for certain contexts - and with ports it shall not apply at all.

This bare metal machine is having trouble accessing https://localhost:6668

This message is also confusing, because port 6668 is not https, but rather (as already said above) http. It seems to be hardcoded https://github.com/kubernetes/minikube/blob/master/pkg/minikube/node/start.go#L653 here. Probably a new issue and new PR, later.

wdyt? There are some tests which fail, but seem to be unrelated to my PR.

The executable, used for testing, was downloaded using

gsutil ls gs://minikube-builds/11585/minikube-linux-amd64

@spowelljr
Copy link
Member

@dmpe The tests look fine, we have many flaky tests. I left a comment on this PR, it's not a requirement to address the comment, so I'll leave it up to you to decide. If you don't want to implement the comment lmk and I'll merge this PR now.

@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11585) |
+----------------+----------+---------------------+
| minikube start | 51.2s    | 50.8s               |
| enable ingress | 32.5s    | 32.7s               |
+----------------+----------+---------------------+

Times for minikube ingress: 31.3s 33.9s 34.3s 31.4s 31.5s
Times for minikube (PR 11585) ingress: 32.9s 32.8s 32.9s 32.4s 32.4s

Times for minikube (PR 11585) start: 49.4s 52.9s 50.4s 53.8s 47.4s
Times for minikube start: 54.5s 49.2s 49.2s 52.4s 50.9s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11585) |
+----------------+----------+---------------------+
| minikube start | 22.9s    | 22.9s               |
| enable ingress | 31.9s    | 33.1s               |
+----------------+----------+---------------------+

Times for minikube start: 23.9s 23.2s 22.3s 23.1s 21.8s
Times for minikube (PR 11585) start: 22.7s 23.0s 23.0s 23.0s 22.6s

Times for minikube (PR 11585) ingress: 27.5s 35.5s 31.0s 35.5s 36.0s
Times for minikube ingress: 35.0s 31.0s 27.0s 30.6s 36.0s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 11585) |
+----------------+----------+---------------------+
| minikube start | 40.3s    | 43.9s               |
| enable ingress |          |                     |
+----------------+----------+---------------------+

Times for minikube start: 27.8s 42.9s 43.8s 43.7s 43.3s
Times for minikube (PR 11585) start: 45.0s 43.6s 43.6s 43.6s 43.9s

@dmpe
Copy link
Contributor Author

dmpe commented Jul 12, 2021

Hi @spowelljr - thank you very much for looking into this and giving me a feedback.

I have rebased on latest master, run make tests and did again above testing (works) and it looks good to me. There is an issue with coredns image (coredns:v1.8.0 - neither from gcr.io nor from k8s.gcr.io - does not exist, but retagging an image from dockerhub coredns/coredns:1.8.0 did the trick) but otherwise it is working good. Feel free to merge :)

Thanks again

### This was tested multiple times, here output from without using nexus3
####
minikube v1.22.0 on Ubuntu 21.04
✨  Using the none driver based on user configuration
👍  Starting control plane node minikube in cluster minikube
🤹  Running on localhost (CPUs=4, Memory=4178MB, Disk=79614MB) ...
ℹ️  OS release is Ubuntu 21.04
🐳  Preparing Kubernetes v1.21.2 on Docker 20.10.7 ...
    ▪ kubelet.resolv-conf=/run/systemd/resolve/resolv.conf
    ▪ Generating certificates and keys ...
    ▪ Booting up control plane ...
    ▪ Configuring RBAC rules ...
🤹  Configuring local host environment ...

❗  The 'none' driver is designed for experts who need to integrate with an existing VM
💡  Most users should use the newer 'docker' driver instead, which does not require root!
📘  For more information, see: https://minikube.sigs.k8s.io/docs/reference/drivers/none/

❗  kubectl and minikube configuration will be stored in /root
❗  To use kubectl or minikube commands as your own user, you may need to relocate them. For example, to overwrite your own settings, run:

    ▪ sudo mv /root/.kube /root/.minikube $HOME
    ▪ sudo chown -R $USER $HOME/.kube $HOME/.minikube

💡  This can also be done automatically by setting the env var CHANGE_MINIKUBE_NONE_USER=true
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: storage-provisioner, default-storageclass
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

@spowelljr spowelljr merged commit 3a18901 into kubernetes:master Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

minikube start --image-repository=xxx does not work with ports in URL
6 participants