WIP (Early): kic (improved VM-free: deploy Kubernetes to containers) #5188

medyagh · 2019-08-23T16:54:00Z

No description provided.

k8s-ci-robot · 2019-08-23T16:54:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [medyagh]

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

afbjorklund

Need to take a more detailed look, both at the difference between bootstrappers (kubeadm and kic) and of course on the github.com/medyagh/kic code itself. Some kind of design document would be great as well.

Keep up the good work!

afbjorklund · 2019-08-25T07:44:44Z

pkg/drivers/kic/kic.go

+func NewDriver(c Config) *Driver {
+	runner := &command.OciRunner{} // MEDYA:TODO pass the node selector
+	// runtime, err := cruntime.New(cruntime.Config{Type: c.ContainerRuntime, Runner: runner})
+	// Libraries shouldn't panic, but there is no way for drivers to return error :(


I think you are doing too much work in the factory, it is only supposed to return the object.

The actual creation of runtime and what not, should not happen until it is actually used ?

cmd/minikube/cmd/start.go

afbjorklund · 2019-08-25T07:49:19Z

pkg/drivers/kic/kic.go

+
+// Remove a host, including any data which may have been written by it.
+func (d *Driver) Remove() error {
+	// TODO: remove the image from cache maybe ?


Probably not

The cache belongs to the runtime that started the node, you could clean up the images in your containerd - but I suppose that will happen anyway

afbjorklund · 2019-08-25T07:53:15Z

pkg/drivers/drivers.go

@@ -29,8 +29,17 @@ import (
 	"github.com/docker/machine/libmachine/ssh"
 	"github.com/golang/glog"
 	"github.com/pkg/errors"
+	"k8s.io/minikube/pkg/minikube/constants"


Hmm, this makes the docker-machine drivers depend on minikube again ?

good catch ! will remove that

afbjorklund · 2019-08-25T07:55:08Z

pkg/drivers/kic/kic.go

+
+const driverName = constants.DriverKic
+
+// Driver is a driver designed to run kubeadm inside a container


Is it really hard-coded to kubeadm ? Seems most of that happens in the bootstrapper ?

afbjorklund · 2019-08-25T07:56:57Z

pkg/drivers/kic/kic.go

+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("tcp://%s:2376", ip), nil


Are you really running your own nested Docker daemon (port 2376) ?

if I am not mistaken that is the port that bing the container to the kube-api server to be sued in kubeconfig.
the docker one is 2375. i do not use my own docker-daemon, kick uses containerd

Ok, containerd runs on a Unix socket as far as I know. So basically you don’t offer a (Docker) URL anyway...

afbjorklund · 2019-08-25T08:00:29Z

pkg/drivers/kic/kic.go

+
+	// MEDYA:TODO
+	// if alread exists use docker inspect --format '{{ .NetworkSettings.IPAddress }}' d.MachineName
+	ip, err := net.ChooseBindAddress(nil)


Hmm, so this is returning the docker host address. I would have expected a docker address like 172.17.0.2

Or at least localhost, rather than the default bind address. This is similar to the current issues with none

I actually dont know what would be best here. what are the current issues with none binding address ?

do you think it is better to bind on localhost ? or on the private ip that machine has ?

I was thinking we let the user pass the ip if they want to. so it doesnt have to be localhost. for mroe advanced options !

I am all ears for any recommednaiton on the bind address

tstromberg

Adding some kind of design doc that can be added to our documentation site would be super great to fully understand what we're reviewing.

tstromberg · 2019-08-30T20:51:02Z

cmd/minikube/cmd/start.go

+
+	// kic
+	startCmd.Flags().String(kicImg, "", "override the kic image. (Only to be used with on with kic driver) ")
+	startCmd.Flags().String(ociClient, "docker", "the oci client to be used. (Only to be used with on with kic driver)")


To avoid introducing an additional level of confusing concepts, I think this should use --driver=docker (and rename --vm-driver to --driver)

tstromberg · 2019-08-30T20:52:10Z

cmd/minikube/cmd/start.go

@@ -197,6 +201,11 @@ func initDriverFlags() {

 	// hyperv
 	startCmd.Flags().String(hypervVirtualSwitch, "", "The hyperv virtual switch name. Defaults to first found. (only supported with HyperV driver)")
+
+	// kic
+	startCmd.Flags().String(kicImg, "", "override the kic image. (Only to be used with on with kic driver) ")


Could we use --image? I would prefer not to have kic as a second brandname that users need to learn about.

tstromberg · 2019-08-30T20:56:20Z

pkg/minikube/bootstrapper/kic/kic.go

+limitations under the License.
+*/
+
+package kic


How can we improve the bootstrapper interface and/or kubeadm enough to prevent the abstraction leakage that makes it necessary for kic to have it's own kubeadm driver?

I'm sure it's the easiest for the initial implementation, but for production, I would rather see about improving our existing code than introducing a second subtly different kubeadm bootstrapper.

tstromberg · 2019-10-09T18:46:19Z

This PR hasn't seen an update in a while. Do you intend on pushing a newer version up soon?

medyagh · 2019-10-14T23:02:49Z

I will make a newer PR once the command runner PR is merged #5530

medyagh added 20 commits August 22, 2019 20:42

add boilerplate kic bootstrapper

01b8f30

Added boilerplate for kic pkg/driver

f1d3ea9

added pkg/minikube/drivers/kics boilerplate

009a815

goimports

066877a

added to supported os

186d2d7

Added command runner boilerplate

e770a66

Added Oci Runner

8901818

create OciRunner based on SSHRunner

d019109

Add KickImage and OciClient to driver config

f61a419

Add auto select image

67913d1

rename to a shorter var name

1538d99

added start flags for ocilient and kicimg

d4bdd76

skip download iso for kic and move helper func

9b4b041

Add cache boiler funcs for kic and rename saveConfig to be more obvious

11e8ba0

Add OciCommandRunner boilerplate

b004228

add boilet plate bootstrapper for kic

4e42176

StartCluster for kic

25450b7

add boilerplate for installCNI

e810f52

Added SetStdin to Runner interface

2e687b3

add SetSetdOut to command runner

52b94c1

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 23, 2019

k8s-ci-robot requested review from afbjorklund and tstromberg August 23, 2019 16:54

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2019

afbjorklund requested changes Aug 25, 2019

View reviewed changes

afbjorklund added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2019

afbjorklund mentioned this pull request Aug 25, 2019

Prototype Docker deployment model #4772

Closed

tstromberg changed the title ~~Early WIP: kic~~ Early WIP: kic (improved VM-free: deploy Kubernetes to containers) Aug 27, 2019

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 Aug 27, 2019

tstromberg suggested changes Aug 30, 2019

View reviewed changes

afbjorklund changed the title ~~Early WIP: kic (improved VM-free: deploy Kubernetes to containers)~~ WIP (Early): kic (improved VM-free: deploy Kubernetes to containers) Aug 31, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2019

medyagh closed this Oct 14, 2019

medyagh deleted the kic branch October 14, 2019 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP (Early): kic (improved VM-free: deploy Kubernetes to containers) #5188

WIP (Early): kic (improved VM-free: deploy Kubernetes to containers) #5188

medyagh commented Aug 23, 2019

k8s-ci-robot commented Aug 23, 2019

afbjorklund left a comment

afbjorklund Aug 25, 2019

afbjorklund Aug 25, 2019

afbjorklund Aug 28, 2019

afbjorklund Aug 25, 2019

medyagh Aug 26, 2019

afbjorklund Aug 25, 2019

afbjorklund Aug 25, 2019

medyagh Aug 26, 2019

afbjorklund Aug 28, 2019

afbjorklund Aug 25, 2019

medyagh Aug 26, 2019

afbjorklund Aug 28, 2019

tstromberg left a comment

tstromberg Aug 30, 2019

tstromberg Aug 30, 2019

tstromberg Aug 30, 2019

tstromberg commented Oct 9, 2019

medyagh commented Oct 14, 2019


		const driverName = constants.DriverKic

		// Driver is a driver designed to run kubeadm inside a container

WIP (Early): kic (improved VM-free: deploy Kubernetes to containers) #5188

WIP (Early): kic (improved VM-free: deploy Kubernetes to containers) #5188

Conversation

medyagh commented Aug 23, 2019

k8s-ci-robot commented Aug 23, 2019

afbjorklund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg commented Oct 9, 2019

medyagh commented Oct 14, 2019