From 4709af55df13ad06af03d125198905f804d22663 Mon Sep 17 00:00:00 2001 From: Medya Gh Date: Wed, 26 Feb 2020 12:44:25 -0800 Subject: [PATCH] resolve review comments --- cmd/minikube/cmd/delete.go | 5 ++--- pkg/drivers/kic/kic.go | 13 +++++++++-- pkg/drivers/kic/oci/network.go | 4 ++-- pkg/drivers/kic/oci/oci.go | 40 +++++++++++++++++----------------- pkg/drivers/kic/oci/volumes.go | 16 +++++++------- pkg/minikube/machine/delete.go | 33 +++++++++++++++++++++++----- 6 files changed, 71 insertions(+), 40 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index fe90c6d578..f64579112f 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -116,7 +116,7 @@ func runDelete(cmd *cobra.Command, args []string) { exit.UsageT("usage: minikube delete --all") } delLabel := fmt.Sprintf("%s=%s", oci.CreatedByLabelKey, "true") - errs := oci.DeleteAllContainersByLabel(oci.Docker, delLabel) + errs := oci.DeleteContainersByLabel(oci.Docker, delLabel) if len(errs) > 0 { // it will error if there is no container to delete glog.Infof("error delete containers by label %q (might be okay): %+v", delLabel, err) } @@ -194,7 +194,7 @@ func deleteProfile(profile *pkg_config.Profile) error { viper.Set(pkg_config.MachineProfile, profile.Name) delLabel := fmt.Sprintf("%s=%s", oci.ProfileLabelKey, profile.Name) - errs := oci.DeleteAllContainersByLabel(oci.Docker, delLabel) + errs := oci.DeleteContainersByLabel(oci.Docker, delLabel) if errs != nil { // it will error if there is no container to delete glog.Infof("error deleting containers for %s (might be okay):\n%v", profile.Name, errs) } @@ -207,7 +207,6 @@ func deleteProfile(profile *pkg_config.Profile) error { if len(errs) > 0 { // it will not error if there is nothing to delete glog.Warningf("error pruning volume (might be okay):\n%v", errs) } - api, err := machine.NewAPIClient() if err != nil { delErr := profileDeletionErr(profile.Name, fmt.Sprintf("error getting client %v", err)) diff --git a/pkg/drivers/kic/kic.go b/pkg/drivers/kic/kic.go index 0ecb6a2ade..a5abcd372d 100644 --- a/pkg/drivers/kic/kic.go +++ b/pkg/drivers/kic/kic.go @@ -17,11 +17,13 @@ limitations under the License. package kic import ( + "context" "fmt" "net" "os/exec" "strconv" "strings" + "time" "github.com/docker/machine/libmachine/drivers" "github.com/docker/machine/libmachine/log" @@ -181,11 +183,18 @@ func (d *Driver) GetURL() (string, error) { // GetState returns the state that the host is in (running, stopped, etc) func (d *Driver) GetState() (state.State, error) { if err := oci.PointToHostDockerDaemon(); err != nil { - return state.Error, errors.Wrap(err, "point host docker-daemon") + return state.Error, 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.Command(d.NodeConfig.OCIBinary, "inspect", "-f", "{{.State.Status}}", d.MachineName) + cmd := exec.CommandContext(ctx, d.NodeConfig.OCIBinary, "inspect", "-f", "{{.State.Status}}", d.MachineName) out, err := cmd.CombinedOutput() + if ctx.Err() == context.DeadlineExceeded { + glog.Errorf("GetState for %s took longer than normal. Restarting your %s daemon might fix this issue.", d.MachineName, d.OCIBinary) + return state.Error, fmt.Errorf("inspect %s timeout", d.MachineName) + } o := strings.TrimSpace(string(out)) if err != nil { return state.Error, errors.Wrapf(err, "get container %s status", d.MachineName) diff --git a/pkg/drivers/kic/oci/network.go b/pkg/drivers/kic/oci/network.go index 5e27528b5f..4e28161d84 100644 --- a/pkg/drivers/kic/oci/network.go +++ b/pkg/drivers/kic/oci/network.go @@ -43,7 +43,7 @@ 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") + return nil, errors.Wrap(err, "point host docker daemon") } cmd := exec.Command(ociBin, "exec", "-t", containerName, "dig", "+short", dns) out, err := cmd.CombinedOutput() @@ -59,7 +59,7 @@ func digDNS(ociBin, containerName, dns string) (net.IP, error) { // 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") + return nil, errors.Wrap(err, "point host docker daemon") } cmd := exec.Command(Docker, "network", "ls", "--filter", "name=bridge", "--format", "{{.ID}}") out, err := cmd.CombinedOutput() diff --git a/pkg/drivers/kic/oci/oci.go b/pkg/drivers/kic/oci/oci.go index fa41c26fc2..784e3250c2 100644 --- a/pkg/drivers/kic/oci/oci.go +++ b/pkg/drivers/kic/oci/oci.go @@ -36,13 +36,13 @@ import ( "strings" ) -// DeleteAllContainersByLabel deletes all containers that have a specific label +// DeleteContainersByLabel deletes all containers that have a specific label // if there no containers found with the given label, it will return nil -func DeleteAllContainersByLabel(ociBin string, label string) []error { +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")} + return []error{errors.Wrap(err, "point host docker daemon")} } } cs, err := listContainersByLabel(ociBin, label) @@ -53,12 +53,12 @@ func DeleteAllContainersByLabel(ociBin string, label string) []error { return nil } for _, c := range cs { - _, err := containerStatus(ociBin, c) + _, err := ContainerStatus(ociBin, c) // only try to delete if docker/podman inspect returns - // if it doesn't it means docker-daemon is stuck and needs restart + // if it doesn't it means docker daemon is stuck and needs restart if err != nil { - deleteErrs = append(deleteErrs, errors.Wrapf(err, "delete container %s: -daemon is stuck.", c, ociBin)) - glog.Errorf("%s-daemon seems to be stuck. Please try restarting your %s.", ociBin, ociBin) + deleteErrs = append(deleteErrs, errors.Wrapf(err, "delete container %s: %s daemon is stuck. please try again!", c, ociBin)) + glog.Errorf("%s daemon seems to be stuck. Please try restarting your %s.", ociBin, ociBin) } else { cmd := exec.Command(ociBin, "rm", "-f", "-v", c) if out, err := cmd.CombinedOutput(); err != nil { @@ -73,7 +73,7 @@ func DeleteAllContainersByLabel(ociBin string, label 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") + return errors.Wrap(err, "point host docker daemon") } runArgs := []string{ @@ -152,7 +152,7 @@ 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") + return nil, errors.Wrap(err, "point host docker daemon") } o := &createOpts{} @@ -196,7 +196,7 @@ func createContainer(ociBinary string, image string, opts ...createOpt) ([]strin // 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") + 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) @@ -217,7 +217,7 @@ func Copy(ociBinary string, ociID string, targetDir string, fName string) error // 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") + return 0, errors.Wrap(err, "point host docker daemon") } var out []byte var err error @@ -272,7 +272,7 @@ 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") + 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}}") @@ -292,7 +292,7 @@ func dockerContainerIP(name string) (string, string, error) { // 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") + return "", errors.Wrap(err, "point host docker daemon") } cmd := exec.Command(ociBinary, "inspect", "-f", "{{.Id}}", nameOrID) id, err := cmd.CombinedOutput() @@ -310,7 +310,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") + return nil, errors.Wrap(err, "point host docker daemon") } cmd := exec.Command(ociBinary, "inspect", "-f", format, @@ -376,7 +376,7 @@ 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") + return false, errors.Wrap(err, "point host docker daemon") } cmd := exec.Command(ociBinary, "info", "--format", "'{{json .SecurityOptions}}'") var buff bytes.Buffer @@ -438,7 +438,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") + 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) @@ -475,11 +475,11 @@ func PointToHostDockerDaemon() error { return nil } -// containerStatus returns status of a container running,exited,... -func containerStatus(ociBin string, name string) (string, 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") + return "", errors.Wrap(err, "point host docker daemon") } } // allow no more than 2 seconds for this. when this takes long this means deadline passed @@ -488,7 +488,7 @@ func containerStatus(ociBin string, name string) (string, error) { 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 docker daemon might fix this issue.") + 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 { diff --git a/pkg/drivers/kic/oci/volumes.go b/pkg/drivers/kic/oci/volumes.go index 1181aa0652..d24bd7bb96 100644 --- a/pkg/drivers/kic/oci/volumes.go +++ b/pkg/drivers/kic/oci/volumes.go @@ -36,7 +36,7 @@ func DeleteAllVolumesByLabel(ociBin string, label string) []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")} + return []error{errors.Wrap(err, "point host docker daemon")} } } @@ -68,22 +68,22 @@ func PruneAllVolumesByLabel(ociBin string, label string) []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")} + return []error{errors.Wrap(err, "point host docker daemon")} } } - // allow no more than 5 seconds for this. when this takes long this means deadline passed - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + // 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() // try to prune afterwards just in case delete didn't go through cmd := exec.CommandContext(ctx, ociBin, "volume", "prune", "-f", "--filter", "label="+label) + 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)) } - if out, err := cmd.CombinedOutput(); err != nil { - deleteErrs = append(deleteErrs, errors.Wrapf(err, "prune volume by label %s: %s", label, string(out))) - } return deleteErrs } @@ -108,7 +108,7 @@ func allVolumesByLabel(ociBin string, label string) ([]string, error) { // TODO: this should be fixed as a part of https://github.com/kubernetes/minikube/issues/6530 func createDockerVolume(name string) error { if err := PointToHostDockerDaemon(); err != nil { - return errors.Wrap(err, "point host docker-daemon") + return errors.Wrap(err, "point host docker daemon") } cmd := exec.Command(Docker, "volume", "create", name, "--label", fmt.Sprintf("%s=%s", ProfileLabelKey, name), "--label", fmt.Sprintf("%s=%s", CreatedByLabelKey, "true")) if out, err := cmd.CombinedOutput(); err != nil { diff --git a/pkg/minikube/machine/delete.go b/pkg/minikube/machine/delete.go index 9a7fa00a81..c18e4c9183 100644 --- a/pkg/minikube/machine/delete.go +++ b/pkg/minikube/machine/delete.go @@ -17,7 +17,9 @@ limitations under the License. package machine import ( + "context" "os/exec" + "time" "github.com/docker/machine/libmachine" "github.com/docker/machine/libmachine/mcnerror" @@ -29,10 +31,30 @@ import ( "k8s.io/minikube/pkg/minikube/out" ) -// deleteOrphanedKIC attempts to delete an orphaned docker instance -func deleteOrphanedKIC(name string) { - cmd := exec.Command(oci.Docker, "rm", "-f", "-v", name) - err := cmd.Run() +// deleteOrphanedKIC attempts to delete an orphaned docker instance for machines without a config file +// used as last effort clean up not returning errors, wont warn user. +func deleteOrphanedKIC(ociBin string, name string) { + if !(ociBin == oci.Podman || ociBin == oci.Docker) { + return + } + if ociBin == oci.Docker { + if err := oci.PointToHostDockerDaemon(); err != nil { + glog.Infof("Failed to point to host docker-damon: %v", err) + return + } + } + + _, err := oci.ContainerStatus(ociBin, name) + if err != nil { + glog.Infof("couldn't inspect container %q before deleting, %s-daemon might needs a restart!: %v", name, ociBin, err) + return + } + // allow no more than 5 seconds for delting the container + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, ociBin, "rm", "-f", "-v", name) + err = cmd.Run() if err == nil { glog.Infof("Found stale kic container and successfully cleaned it up!") } @@ -42,7 +64,8 @@ func deleteOrphanedKIC(name string) { func DeleteHost(api libmachine.API, machineName string) error { host, err := api.Load(machineName) if err != nil && host == nil { - deleteOrphanedKIC(machineName) + deleteOrphanedKIC(oci.Docker, machineName) + deleteOrphanedKIC(oci.Podman, machineName) // Keep going even if minikube does not know about the host }