Merge pull request #7819 from medyagh/fix_delete_bug

kic: fix delete -p not cleaning up & add integration test

cmd/minikube/cmd/delete.go

@@ -147,6 +147,8 @@ func runDelete(cmd *cobra.Command, args []string) {
 			out.ErrT(out.Meh, `"{{.name}}" profile does not exist, trying anyways.`, out.V{"name": cname})
 		}
 
+		deletePossibleKicLeftOver(cname)
+
 		errs := DeleteProfiles([]*config.Profile{profile})
 		if len(errs) > 0 {
 			HandleDeletionErrors(errs)
@@ -189,20 +191,30 @@ func DeleteProfiles(profiles []*config.Profile) []error {
 	return errs
 }
 
-func deleteProfileContainersAndVolumes(name string) {
+func deletePossibleKicLeftOver(name string) {
 	delLabel := fmt.Sprintf("%s=%s", oci.ProfileLabelKey, name)
-	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", name, errs)
-	}
-	errs = oci.DeleteAllVolumesByLabel(oci.Docker, delLabel)
-	if errs != nil { // it will not error if there is nothing to delete
-		glog.Warningf("error deleting volumes (might be okay).\nTo see the list of volumes run: 'docker volume ls'\n:%v", errs)
-	}
+	for _, bin := range []string{oci.Docker, oci.Podman} {
+		cs, err := oci.ListContainersByLabel(bin, delLabel)
+		if err == nil && len(cs) > 0 {
+			for _, c := range cs {
+				out.T(out.DeletingHost, `Deleting container "{{.name}}" ...`, out.V{"name": name})
+				err := oci.DeleteContainer(bin, c)
+				if err != nil { // it will error if there is no container to delete
+					glog.Errorf("error deleting container %q. you might want to delete that manually :\n%v", name, err)
+				}
 
-	errs = oci.PruneAllVolumesByLabel(oci.Docker, delLabel)
-	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)
+			}
+		}
+
+		errs := oci.DeleteAllVolumesByLabel(bin, delLabel)
+		if errs != nil { // it will not error if there is nothing to delete
+			glog.Warningf("error deleting volumes (might be okay).\nTo see the list of volumes run: 'docker volume ls'\n:%v", errs)
+		}
+
+		errs = oci.PruneAllVolumesByLabel(bin, delLabel)
+		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)
+		}
 	}
 }
 
@@ -212,7 +224,7 @@ func deleteProfile(profile *config.Profile) error {
 		// if driver is oci driver, delete containers and volumes
 		if driver.IsKIC(profile.Config.Driver) {
 			out.T(out.DeletingHost, `Deleting "{{.profile_name}}" in {{.driver_name}} ...`, out.V{"profile_name": profile.Name, "driver_name": profile.Config.Driver})
-			deleteProfileContainersAndVolumes(profile.Name)
+			deletePossibleKicLeftOver(profile.Name)
 		}
 	}
 

pkg/drivers/kic/oci/oci.go

