Skip to content

Conversation

@bacongobbler
Copy link
Contributor

The default CIDR should be 10.0.0.0/24 rather than 10.0.0.1.
Accidental off-by-one error. :)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 16, 2017
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@aaron-prindle
Copy link
Contributor

@minikube-bot ok to test

@bacongobbler
Copy link
Contributor Author

See #1583 for when this was introduced

@bacongobbler
Copy link
Contributor Author

It also appears that there are a few more bugs like the pods not being in the service's pod selector... More fixes to come in this PR!

The default CIDR should be 10.0.0.0/24 rather than 10.0.0.1.
Accidental off-by-one error. :)
@bacongobbler bacongobbler force-pushed the fixup-registry-addon branch from 5f8ace7 to a3c5bf3 Compare June 16, 2017 21:28
@bacongobbler
Copy link
Contributor Author

k, I'm going to manually test these changes before they're ready to merge to make sure I didn't mess anything up this time around.

@bacongobbler
Copy link
Contributor Author

Okay, this now works as intended.

From one pod in the cluster, pushing an image into the registry using the registry service's virtual IP address:

--> Pushing 10.0.0.131/draft/py:ab8c330f1325c0b25671841381430498edb98259
The push refers to a repository [10.0.0.131/draft/py]
[...]
e4c17ef2609e: Pushed
ab8c330f1325c0b25671841381430498edb98259: digest: sha256:1877adfe66e955b7f4bdd5fa27cdf908a864a8cf142052f311f19676bc9c8b77 size: 2840

Then, with a deployment pointing at the service IP address as the image name:

><> k get po py-exasperated-dragon-1723738893-6rw6h -o jsonpath='{.spec.containers[0].image}'
10.0.0.131/draft/py:ab8c330f1325c0b25671841381430498edb98259

><> k get po py-exasperated-dragon-1723738893-6rw6h -o jsonpath='{.status.containerStatuses[0].ready}'
true

All good to go :)

@dlorenc
Copy link
Contributor

dlorenc commented Jun 19, 2017

@minikube-bot OK to test

startCmd.Flags().String(apiServerName, constants.APIServerName, "The apiserver name which is used in the generated certificate for localkube/kubernetes. This can be used if you want to make the apiserver available from outside the machine")
startCmd.Flags().String(dnsDomain, constants.ClusterDNSDomain, "The cluster dns domain name used in the kubernetes cluster")
startCmd.Flags().StringSliceVar(&insecureRegistry, "insecure-registry", []string{util.DefaultServiceClusterIP + "/24"}, "Insecure Docker registries to pass to the Docker daemon")
startCmd.Flags().StringSliceVar(&insecureRegistry, "insecure-registry", []string{constants.DefaultInsecureRegistry}, "Insecure Docker registries to pass to the Docker daemon")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to reference this as "pkgutil.DefaultInsecureRegistry" (or move the constant definition to pkg/minikube/constants/constants.go)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry! I fixed this locally but never pushed up the change. Thanks!

@bacongobbler bacongobbler force-pushed the fixup-registry-addon branch from a3c5bf3 to b684071 Compare June 19, 2017 16:13
@codecov-io
Copy link

codecov-io commented Jun 19, 2017

Codecov Report

Merging #1603 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1603   +/-   ##
=======================================
  Coverage   38.66%   38.66%           
=======================================
  Files          51       51           
  Lines        2607     2607           
=======================================
  Hits         1008     1008           
  Misses       1421     1421           
  Partials      178      178
Impacted Files Coverage Δ
pkg/util/constants.go 0% <ø> (ø) ⬆️
cmd/minikube/cmd/start.go 17.05% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 745e494...cce611e. Read the comment docs.

template:
metadata:
labels:
addonmanager.kubernetes.io/mode: Reconcile
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to keep this Reconcile label here in the template/metadata for the addon manager to pick it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it required for the pod manifest as well? I guess replication controllers don't work the same as deployments. I'll add this back.

In a last-minute fix, I accidentally changed the pod labels all to
the minikube add-on reconciliation mode label instead of the add-on
name. This is causing the registry service to be unable to forward
requests over to the pods due to mismatched label selectors.
@bacongobbler bacongobbler force-pushed the fixup-registry-addon branch from b684071 to cce611e Compare June 19, 2017 16:57
@dlorenc dlorenc merged commit 51cf8cf into kubernetes:master Jun 19, 2017
@bacongobbler bacongobbler deleted the fixup-registry-addon branch June 19, 2017 19:55
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants