Code review comments

Instead of checking in default config.toml, save it at /tmp/config.toml
on the node upon enable and copy it back upon disable.

Also, instead of using the prestop hook, intercept the SIGTERM kill signal upon pod
termination, disable gvisor, and then exit with code 0. This should work
better because now we will be able to see the logs from disabling, and
because the prestop hook wouldn't consistenly run the disable command
and clean up the pod correctly.

deploy/addons/gvisor/gvisor-config.toml

@@ -44,7 +44,7 @@ oom_score = 0
       [plugins.cri.containerd.untrusted_workload_runtime]
         runtime_type = "io.containerd.runtime.v1.linux"
         runtime_engine = "/usr/local/bin/runsc"
-          runtime_root = "/run/containerd/runsc"
+        runtime_root = "/run/containerd/runsc"
     [plugins.cri.cni]
       bin_dir = "/opt/cni/bin"
       conf_dir = "/etc/cni/net.d"

deploy/addons/gvisor/gvisor-containerd-shim.toml

@@ -2,4 +2,4 @@ runc_shim = "/bin/containerd-shim"
 [runsc_config]
  debug-log="/tmp/runsc/"
  debug="true"
- log="/tmp/runsc/out.log"
+ user-log="/tmp/runsc/user-log-%ID%.log"

deploy/addons/gvisor/gvisor-pod.yaml

@@ -46,10 +46,6 @@ spec:
           value: /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/node/bin
         - name: SYSTEMD_IGNORE_CHROOT
           value: "yes"
-      lifecycle:
-        preStop:
-          exec:
-            command: ["/gvisor-addon","-disable"]
   volumes:
   - name: node
     hostPath:

deploy/gvisor/Dockerfile

