From 3fee641a138662451599861c43a2ed4c221bcfdb Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Fri, 6 Mar 2020 15:15:05 -0800 Subject: [PATCH 1/2] Remove remaining PointToHostDockerDaemon calls --- pkg/drivers/kic/oci/info.go | 7 +-- pkg/drivers/kic/oci/network.go | 24 +++++----- pkg/drivers/kic/oci/oci.go | 81 ++++++++++++++-------------------- pkg/drivers/kic/oci/volumes.go | 22 +++------ 4 files changed, 56 insertions(+), 78 deletions(-) diff --git a/pkg/drivers/kic/oci/info.go b/pkg/drivers/kic/oci/info.go index 3245bee2a64f..548c071f16d5 100644 --- a/pkg/drivers/kic/oci/info.go +++ b/pkg/drivers/kic/oci/info.go @@ -216,17 +216,18 @@ type podmanSysInfo struct { // dockerSystemInfo returns docker system info --format '{{json .}}' func dockerSystemInfo() (dockerSysInfo, error) { var ds dockerSysInfo - if err := PointToHostDockerDaemon(); err != nil { - return ds, errors.Wrap(err, "point host docker-daemon") - } + cmd := exec.Command(Docker, "system", "info", "--format", "{{json .}}") out, err := cmd.CombinedOutput() + if err != nil { return ds, errors.Wrap(err, "get docker system info") } + if err := json.Unmarshal([]byte(strings.TrimSpace(string(out))), &ds); err != nil { return ds, errors.Wrapf(err, "unmarshal docker system info") } + return ds, nil } diff --git a/pkg/drivers/kic/oci/network.go b/pkg/drivers/kic/oci/network.go index bfb602dcde17..26d85c520cf8 100644 --- a/pkg/drivers/kic/oci/network.go +++ b/pkg/drivers/kic/oci/network.go @@ -43,15 +43,14 @@ func RoutableHostIPFromInside(ociBin string, containerName string) (net.IP, erro // digDNS will get the IP record for a dns func digDNS(ociBin, containerName, dns string) (net.IP, error) { - if err := PointToHostDockerDaemon(); err != nil { - return nil, errors.Wrap(err, "point host docker daemon") - } cmd := exec.Command(ociBin, "exec", "-t", containerName, "dig", "+short", dns) out, err := cmd.CombinedOutput() ip := net.ParseIP(strings.TrimSpace(string(out))) + if err != nil { return ip, errors.Wrapf(err, "resolve dns to ip: %s", string(out)) } + glog.Infof("got host ip for mount in container by digging dns: %s", ip.String()) return ip, nil } @@ -59,21 +58,22 @@ func digDNS(ociBin, containerName, dns string) (net.IP, error) { // dockerGatewayIP gets the default gateway ip for the docker bridge on the user's host machine // gets the ip from user's host docker func dockerGatewayIP() (net.IP, error) { - if err := PointToHostDockerDaemon(); err != nil { - return nil, errors.Wrap(err, "point host docker daemon") - } cmd := exec.Command(Docker, "network", "ls", "--filter", "name=bridge", "--format", "{{.ID}}") out, err := cmd.CombinedOutput() + if err != nil { return nil, errors.Wrapf(err, "get network bridge. output: %s", string(out)) } + bridgeID := strings.TrimSpace(string(out)) cmd = exec.Command(Docker, "inspect", "--format", "{{(index .IPAM.Config 0).Gateway}}", bridgeID) out, err = cmd.CombinedOutput() + if err != nil { return nil, errors.Wrapf(err, "inspect IP gatway for bridge network: %q. output: %s", string(out), bridgeID) } + ip := net.ParseIP(strings.TrimSpace(string(out))) glog.Infof("got host ip for mount in container by inspect docker network: %s", ip.String()) return ip, nil @@ -85,11 +85,9 @@ func dockerGatewayIP() (net.IP, error) { // 32769, nil // only supports TCP ports func HostPortBinding(ociBinary string, ociID string, contPort int) (int, error) { - if err := PointToHostDockerDaemon(); err != nil { - return 0, errors.Wrap(err, "point host docker daemon") - } var out []byte var err error + if ociBinary == Podman { //podman inspect -f "{{range .NetworkSettings.Ports}}{{if eq .ContainerPort "80"}}{{.HostPort}}{{end}}{{end}}" cmd := exec.Command(ociBinary, "inspect", "-f", fmt.Sprintf("{{range .NetworkSettings.Ports}}{{if eq .ContainerPort %s}}{{.HostPort}}{{end}}{{end}}", fmt.Sprint(contPort)), ociID) @@ -108,9 +106,11 @@ func HostPortBinding(ociBinary string, ociID string, contPort int) (int, error) o := strings.TrimSpace(string(out)) o = strings.Trim(o, "'") p, err := strconv.Atoi(o) + if err != nil { return p, errors.Wrapf(err, "convert host-port %q to number", p) } + return p, nil } @@ -140,20 +140,20 @@ func podmanConttainerIP(name string) (string, string, error) { // dockerContainerIP returns ipv4, ipv6 of container or error func dockerContainerIP(name string) (string, string, error) { - if err := PointToHostDockerDaemon(); err != nil { - return "", "", errors.Wrap(err, "point host docker daemon") - } // retrieve the IP address of the node using docker inspect lines, err := inspect(Docker, name, "{{range .NetworkSettings.Networks}}{{.IPAddress}},{{.GlobalIPv6Address}}{{end}}") if err != nil { return "", "", errors.Wrap(err, "inspecting NetworkSettings.Networks") } + if len(lines) != 1 { return "", "", errors.Errorf("IPs output should only be one line, got %d lines", len(lines)) } + ips := strings.Split(lines[0], ",") if len(ips) != 2 { return "", "", errors.Errorf("container addresses should have 2 values, got %d values: %+v", len(ips), ips) } + return ips[0], ips[1], nil } diff --git a/pkg/drivers/kic/oci/oci.go b/pkg/drivers/kic/oci/oci.go index 32835d705604..90f7a656be56 100644 --- a/pkg/drivers/kic/oci/oci.go +++ b/pkg/drivers/kic/oci/oci.go @@ -39,18 +39,16 @@ import ( // if there no containers found with the given label, it will return nil func DeleteContainersByLabel(ociBin string, label string) []error { var deleteErrs []error - if ociBin == Docker { - if err := PointToHostDockerDaemon(); err != nil { - return []error{errors.Wrap(err, "point host docker daemon")} - } - } + cs, err := listContainersByLabel(ociBin, label) if err != nil { return []error{fmt.Errorf("listing containers by label %q", label)} } + if len(cs) == 0 { return nil } + for _, c := range cs { _, err := ContainerStatus(ociBin, c) // only try to delete if docker/podman inspect returns @@ -71,9 +69,7 @@ func DeleteContainersByLabel(ociBin string, label string) []error { // DeleteContainer deletes a container by ID or Name func DeleteContainer(ociBin string, name string) error { - if err := PointToHostDockerDaemon(); err != nil { - return errors.Wrap(err, "point host docker daemon") - } + _, err := ContainerStatus(ociBin, name) if err != nil { glog.Errorf("%s daemon seems to be stuck. Please try restarting your %s. Will try to delete anyways: %v", ociBin, ociBin, err) @@ -88,9 +84,6 @@ func DeleteContainer(ociBin string, name string) error { // CreateContainerNode creates a new container node func CreateContainerNode(p CreateParams) error { - if err := PointToHostDockerDaemon(); err != nil { - return errors.Wrap(err, "point host docker daemon") - } runArgs := []string{ "-d", // run the container detached @@ -169,10 +162,6 @@ func CreateContainerNode(p CreateParams) error { // CreateContainer creates a container with "docker/podman run" func createContainer(ociBinary string, image string, opts ...createOpt) ([]string, error) { - if err := PointToHostDockerDaemon(); err != nil { - return nil, errors.Wrap(err, "point host docker daemon") - } - o := &createOpts{} for _, opt := range opts { o = opt(o) @@ -187,10 +176,12 @@ func createContainer(ociBinary string, image string, opts ...createOpt) ([]strin } // construct the actual docker run argv args := []string{"run"} + // to run nested container from privileged container in podman https://bugzilla.redhat.com/show_bug.cgi?id=1687713 if ociBinary == Podman { args = append(args, "--cgroup-manager", "cgroupfs") } + args = append(args, runArgs...) args = append(args, image) args = append(args, o.ContainerArgs...) @@ -201,6 +192,7 @@ func createContainer(ociBinary string, image string, opts ...createOpt) ([]strin err := cmd.Run() scanner := bufio.NewScanner(&buff) var output []string + for scanner.Scan() { output = append(output, scanner.Text()) } @@ -208,82 +200,81 @@ func createContainer(ociBinary string, image string, opts ...createOpt) ([]strin if err != nil { return output, errors.Wrapf(err, "args: %v output: %s ", args, output) } + return output, nil } // Copy copies a local asset into the container func Copy(ociBinary string, ociID string, targetDir string, fName string) error { - if err := PointToHostDockerDaemon(); err != nil { - return errors.Wrap(err, "point host docker daemon") - } if _, err := os.Stat(fName); os.IsNotExist(err) { return errors.Wrapf(err, "error source %s does not exist", fName) } + destination := fmt.Sprintf("%s:%s", ociID, targetDir) cmd := exec.Command(ociBinary, "cp", fName, destination) - err := cmd.Run() - if err != nil { + if err := cmd.Run(); err != nil { return errors.Wrapf(err, "error copying %s into node", fName) } + return nil } // ContainerID returns id of a container name func ContainerID(ociBinary string, nameOrID string) (string, error) { - if err := PointToHostDockerDaemon(); err != nil { - return "", errors.Wrap(err, "point host docker daemon") - } cmd := exec.Command(ociBinary, "inspect", "-f", "{{.Id}}", nameOrID) out, err := cmd.CombinedOutput() + if err != nil { // don't return error if not found, only return empty string if strings.Contains(string(out), "Error: No such object:") || strings.Contains(string(out), "unable to find") { err = nil } out = []byte{} } + return string(out), err } // ContainerExists checks if container name exists (either running or exited) func ContainerExists(ociBin string, name string) (bool, error) { - if err := PointToHostDockerDaemon(); err != nil { - return false, errors.Wrap(err, "point host docker daemon") - } // allow no more than 3 seconds for this. ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) defer cancel() + cmd := exec.CommandContext(ctx, ociBin, "ps", "-a", "--format", "{{.Names}}") out, err := cmd.CombinedOutput() + if ctx.Err() == context.DeadlineExceeded { return false, fmt.Errorf("time out running %s ps -a", ociBin) } + if err != nil { return false, errors.Wrapf(err, string(out)) } + containers := strings.Split(string(out), "\n") for _, c := range containers { if strings.TrimSpace(c) == name { return true, nil } } + return false, nil } // IsCreatedByMinikube returns true if the container was created by minikube // with default assumption that it is not created by minikube when we don't know for sure func IsCreatedByMinikube(ociBinary string, nameOrID string) bool { - if err := PointToHostDockerDaemon(); err != nil { - glog.Warningf("Failed to point to host docker daemon") - return false - } cmd := exec.Command(ociBinary, "inspect", nameOrID, "--format", "{{.Config.Labels}}") out, err := cmd.CombinedOutput() + if err != nil { return false } + if strings.Contains(string(out), fmt.Sprintf("%s:true", CreatedByLabelKey)) { return true } + return false } @@ -294,9 +285,7 @@ func ListOwnedContainers(ociBinary string) ([]string, error) { // inspect return low-level information on containers func inspect(ociBinary string, containerNameOrID, format string) ([]string, error) { - if err := PointToHostDockerDaemon(); err != nil { - return nil, errors.Wrap(err, "point host docker daemon") - } + cmd := exec.Command(ociBinary, "inspect", "-f", format, containerNameOrID) // ... against the "node" container @@ -360,27 +349,28 @@ func generateMountBindings(mounts ...Mount) []string { // isUsernsRemapEnabled checks if userns-remap is enabled in docker func isUsernsRemapEnabled(ociBinary string) (bool, error) { - if err := PointToHostDockerDaemon(); err != nil { - return false, errors.Wrap(err, "point host docker daemon") - } cmd := exec.Command(ociBinary, "info", "--format", "'{{json .SecurityOptions}}'") var buff bytes.Buffer cmd.Stdout = &buff cmd.Stderr = &buff err := cmd.Run() + if err != nil { + return false, nil + } + scanner := bufio.NewScanner(&buff) var lines []string + for scanner.Scan() { lines = append(lines, scanner.Text()) } - if err != nil { - return false, nil - } + if len(lines) > 0 { if strings.Contains(lines[0], "name=userns") { return true, nil } } + return false, nil } @@ -422,9 +412,7 @@ func withPortMappings(portMappings []PortMapping) createOpt { // listContainersByLabel returns all the container names with a specified label func listContainersByLabel(ociBinary string, label string) ([]string, error) { - if err := PointToHostDockerDaemon(); err != nil { - return nil, errors.Wrap(err, "point host docker daemon") - } + // allow no more than 5 seconds for docker ps ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -462,22 +450,21 @@ func PointToHostDockerDaemon() error { // ContainerStatus returns status of a container running,exited,... func ContainerStatus(ociBin string, name string) (string, error) { - if ociBin == Docker { - if err := PointToHostDockerDaemon(); err != nil { - return "", errors.Wrap(err, "point host docker daemon") - } - } // allow no more than 2 seconds for this. when this takes long this means deadline passed ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() + cmd := exec.CommandContext(ctx, ociBin, "inspect", name, "--format={{.State.Status}}") out, err := cmd.CombinedOutput() + if ctx.Err() == context.DeadlineExceeded { glog.Warningf("%s inspect %s took longer than normal. Restarting your %s daemon might fix this issue.", ociBin, name, ociBin) return strings.TrimSpace(string(out)), fmt.Errorf("inspect %s timeout", name) } + if err != nil { return string(out), errors.Wrapf(err, "inspecting container: output %s", out) } + return strings.TrimSpace(string(out)), nil } diff --git a/pkg/drivers/kic/oci/volumes.go b/pkg/drivers/kic/oci/volumes.go index 2163d3bf6598..115ab51a0aea 100644 --- a/pkg/drivers/kic/oci/volumes.go +++ b/pkg/drivers/kic/oci/volumes.go @@ -34,16 +34,13 @@ import ( func DeleteAllVolumesByLabel(ociBin string, label string) []error { var deleteErrs []error glog.Infof("trying to delete all %s volumes with label %s", ociBin, label) - if ociBin == Docker { - if err := PointToHostDockerDaemon(); err != nil { - return []error{errors.Wrap(err, "point host docker daemon")} - } - } vs, err := allVolumesByLabel(ociBin, label) + if err != nil { return []error{fmt.Errorf("listing volumes by label %q: %v", label, err)} } + for _, v := range vs { // allow no more than 3 seconds for this. when this takes long this means deadline passed ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) @@ -57,6 +54,7 @@ func DeleteAllVolumesByLabel(ociBin string, label string) []error { deleteErrs = append(deleteErrs, fmt.Errorf("deleting volume %s: output: %s", v, string(out))) } } + return deleteErrs } @@ -66,11 +64,7 @@ func DeleteAllVolumesByLabel(ociBin string, label string) []error { func PruneAllVolumesByLabel(ociBin string, label string) []error { var deleteErrs []error glog.Infof("trying to prune all %s volumes with label %s", ociBin, label) - if ociBin == Docker { - if err := PointToHostDockerDaemon(); err != nil { - return []error{errors.Wrap(err, "point host docker daemon")} - } - } + // allow no more than 3 seconds for this. when this takes long this means deadline passed ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) defer cancel() @@ -80,10 +74,12 @@ func PruneAllVolumesByLabel(ociBin string, label string) []error { if out, err := cmd.CombinedOutput(); err != nil { deleteErrs = append(deleteErrs, errors.Wrapf(err, "prune volume by label %s: %s", label, string(out))) } + if ctx.Err() == context.DeadlineExceeded { glog.Warningf("pruning volume with label %s took longer than normal. Restarting your %s daemon might fix this issue.", label, ociBin) deleteErrs = append(deleteErrs, fmt.Errorf("prune deadline exceeded for %s", label)) } + return deleteErrs } @@ -106,9 +102,6 @@ func allVolumesByLabel(ociBin string, label string) ([]string, error) { // ExtractTarballToVolume runs a docker image imageName which extracts the tarball at tarballPath // to the volume named volumeName func ExtractTarballToVolume(tarballPath, volumeName, imageName string) error { - if err := PointToHostDockerDaemon(); err != nil { - return errors.Wrap(err, "point host docker-daemon") - } cmd := exec.Command(Docker, "run", "--rm", "--entrypoint", "/usr/bin/tar", "-v", fmt.Sprintf("%s:/preloaded.tar:ro", tarballPath), "-v", fmt.Sprintf("%s:/extractDir", volumeName), imageName, "-I", "lz4", "-xvf", "/preloaded.tar", "-C", "/extractDir") if out, err := cmd.CombinedOutput(); err != nil { return errors.Wrapf(err, "output %s", string(out)) @@ -120,9 +113,6 @@ func ExtractTarballToVolume(tarballPath, volumeName, imageName string) error { // Caution ! if volume already exists does NOT return an error and will not apply the minikube labels on it. // TODO: this should be fixed as a part of https://github.com/kubernetes/minikube/issues/6530 func createDockerVolume(profile string, nodeName string) error { - if err := PointToHostDockerDaemon(); err != nil { - return errors.Wrap(err, "point host docker daemon") - } cmd := exec.Command(Docker, "volume", "create", nodeName, "--label", fmt.Sprintf("%s=%s", ProfileLabelKey, profile), "--label", fmt.Sprintf("%s=%s", CreatedByLabelKey, "true")) if out, err := cmd.CombinedOutput(); err != nil { return errors.Wrapf(err, "output %s", string(out)) From e027dfc4d2be649621544ee0d199f4f743d910f0 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Fri, 6 Mar 2020 15:57:12 -0800 Subject: [PATCH 2/2] More unused code cleanup --- pkg/drivers/kic/oci/oci.go | 44 +++++++++----------------------------- 1 file changed, 10 insertions(+), 34 deletions(-) diff --git a/pkg/drivers/kic/oci/oci.go b/pkg/drivers/kic/oci/oci.go index 90f7a656be56..2a03777294ba 100644 --- a/pkg/drivers/kic/oci/oci.go +++ b/pkg/drivers/kic/oci/oci.go @@ -84,7 +84,6 @@ func DeleteContainer(ociBin string, name string) error { // CreateContainerNode creates a new container node func CreateContainerNode(p CreateParams) error { - runArgs := []string{ "-d", // run the container detached "-t", // allocate a tty for entrypoint logs @@ -138,30 +137,17 @@ func CreateContainerNode(p CreateParams) error { // adds node specific args runArgs = append(runArgs, p.ExtraArgs...) - enabled, err := isUsernsRemapEnabled(p.OCIBinary) - if err != nil { - glog.Warningf("Failed to detect if userns is enabled: %v", err) - } - if enabled { + if enabled := isUsernsRemapEnabled(p.OCIBinary); enabled { // We need this argument in order to make this command work // in systems that have userns-remap enabled on the docker daemon runArgs = append(runArgs, "--userns=host") } - _, err = createContainer(p.OCIBinary, - p.Image, - withRunArgs(runArgs...), - withMounts(p.Mounts), - withPortMappings(p.PortMappings), - ) - if err != nil { - return errors.Wrap(err, "create a kic node") - } - return nil + return createContainer(p.OCIBinary, p.Image, withRunArgs(runArgs...), withMounts(p.Mounts), withPortMappings(p.PortMappings)) } // CreateContainer creates a container with "docker/podman run" -func createContainer(ociBinary string, image string, opts ...createOpt) ([]string, error) { +func createContainer(ociBinary string, image string, opts ...createOpt) error { o := &createOpts{} for _, opt := range opts { o = opt(o) @@ -185,23 +171,13 @@ func createContainer(ociBinary string, image string, opts ...createOpt) ([]strin args = append(args, runArgs...) args = append(args, image) args = append(args, o.ContainerArgs...) - cmd := exec.Command(ociBinary, args...) - var buff bytes.Buffer - cmd.Stdout = &buff - cmd.Stderr = &buff - err := cmd.Run() - scanner := bufio.NewScanner(&buff) - var output []string - - for scanner.Scan() { - output = append(output, scanner.Text()) - } + out, err := exec.Command(ociBinary, args...).CombinedOutput() if err != nil { - return output, errors.Wrapf(err, "args: %v output: %s ", args, output) + return errors.Wrapf(err, "failed args: %v output: %s", args, out) } - return output, nil + return nil } // Copy copies a local asset into the container @@ -348,14 +324,14 @@ func generateMountBindings(mounts ...Mount) []string { } // isUsernsRemapEnabled checks if userns-remap is enabled in docker -func isUsernsRemapEnabled(ociBinary string) (bool, error) { +func isUsernsRemapEnabled(ociBinary string) bool { cmd := exec.Command(ociBinary, "info", "--format", "'{{json .SecurityOptions}}'") var buff bytes.Buffer cmd.Stdout = &buff cmd.Stderr = &buff err := cmd.Run() if err != nil { - return false, nil + return false } scanner := bufio.NewScanner(&buff) @@ -367,11 +343,11 @@ func isUsernsRemapEnabled(ociBinary string) (bool, error) { if len(lines) > 0 { if strings.Contains(lines[0], "name=userns") { - return true, nil + return true } } - return false, nil + return false } func generatePortMappings(portMappings ...PortMapping) []string {