Skip to content

Commit

Permalink
fix: fix empty tarball when generating image save
Browse files Browse the repository at this point in the history
  • Loading branch information
ComradeProgrammer authored and medyagh committed Aug 7, 2024
1 parent 78ebd9a commit ab81991
Show file tree
Hide file tree
Showing 6 changed files with 81 additions and 15 deletions.
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) ||
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 @@ 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),
})
}
Expand Down Expand Up @@ -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

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)

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

0 comments on commit ab81991

Please sign in to comment.