@@ -17,4 +17,4 @@ RUN apt-get update && \
     apt-get install -y kmod gcc wget xz-utils libc6-dev bc libelf-dev bison flex openssl libssl-dev libidn2-0 sudo libcap2 && \
     rm -rf /var/lib/apt/lists/*
 COPY out/gvisor-addon /gvisor-addon
-CMD /gvisor-addon
+CMD ["/gvisor-addon"]

docs/addons.md

@@ -40,7 +40,7 @@ The currently supported addons include:
 * [Freshpod](https://github.com/GoogleCloudPlatform/freshpod)
 * [nvidia-driver-installer](https://github.com/GoogleCloudPlatform/container-engine-accelerators/tree/master/nvidia-driver-installer/minikube)
 * [nvidia-gpu-device-plugin](https://github.com/GoogleCloudPlatform/container-engine-accelerators/tree/master/cmd/nvidia_gpu)
-* [gvisor](gvisor.md)
+* [gvisor](../deploy/addons/gvisor/README.md)
 
 If you would like to have minikube properly start/restart custom addons, place the addon(s) you wish to be launched with minikube in the `.minikube/addons` directory. Addons in this folder will be moved to the minikube VM and launched each time minikube is started/restarted.
 

pkg/gvisor/disable.go

@@ -18,21 +18,28 @@ package gvisor
 
 import (
 	"log"
+	"os"
 	"path/filepath"
 
+	"github.com/docker/machine/libmachine/mcnutils"
 	"github.com/pkg/errors"
 	"k8s.io/minikube/pkg/minikube/constants"
 )
 
 // Disable reverts containerd config files and restarts containerd
 func Disable() error {
 	log.Print("Disabling gvisor...")
-	if err := copyAssetToDest(constants.DefaultConfigTomlTargetName, filepath.Join(nodeDir, constants.ContainerdConfigTomlPath)); err != nil {
-		return errors.Wrap(err, "reverting config.toml to default")
+	if err := os.Remove(filepath.Join(nodeDir, constants.ContainerdConfigTomlPath)); err != nil {
+		return errors.Wrapf(err, "removing %s", constants.ContainerdConfigTomlPath)
+	}
+	log.Printf("Storing default config.toml at %s", constants.ContainerdConfigTomlPath)
+	if err := mcnutils.CopyFile(filepath.Join(nodeDir, constants.StoredContainerdConfigTomlPath), filepath.Join(nodeDir, constants.ContainerdConfigTomlPath)); err != nil {
+		return errors.Wrap(err, "reverting back to default config.toml")
 	}
 	// restart containerd
 	if err := restartContainerd(); err != nil {
 		return errors.Wrap(err, "restarting containerd")
 	}
+	log.Print("Successfully disabled gvisor")
 	return nil
 }

pkg/gvisor/enable.go

@@ -24,13 +24,15 @@ import (
 	"net/http"
 	"os"
 	"os/exec"
+	"os/signal"
 	"path/filepath"
+	"syscall"
 	"time"
 
-	"k8s.io/minikube/pkg/minikube/constants"
-
+	"github.com/docker/machine/libmachine/mcnutils"
 	"github.com/pkg/errors"
 	"k8s.io/minikube/pkg/minikube/assets"
+	"k8s.io/minikube/pkg/minikube/constants"
 )
 
 const (
@@ -55,6 +57,17 @@ func Enable() error {
 	if err := restartContainerd(); err != nil {
 		return errors.Wrap(err, "restarting containerd")
 	}
+	// When pod is terminated, disable gvisor and exit
+	c := make(chan os.Signal)
+	signal.Notify(c, os.Interrupt, syscall.SIGTERM)
+	go func() {
+		<-c
+		if err := Disable(); err != nil {
+			log.Printf("Error disabling gvisor: %v", err)
+		}
+		os.Exit(0)
+	}()
+	log.Print("gvisor successfully enabled in cluster")
 	// sleep for one year so the pod continuously runs
 	time.Sleep(24 * 7 * 52 * time.Hour)
 	return nil
@@ -96,14 +109,13 @@ func downloadBinaries() error {
 // downloads the gvisor-containerd-shim
 func gvisorContainerdShim() error {
 	dest := filepath.Join(nodeDir, "usr/bin/gvisor-containerd-shim")
-	// TODO: priyawadhwa@, replace with official release
-	return downloadFileToDest("http://storage.googleapis.com/balintp-minikube/gvisor-containerd-shim", dest)
+	return downloadFileToDest(constants.GvisorContainerdShimURL, dest)
 }
 
 // downloads the runsc binary and returns a path to the binary
 func runsc() error {
 	dest := filepath.Join(nodeDir, "usr/local/bin/runsc")
-	return downloadFileToDest("http://storage.googleapis.com/gvisor/releases/nightly/latest/runsc", dest)
+	return downloadFileToDest(constants.GvisorURL, dest)
 }
 
 // downloadFileToDest downlaods the given file to the dest
@@ -136,7 +148,12 @@ func downloadFileToDest(url, dest string) error {
 // Must write the following files:
 //    1. gvisor-containerd-shim.toml
 //    2. gvisor containerd config.toml
+// and save the default version of config.toml
 func copyFiles() error {
+	log.Printf("Storing default config.toml at %s", constants.StoredContainerdConfigTomlPath)
+	if err := mcnutils.CopyFile(filepath.Join(nodeDir, constants.ContainerdConfigTomlPath), filepath.Join(nodeDir, constants.StoredContainerdConfigTomlPath)); err != nil {
+		return errors.Wrap(err, "copying default config.toml")
+	}
 	log.Print("Copying gvisor-containerd-shim.toml...")
 	if err := copyAssetToDest(constants.GvisorContainerdShimTargetName, filepath.Join(nodeDir, constants.GvisorContainerdShimTomlPath)); err != nil {
 		return errors.Wrap(err, "copying gvisor-containerd-shim.toml")

pkg/minikube/assets/addons.go

@@ -292,11 +292,6 @@ var Addons = map[string]*Addon{
 			constants.GvisorFilesPath,
 			constants.GvisorContainerdShimTargetName,
 			"0640"),
-		NewBinDataAsset(
-			"deploy/addons/gvisor/config.toml",
-			constants.GvisorFilesPath,
-			constants.DefaultConfigTomlTargetName,
-			"0640"),
 	}, false, "gvisor"),
 }
 

pkg/minikube/constants/constants.go

@@ -270,11 +270,16 @@ const (
 	ContainerdConfigTomlPath = "/etc/containerd/config.toml"
 	// GvisorContainerdShimTomlPath is the path to givosr-containerd-shim.toml
 	GvisorContainerdShimTomlPath = "/etc/containerd/gvisor-containerd-shim.toml"
+	// StoredContainerdConfigTomlPath is the path where the default config.toml will be stored
+	StoredContainerdConfigTomlPath = "/tmp/config.toml"
 
 	//GvisorConfigTomlTargetName is the go-bindata target name for the gvisor config.toml
 	GvisorConfigTomlTargetName = "gvisor-config.toml"
 	// GvisorContainerdShimTargetName is the go-bindata target name for gvisor-containerd-shim
 	GvisorContainerdShimTargetName = "gvisor-containerd-shim.toml"
-	// DefaultConfigTomlTargetName is the go-bindata target name for the default config.toml
-	DefaultConfigTomlTargetName = "config.toml"
+
+	// GvisorContainerdShimURL is the url to download gvisor-containerd-shim
+	GvisorContainerdShimURL = "https://github.com/google/gvisor-containerd-shim/releases/download/v0.0.1-rc.0/gvisor-containerd-shim-v0.0.1-rc.0.linux-amd64"
+	// GvisorURL is the url to download gvisor
+	GvisorURL = "http://storage.googleapis.com/gvisor/releases/nightly/latest/runsc"
 )