kind load remove duplicate images names #2536
Conversation
|
/assign @BenTheElder |
k8s-ci-robot
added
approved
| @@ -24,6 +24,8 @@ import ( | |||
|
|
|||
| "github.com/spf13/cobra" | |||
|
|
|||
| "k8s.io/apimachinery/pkg/util/sets" | |||
There was a problem hiding this comment.
ughhh we need to get rid of this dependency
There was a problem hiding this comment.
grep -r machinery pkg/ | grep sets
pkg/build/nodeimage/buildcontext.go: "k8s.io/apimachinery/pkg/util/sets"
pkg/cluster/internal/kubeconfig/internal/kubeconfig/paths.go: "k8s.io/apimachinery/pkg/util/sets"
pkg/cluster/internal/providers/docker/provider.go: "k8s.io/apimachinery/pkg/util/sets"
pkg/cluster/internal/providers/podman/provider.go: "k8s.io/apimachinery/pkg/util/sets"
pkg/cluster/internal/providers/common/images_test.go: "k8s.io/apimachinery/pkg/util/sets"
pkg/cluster/internal/providers/common/images.go: "k8s.io/apimachinery/pkg/util/sets"
pkg/cmd/kind/load/docker-image/docker-image.go: "k8s.io/apimachinery/pkg/util/sets"
There was a problem hiding this comment.
pkg/internal/sets after #2539
There was a problem hiding this comment.
(please group internal imports on their own, at the bottom, I had hoped to avoid internal imports in cmd/ but I think we can make an exception here, people re-implementing the cmd/ logic using the kind public APIs can just import k8s.io/apimachinery sets themselves if they need to implement this bit of logic)
|
I'm not sure this optimization is necessary, the check for presense is cheap (also the user can just dedupe themselves?) and docker etc. dedupe layers when save/load. |
the bug is legit and reproducible 🤷 |
it does not reproduce for me? what are you seeing? we print two messages about loading the image (that just means we detected it wasn't loaded before loading it), we still only load the image once because we make one docker save call and docker dedupes (as does containerd), this seems to be WAI to me. if the user asks to load it twice we check both names. besides, the user can have two slightly different names for the same image and the same behavior happens. it could be |
$ kind load docker-image busybox busybox -v7
Image: "busybox" with ID "sha256:018c9d7b792b4be80095d957533667279843acf9a46c973067c8d1dff31ea8b4" not yet present on node "kind-control-plane", loading...
Image: "busybox" with ID "sha256:018c9d7b792b4be80095d957533667279843acf9a46c973067c8d1dff31ea8b4" not yet present on node "kind-control-plane", loading...
ERROR: failed to load image: command "docker exec --privileged -i kind-control-plane ctr --namespace=k8s.io images import -" failed with error: exit status 1
Command Output: ctr: image "docker.io/library/busybox:1.29": already exists
Stack Trace:
sigs.k8s.io/kind/pkg/errors.WithStack
/home/aojea/go/pkg/mod/sigs.k8s.io/kind@v0.11.2-0.20210826184821-f8e6aa668edd/pkg/errors/errors.go:59
sigs.k8s.io/kind/pkg/exec.(*LocalCmd).Run
/home/aojea/go/pkg/mod/sigs.k8s.io/kind@v0.11.2-0.20210826184821-f8e6aa668edd/pkg/exec/local.go:124
sigs.k8s.io/kind/pkg/cluster/internal/providers/docker.(*nodeCmd).Run
/home/aojea/go/pkg/mod/sigs.k8s.io/kind@v0.11.2-0.20210826184821-f8e6aa668edd/pkg/cluster/internal/providers/docker/node.go:146
sigs.k8s.io/kind/pkg/cluster/nodeutils.LoadImageArchive
/home/aojea/go/pkg/mod/sigs.k8s.io/kind@v0.11.2-0.20210826184821-f8e6aa668edd/pkg/cluster/nodeutils/util.go:80
sigs.k8s.io/kind/pkg/cmd/kind/load/docker-image.loadImage
/home/aojea/go/pkg/mod/sigs.k8s.io/kind@v0.11.2-0.20210826184821-f8e6aa668edd/pkg/cmd/kind/load/docker-image/docker-image.go:181
sigs.k8s.io/kind/pkg/cmd/kind/load/docker-image.runE.func1
/home/aojea/go/pkg/mod/sigs.k8s.io/kind@v0.11.2-0.20210826184821-f8e6aa668edd/pkg/cmd/kind/load/docker-image/docker-image.go:166
sigs.k8s.io/kind/pkg/errors.UntilErrorConcurrent.func1
/home/aojea/go/pkg/mod/sigs.k8s.io/kind@v0.11.2-0.20210826184821-f8e6aa668edd/pkg/errors/concurrent.go:30
runtime.goexit
/usr/local/go/src/runtime/asm_amd64.s:1581 |
|
f8e6aa668edd is a bit old, perhaps I can't reproduce because this was fixed in containerd at some point? |
k8s-ci-robot
added
size/M
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
| return result | ||
| } | ||
| seen := make(map[string]struct{}) | ||
| for _, s := range slice { |
There was a problem hiding this comment.
- the loop below is not go formatted properly (should be k := ...), I would guess your editor isn't configured to gofmt in simple mode? this is failing lints
- since we're rolling our own:
func removeDuplicates(slice []string) []string {
result := []string{}
seenKeys := make(map[string]struct{})
for _, k := range slice {
if _, seen := seenKeys[k]; !seen {
result = append(result, k)
seenKeys[k] = struct{}{}
}
}
return result
}(faster with one loop, simpler, no need to optimize for empty input, we require an argument)
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, BenTheElder The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
lgtm
|
/retest |
Fixes: #2535