From ab81991ff6f6eaa922133fcd73c7bac5053900cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=94=A6=E5=8D=97=E8=B7=AF=E4=B9=8B=E8=8A=B1?= Date: Mon, 22 Jul 2024 00:32:49 +0200 Subject: [PATCH] fix: fix empty tarball when generating image save --- pkg/minikube/cruntime/containerd.go | 2 +- pkg/minikube/cruntime/cri.go | 29 +++++++++++++++++++++++++--- pkg/minikube/cruntime/docker.go | 16 +++++++++++---- pkg/minikube/image/image.go | 20 ++++++++++++++++++- pkg/minikube/machine/cache_images.go | 19 +++++++++++++++--- test/integration/functional_test.go | 10 +++++++--- 6 files changed, 81 insertions(+), 15 deletions(-) diff --git a/pkg/minikube/cruntime/containerd.go b/pkg/minikube/cruntime/containerd.go index 73df60d4c042..4cb0ad665966 100644 --- a/pkg/minikube/cruntime/containerd.go +++ b/pkg/minikube/cruntime/containerd.go @@ -265,7 +265,7 @@ func (r *Containerd) Disable() error { // ImageExists checks if image exists based on image name and optionally image sha func (r *Containerd) ImageExists(name string, sha string) bool { klog.Infof("Checking existence of image with name %q and sha %q", name, sha) - c := exec.Command("sudo", "ctr", "-n=k8s.io", "images", "check") + c := exec.Command("sudo", "ctr", "-n=k8s.io", "images", "ls", fmt.Sprintf("name==%s", name)) // note: image name and image id's sha can be on different lines in ctr output if rr, err := r.Runner.RunCmd(c); err != nil || !strings.Contains(rr.Output(), name) || diff --git a/pkg/minikube/cruntime/cri.go b/pkg/minikube/cruntime/cri.go index fa3b2c1cb49c..3dfc50dec35f 100644 --- a/pkg/minikube/cruntime/cri.go +++ b/pkg/minikube/cruntime/cri.go @@ -220,10 +220,28 @@ func removeCRIImage(cr CommandRunner, name string) error { crictl := getCrictlPath(cr) args := append([]string{crictl, "rmi"}, name) c := exec.Command("sudo", args...) - if _, err := cr.RunCmd(c); err != nil { - return errors.Wrap(err, "crictl") + var err error + if _, err = cr.RunCmd(c); err == nil { + return nil } - return nil + // the reason why we are doing this is that + // unlike other tool, podman assume the image has a localhost registry (not docker.io) + // if the image is loaded with a tarball without a registry specified in tag + // see https://github.com/containers/podman/issues/15974 + + // then retry with dockerio prefix + if _, err := cr.RunCmd(exec.Command("sudo", crictl, "rmi", AddDockerIO(name))); err == nil { + return nil + } + + // then retry with localhost prefix + if _, err := cr.RunCmd(exec.Command("sudo", crictl, "rmi", AddLocalhostPrefix(name))); err == nil { + + return nil + } + + // if all of above failed, return original error + return errors.Wrap(err, "crictl") } // stopCRIContainers stops containers using crictl @@ -358,3 +376,8 @@ func checkCNIPlugins(kubernetesVersion semver.Version) error { _, err := os.Stat("/opt/cni/bin") return err } + +// Add localhost prefix if the registry part is missing +func AddLocalhostPrefix(name string) string { + return addRegistryPreix(name, "localhost") +} diff --git a/pkg/minikube/cruntime/docker.go b/pkg/minikube/cruntime/docker.go index c2bf1c7b7bdb..f22dc9738562 100644 --- a/pkg/minikube/cruntime/docker.go +++ b/pkg/minikube/cruntime/docker.go @@ -292,7 +292,7 @@ func (r *Docker) ListImages(ListImagesOptions) ([]ListImage, error) { result = append(result, ListImage{ ID: strings.TrimPrefix(jsonImage.ID, "sha256:"), RepoDigests: []string{}, - RepoTags: []string{addDockerIO(repoTag)}, + RepoTags: []string{AddDockerIO(repoTag)}, Size: fmt.Sprintf("%d", size), }) } @@ -696,14 +696,22 @@ func dockerImagesPreloaded(runner command.Runner, images []string) bool { } // Add docker.io prefix -func addDockerIO(name string) string { +func AddDockerIO(name string) string { + return addRegistryPreix(name, "docker.io") +} +func addRegistryPreix(name string, prefix string) string { + // we seperate the image name following this logic + // https://pkg.go.dev/github.com/distribution/reference#ParseNormalizedNamed var reg, usr, img string p := strings.SplitN(name, "/", 2) - if len(p) > 1 && strings.Contains(p[0], ".") { + // containing . means that it is a valid url for registry(e.g. xxx.io) + // containing : means it contains some port number (e.g. xxx:5432) + // it may also start with localhost, which is also regarded as a valid registry + if len(p) > 1 && (strings.ContainsAny(p[0], ".:") || strings.Contains(p[0], "localhost")) { reg = p[0] img = p[1] } else { - reg = "docker.io" + reg = prefix img = name p = strings.SplitN(img, "/", 2) if len(p) > 1 { diff --git a/pkg/minikube/image/image.go b/pkg/minikube/image/image.go index 0db355616488..b567036c63b5 100644 --- a/pkg/minikube/image/image.go +++ b/pkg/minikube/image/image.go @@ -19,6 +19,7 @@ package image import ( "context" "fmt" + "io" "os" "path/filepath" "runtime" @@ -207,7 +208,24 @@ func UploadCachedImage(imgName string) error { klog.Infof("error parsing image name %s tag %v ", imgName, err) return err } - return uploadImage(tag, imagePathInCache(imgName)) + if err := uploadImage(tag, imagePathInCache(imgName)); err != nil { + // this time try determine image tags from tarball + + manifest, err := tarball.LoadManifest(func() (io.ReadCloser, error) { + return os.Open(imagePathInCache(imgName)) + }) + if err != nil || len(manifest) == 0 || len(manifest[0].RepoTags) == 0 { + return fmt.Errorf("failed to determine the image tag from tarball, err: %v", err) + } + + tag, err = name.NewTag(manifest[0].RepoTags[0], name.WeakValidation) + if err != nil { + klog.Infof("error parsing image name: %s ", err.Error()) + return err + } + return uploadImage(tag, imagePathInCache(imgName)) + } + return nil } func uploadImage(tag name.Tag, p string) error { diff --git a/pkg/minikube/machine/cache_images.go b/pkg/minikube/machine/cache_images.go index 4aab4766574c..2e0c6be38002 100644 --- a/pkg/minikube/machine/cache_images.go +++ b/pkg/minikube/machine/cache_images.go @@ -335,7 +335,7 @@ func removeExistingImage(r cruntime.Manager, src string, imgName string) error { } errStr := strings.ToLower(err.Error()) - if !strings.Contains(errStr, "no such image") { + if !strings.Contains(errStr, "no such image") && !strings.Contains(errStr, "unable to remove the image") { return errors.Wrap(err, "removing image") } @@ -472,9 +472,22 @@ func transferAndSaveImage(cr command.Runner, k8s config.KubernetesConfig, dst st if err != nil { return errors.Wrap(err, "runtime") } + found := false + // the reason why we are doing this is that + // unlike other tool, podman assume the image has a localhost registry (not docker.io) + // if the image is loaded with a tarball without a registry specified in tag + // see https://github.com/containers/podman/issues/15974 + tryImageExist := []string{imgName, cruntime.AddDockerIO(imgName), cruntime.AddLocalhostPrefix(imgName)} + for _, imgName = range tryImageExist { + if r.ImageExists(imgName, "") { + found = true + break + } + } - if !r.ImageExists(imgName, "") { - return errors.Errorf("image %s not found", imgName) + if !found { + // if all of this failed, return the original error + return fmt.Errorf("image not found %s", imgName) } klog.Infof("Saving image to: %s", dst) diff --git a/test/integration/functional_test.go b/test/integration/functional_test.go index 8d08e5770280..3792a8073344 100644 --- a/test/integration/functional_test.go +++ b/test/integration/functional_test.go @@ -55,9 +55,10 @@ import ( "github.com/phayes/freeport" "github.com/pkg/errors" "golang.org/x/build/kubernetes/api" + "k8s.io/minikube/pkg/minikube/cruntime" ) -const echoServerImg = "docker.io/kicbase/echo-server" +const echoServerImg = "kicbase/echo-server" // validateFunc are for subtests that share a single setup type validateFunc func(context.Context, *testing.T, string) @@ -424,8 +425,11 @@ func validateImageCommands(ctx context.Context, t *testing.T, profile string) { if err != nil { t.Fatalf("saving image from minikube to daemon: %v\n%s", err, rr.Output()) } - - rr, err = Run(t, exec.CommandContext(ctx, "docker", "image", "inspect", taggedImage)) + imageToDelete := taggedImage + if ContainerRuntime() == "crio" { + imageToDelete = cruntime.AddLocalhostPrefix(imageToDelete) + } + rr, err = Run(t, exec.CommandContext(ctx, "docker", "image", "inspect", imageToDelete)) if err != nil { t.Fatalf("expected image to be loaded into Docker, but image was not found: %v\n%s", err, rr.Output()) }