image-archive: enable loading multiple image archives

[harshanarayana]

Current command implementation of the kind load image-archive expects that only one tar can be loaded at a time. Though the command handler checks to see a minimum of one file is passed as argument to the command while invocation, it doesn't enforce the maximum number of files that can be passed to the command.

However, the implementation picks the args[0] and ignores the rest without any error or warning. This can lead to a bit of confusion. It is useful to be able to load multiple image archives in one go anyway.

This PR enables the ability to load images from more than one archive at one go while honouring the original behavior.

Closes #2881

❯ docker exec -it test-control-plane bash
root@test-control-plane:/# crictl images | grep alpine
root@test-control-plane:/#
exit

❯ docker save alpine:3.12 > archive1.tar

❯ docker save alpine:3.13 > archive2.tar

❯ bin/kind load image-archive archive1.tar archive2.tar --name test

❯ docker exec -it test-control-plane bash
root@test-control-plane:/# crictl images | grep alpine
docker.io/library/alpine                   3.12                                      24c8ece58a1aa       5.87MB
docker.io/library/alpine                   3.13                                      6b5c5e00213a4       5.91MB
root@test-control-plane:/#
exit

❯ bin/kind -v=3 load image-archive archive1.tar archive2.tar --name test
DEBUG: image-archive/image-archive.go:142] Loading Docker Image from archive archive1.tar to node test-control-plane
DEBUG: image-archive/image-archive.go:142] Loading Docker Image from archive archive2.tar to node test-control-plane

[k8s-ci-robot]

Hi @harshanarayana. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: harshanarayana
Once this PR has been reviewed and has the lgtm label, please assign amwat for approval by writing /assign @amwat in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[aojea]

I don't know if we should expand the command to multiple files, let's say we have
kind load image file1 file2 file3 and file2 fails.

The command exits with error , but file1 was already loaded.

I think that is safer to check that only one argument is passed

@BenTheElder WDYT?

[harshanarayana]

The command exits with error , but file1 was already loaded.

This can be indicated clearly in the error message though. Or we can add a --ignore-error kind of an argument and turn the failed load into a warning and continue to load the rest of the archive.

Having multiple archive load can be really useful though

[BenTheElder]

Having multiple archive load can be really useful though

That's less clear actually, multiple invocations can be made at the same performance and you can instead create a single tarball with all your images at greater efficiency than any of these.

[harshanarayana]

a single tarball with all your images at greater efficiency than any of these

This is very much valid. But at the same time if you are loading up a bunch of archives collected from different places (Yes, we do this internally 😢 in our dev workflows), it is not going to be easy to aggregate them into one common tar ball without doing a few things first.

So shall I turn this PR to enforce the Nargs=1 behavior instead then or will that be considered a backward incompatible changes of sort ?

[harshanarayana]

multiple invocations can be made at the same performance

True. But with support for multiple one, you can do kind load image-archive *.tar --name test. Would this not be a bit more user friendly ?

[BenTheElder]

True. But with support for multiple one, you can do kind load image-archive *.tar --name test. Would this not be a bit more user friendly ?

maybe a tad, but it's also hiding the tradeoff on handling individual image loading and if you're collecting up multiple images you already have some script that could handle loading these in a loop.

This is very much valid. But at the same time if you are loading up a bunch of archives collected from different places (Yes, we do this internally 😢 in our dev workflows), it is not going to be easy to aggregate them into one common tar ball without doing a few things first.

ack

[aojea]

I think that first we have to validate that all files exist, and then try to load all ... basically is split this in 2 loops

[harshanarayana]

@aojea Done. PTAL.

[harshanarayana]

I think it's OK to implement this but I almost think we should throw a warning or something to nudge people in the right direction,

@aojea @BenTheElder Took care of this one too by adding PreRun hook.

[BenTheElder]

However, the implementation picks the args[0] and ignores the rest without any error or warning. This can lead to a bit of confusion.

this I agree, it should have enforced Nargs = 1

It's less obvious that we should keep adding complexity to these vs fleshing out #2038, and we should actually nudge users to use a single tarball if possible to get better performance in most cases.

[harshanarayana]

this I agree, it should have enforced Nargs = 1

@BenTheElder @aojea Should I redo this PR to enforce this then ?

[BenTheElder]

let me circle back to this later when I don't have COVID brain, I shouldn't be on here but I'm restless :+)

defer to @aojea in the meantime, I think it's OK to implement this but I almost think we should throw a warning or something to nudge people in the right direction, the performance using individual tarballs will be disappointing and for most users this is probably something they could avoid with something like docker save foo bar baz.

[aojea]

/ok-to-test
/lgtm

[harshanarayana]

@BenTheElder @aojea Just a ping to check if this is all good or anything else needs to be taken care of as part of this ?