@@ -43,7 +43,7 @@ import (
 func DeleteContainersByLabel(ociBin string, label string) []error {
 	var deleteErrs []error
 
-	cs, err := listContainersByLabel(ociBin, label)
+	cs, err := ListContainersByLabel(ociBin, label)
 	if err != nil {
 		return []error{fmt.Errorf("listing containers by label %q", label)}
 	}
@@ -322,7 +322,7 @@ func IsCreatedByMinikube(ociBinary string, nameOrID string) bool {
 
 // ListOwnedContainers lists all the containres that kic driver created on user's machine using a label
 func ListOwnedContainers(ociBinary string) ([]string, error) {
-	return listContainersByLabel(ociBinary, ProfileLabelKey)
+	return ListContainersByLabel(ociBinary, ProfileLabelKey)
 }
 
 // inspect return low-level information on containers
@@ -452,8 +452,8 @@ func withPortMappings(portMappings []PortMapping) createOpt {
 	}
 }
 
-// listContainersByLabel returns all the container names with a specified label
-func listContainersByLabel(ociBinary string, label string) ([]string, error) {
+// ListContainersByLabel returns all the container names with a specified label
+func ListContainersByLabel(ociBinary string, label string) ([]string, error) {
 	stdout, err := WarnIfSlow(ociBinary, "ps", "-a", "--filter", fmt.Sprintf("label=%s", label), "--format", "{{.Names}}")
 	if err != nil {
 		return nil, err

test/integration/main.go

@@ -69,9 +69,19 @@ func HyperVDriver() bool {
 	return strings.Contains(*startArgs, "--driver=hyperv") || strings.Contains(*startArgs, "--vm-driver=hyperv")
 }
 
+// DockerDriver returns whether or not this test is using the docker or podman driver
+func DockerDriver() bool {
+	return strings.Contains(*startArgs, "--driver=docker") || strings.Contains(*startArgs, "--vm-driver=docker")
+}
+
+// PodmanDriver returns whether or not this test is using the docker or podman driver
+func PodmanDriver() bool {
+	return strings.Contains(*startArgs, "--vm-driver=podman") || strings.Contains(*startArgs, "driver=podman")
+}
+
 // KicDriver returns whether or not this test is using the docker or podman driver
 func KicDriver() bool {
-	return strings.Contains(*startArgs, "--driver=docker") || strings.Contains(*startArgs, "--vm-driver=docker") || strings.Contains(*startArgs, "--vm-driver=podman") || strings.Contains(*startArgs, "driver=podman")
+	return DockerDriver() || PodmanDriver()
 }
 
 // NeedsPortForward returns access to endpoints with this driver needs port forwarding

test/integration/pause_test.go

@@ -20,6 +20,7 @@ package integration
 
 import (
 	"context"
+	"encoding/json"
 	"os/exec"
 	"strings"
 	"testing"
@@ -45,6 +46,7 @@ func TestPause(t *testing.T) {
 			{"Unpause", validateUnpause},
 			{"PauseAgain", validatePause},
 			{"DeletePaused", validateDelete},
+			{"VerifyDeletedResources", validateVerifyDeleted},
 		}
 		for _, tc := range tests {
 			tc := tc
@@ -96,10 +98,51 @@ func validateUnpause(ctx context.Context, t *testing.T, profile string) {
 }
 
 func validateDelete(ctx context.Context, t *testing.T, profile string) {
-	// vervose logging because this might go wrong, if container get stuck
 	args := []string{"delete", "-p", profile, "--alsologtostderr", "-v=5"}
 	rr, err := Run(t, exec.CommandContext(ctx, Target(), args...))
 	if err != nil {
 		t.Errorf("failed to delete minikube with args: %q : %v", rr.Command(), err)
 	}
 }
+
+// make sure no left over left after deleting a profile such as containers or volumes
+func validateVerifyDeleted(ctx context.Context, t *testing.T, profile string) {
+	rr, err := Run(t, exec.CommandContext(ctx, Target(), "profile", "list", "--output", "json"))
+	if err != nil {
+		t.Errorf("failed to list profiles with json format after it was deleted. args %q: %v", rr.Command(), err)
+	}
+
+	var jsonObject map[string][]map[string]interface{}
+	if err := json.Unmarshal(rr.Stdout.Bytes(), &jsonObject); err != nil {
+		t.Errorf("failed to decode json from profile list: args %q: %v", rr.Command(), err)
+	}
+	validProfiles := jsonObject["valid"]
+	profileExists := false
+	for _, profileObject := range validProfiles {
+		if profileObject["Name"] == profile {
+			profileExists = true
+			break
+		}
+	}
+	if profileExists {
+		t.Errorf("expected the deleted profile %q not to show up in profile list but it does! output: %s . args: %q", profile, rr.Stdout.String(), rr.Command())
+	}
+
+	if KicDriver() {
+		bin := "docker"
+		if PodmanDriver() {
+			bin = "podman"
+		}
+		rr, err := Run(t, exec.CommandContext(ctx, bin, "ps", "-a"))
+		if err == nil && strings.Contains(rr.Output(), profile) {
+			t.Errorf("expected container %q not to exist in output of %s but it does output: %s.", profile, rr.Command(), rr.Output())
+		}
+
+		rr, err = Run(t, exec.CommandContext(ctx, bin, "volume", "inspect", profile))
+		if err == nil {
+			t.Errorf("expected to see error and volume %q to not exist after deletion but got no error and this output: %s", rr.Command(), rr.Output())
+		}
+
+	}
+
+}