Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix empty tarball when generating image save #19312

Merged
merged 1 commit into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/minikube/cruntime/containerd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) ||
medyagh marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
29 changes: 26 additions & 3 deletions pkg/minikube/cruntime/cri.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
}
16 changes: 12 additions & 4 deletions pkg/minikube/cruntime/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@
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),
})
}
Expand Down Expand Up @@ -696,14 +696,22 @@
}

// 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

Check failure on line 703 in pkg/minikube/cruntime/docker.go

View workflow job for this annotation

GitHub Actions / lint

`seperate` is a misspelling of `separate` (misspell)
// 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 {
Expand Down
20 changes: 19 additions & 1 deletion pkg/minikube/image/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package image
import (
"context"
"fmt"
"io"
"os"
"path/filepath"
"runtime"
Expand Down Expand Up @@ -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 {
Expand Down
19 changes: 16 additions & 3 deletions pkg/minikube/machine/cache_images.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down Expand Up @@ -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)
Expand Down
10 changes: 7 additions & 3 deletions test/integration/functional_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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())
}
Expand Down
Loading