WIP: Add minikube support for the "generic" VM driver #9545
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afbjorklund 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 |
k8s-ci-robot
added
the
approved
|
These comments still apply: But it should compile now... It would be nice to clean those up, perhaps by completing the fork of the "generic" driver from libmachine. Maybe something that could be done at the same time as completing the fork of the "none" driver (#7772) |
Travis tests have failedHey @afbjorklund, 1st Buildmake test2nd Buildmake testTravisBuddy Request Identifier: ed65bcc0-1604-11eb-94fa-edf6748ec8ed |
This driver connects to an already existing virtual machine, using the provided IP address and SSH user/key/port parameters. On this machine, libmachine will provision the docker daemon and minikube will bootstrap the kubernetes cluster (as usual).
It is not supported anyway, and just throws errors. There is no use to restart or to retry, just give up. This should never be a problem with "none", though. That always return running, while generic tests ssh.
priyawadhwa
reviewed
Oct 28, 2020
| @@ -212,6 +217,12 @@ func initNetworkingFlags() { | |||
| startCmd.Flags().String(serviceCIDR, constants.DefaultServiceCIDR, "The CIDR to be used for service cluster IPs.") | |||
| startCmd.Flags().StringArrayVar(&config.DockerEnv, "docker-env", nil, "Environment variables to pass to the Docker daemon. (format: key=value)") | |||
| startCmd.Flags().StringArrayVar(&config.DockerOpt, "docker-opt", nil, "Specify arbitrary flags to pass to the Docker daemon. (format: key=value)") | |||
|
|
|||
| // generic | |||
| startCmd.Flags().String(genericIPAddress, "", "IP address (generic)") | |||
There was a problem hiding this comment.
We should validate that all of these flags are set and exit with a helpful error message if the user has forgotten anything
There was a problem hiding this comment.
I did that, but it was lost during the rebase. It should at least give an error if forgetting to provide an IP address ?
The other parameters are optional, by default it will connect as root on port 22 and assume keys are in ssh-agent
|
Thanks for the review, I will address some of the style issues shortly.
As for testing, maybe we could "reuse" the same VM over and over ? I also want to complete the fork of the driver, will get started on that... |
When I talk about the "fork", I mean the change that @medyagh did to the none driver... TODO from #6873 (comment) Do the same thing here, change the API so that "stop" means stop kubelet + containers ? Before: func (d *Driver) Start() error {
return fmt.Errorf("hosts without a driver cannot be started")
}
func (d *Driver) Stop() error {
return fmt.Errorf("hosts without a driver cannot be stopped")
}
func (d *Driver) Restart() error {
return fmt.Errorf("hosts without a driver cannot be restarted")
}
func (d *Driver) Kill() error {
return fmt.Errorf("hosts without a driver cannot be killed")
}After: // Start a host
func (d *Driver) Start() error {
var err error
d.IPAddress, err = d.GetIP()
if err != nil {
return err
}
d.URL, err = d.GetURL()
if err != nil {
return err
}
return nil
}
// Stop a host gracefully, including any containers that we are managing.
func (d *Driver) Stop() error {
if err := sysinit.New(d.exec).Stop("kubelet"); err != nil {
klog.Warningf("couldn't stop kubelet. will continue with stop anyways: %v", err)
if err := sysinit.New(d.exec).ForceStop("kubelet"); err != nil {
klog.Warningf("couldn't force stop kubelet. will continue with stop anyways: %v", err)
}
}
containers, err := d.runtime.ListContainers(cruntime.ListOptions{})
if err != nil {
return errors.Wrap(err, "containers")
}
if len(containers) > 0 {
if err := d.runtime.StopContainers(containers); err != nil {
return errors.Wrap(err, "stop containers")
}
}
klog.Infof("none driver is stopped!")
return nil
}
// Restart a host
func (d *Driver) Restart() error {
return restartKubelet(d.exec)
}
// restartKubelet restarts the kubelet
func restartKubelet(cr command.Runner) error {
klog.Infof("restarting kubelet.service ...")
c := exec.Command("sudo", "systemctl", "restart", "kubelet.service")
if _, err := cr.RunCmd(c); err != nil {
return err
}
return nil
}
// Kill stops a host forcefully, including any containers that we are managing.
func (d *Driver) Kill() error {
if err := sysinit.New(d.exec).ForceStop("kubelet"); err != nil {
klog.Warningf("couldn't force stop kubelet. will continue with kill anyways: %v", err)
}
// First try to gracefully stop containers
containers, err := d.runtime.ListContainers(cruntime.ListOptions{})
if err != nil {
return errors.Wrap(err, "containers")
}
if len(containers) == 0 {
return nil
}
// Try to be graceful before sending SIGKILL everywhere.
if err := d.runtime.StopContainers(containers); err != nil {
return errors.Wrap(err, "stop")
}
containers, err = d.runtime.ListContainers(cruntime.ListOptions{})
if err != nil {
return errors.Wrap(err, "containers")
}
if len(containers) == 0 {
return nil
}
if err := d.runtime.KillContainers(containers); err != nil {
return errors.Wrap(err, "kill")
}
return nil
}If we do the same API change here, we don't need those conditionals... Basically we change it from managing the host, to managing k8s. We could probably factor those common operations out to a separate file. |
|
The before and after can be rather drastic: Before: _, err := drivers.RunSSHCommandFromDriver(d, "sudo shutdown -r now") // a.k.a. "reboot"After: _, err := drivers.RunSSHCommandFromDriver(d, "sudo systemctl restart kubelet.service")But there is no |
medyagh
requested changes
Oct 28, 2020
|
I will give it some more testing locally, basically you will need a "raw" VM - I used Vagrant to set one up:
But it could do with some updating, and it is rather spread out over several comments and issues too... Vagrant.configure("2") do |config|
config.vm.box = "ubuntu/focal64"
config.vm.network "private_network", type: "dhcp"
config.vm.provider "virtualbox" do |vb|
vb.cpus = 2
vb.memory = "2048"
# focal64 bug: https://bugs.launchpad.net/cloud-images/+bug/1829625
vb.customize [ "modifyvm", :id, "--uartmode1", "file", File::NULL ]
end
config.vm.provision "shell", inline: <<-SHELL
apt-get update
apt-get install -y conntrack
SHELL
end |
|
After the fork (of the driver, from libmachine), we might also want to change the name of the driver... |
Assuming that this works: (Normally that requires a Then you should be able to use this setup: Where $HOST would be the IP address of the VM that you are providing. As usually, it needs to have 2 vCPU and 2 GB RAM (but not Docker etc) You would also need to have ssh running, and the user needs sudo access. Any VM that currently works with --driver none should also work remotely. |
|
Forgot that the list of drivers is hard-coded now... ( |
All drivers *must* be in "supportedDrivers" Delete Podman on Darwin (Mac), not available
afbjorklund
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
It was failing to install the "docker" package Since the provisioning was defaulting to ISO
Travis tests have failedHey @afbjorklund, 1st Buildmake testTravisBuddy Request Identifier: ec885ac0-1996-11eb-a6ec-b9eb403ed3be |
|
Added proper progress: Compare with "none": Main difference is that we need ssh to be up, so the progress and host info comes later |
Travis tests have failedHey @afbjorklund, 1st Buildmake testTravisBuddy Request Identifier: 808542b0-211c-11eb-a20b-45518f503c27 |
tstromberg
reviewed
Nov 10, 2020
afbjorklund
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
I reverted the added feature to show proper progress and host info... Needs rebasing before merging, but not needed for the review process |
|
Should give it some weird helper function, so that these become "symmetrical": Can start with IsGeneric for now, but I guess it really needs a better name later on |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
Not hearing anything, but will rebase and redo the testing tomorrow. |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
This was referenced Jan 6, 2021
Can you use |
|
Currently the provisoner in minikube is still broken, so you will have to provide the container runtime yourself. But you can choose cri-o or containerd instead of docker, if you want. Not "runc" though, that is more low-level... k8s runtimes: docker, cri-o, containerd OCI runtimes: runc, crun |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Let's try another rebase.... (PR #6873, PR #4734)
This driver connects to an already existing virtual machine,
using the provided IP address and SSH user/key/port parameters.
On this machine, libmachine will provision the docker daemon
and minikube will bootstrap the kubernetes cluster (as usual).
Implements #4733