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.

Sign up for GitHub

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

Jump to bottom

image-archive: enable loading multiple image archives #2891

Merged
merged 1 commit into from
Jun 27, 2024
Merged

image-archive: enable loading multiple image archives #2891

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 25 additions & 10 deletions pkg/cmd/kind/load/image-archive/image-archive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ func NewCommand(logger log.Logger, streams cmd.IOStreams) *cobra.Command {
}
return nil
},
PreRun: func(cmd *cobra.Command, args []string) {
if len(args) > 1 {
logger.Warn("It is suggested that you save multiple images into a common archive and load that instead of loading multiple archives for better performance")
}
},
Use: "image-archive <IMAGE.tar>",
Short: "Loads docker image from archive into nodes",
Long: "Loads docker image from archive into all or specified nodes by name",
Expand Down Expand Up @@ -78,19 +83,28 @@ func runE(logger log.Logger, flags *flagpole, args []string) error {
runtime.GetDefault(logger),
)

// Check if file exists
imageTarPath := args[0]
if _, err := os.Stat(imageTarPath); err != nil {
return err
for _, imageTarPath := range args {
if _, err := os.Stat(imageTarPath); err != nil {
return err
Comment on lines +87 to +88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aojea Done. PTAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

for _, imageTarPath := range args {
if err := loadArchiveToNodes(logger, provider, flags.Name, flags.Nodes, imageTarPath); err != nil {
return err
}
}
return nil
}

func loadArchiveToNodes(logger log.Logger, provider *cluster.Provider, clusterName string, nodeNames []string, imageArchivePath string) error {
// Check if the cluster nodes exist
nodeList, err := provider.ListInternalNodes(flags.Name)
nodeList, err := provider.ListInternalNodes(clusterName)
if err != nil {
return err
}
if len(nodeList) == 0 {
return fmt.Errorf("no nodes found for cluster %q", flags.Name)
return fmt.Errorf("no nodes found for cluster %q", clusterName)
}

// map cluster nodes by their name
Expand All @@ -104,9 +118,9 @@ func runE(logger log.Logger, flags *flagpole, args []string) error {
// pick only the user selected nodes and ensure they exist
// the default is all nodes unless flags.Nodes is set
selectedNodes := nodeList
if len(flags.Nodes) > 0 {
if len(nodeNames) > 0 {
selectedNodes = []nodes.Node{}
for _, name := range flags.Nodes {
for _, name := range nodeNames {
node, ok := nodesByName[name]
if !ok {
return fmt.Errorf("unknown node: %s", name)
Expand All @@ -120,18 +134,19 @@ func runE(logger log.Logger, flags *flagpole, args []string) error {
for _, selectedNode := range selectedNodes {
selectedNode := selectedNode // capture loop variable
fns = append(fns, func() error {
return loadImage(imageTarPath, selectedNode)
return loadImage(logger, imageArchivePath, selectedNode)
})
}
return errors.UntilErrorConcurrent(fns)
}

// loads an image tarball onto a node
func loadImage(imageTarName string, node nodes.Node) error {
func loadImage(logger log.Logger, imageTarName string, node nodes.Node) error {
f, err := os.Open(imageTarName)
if err != nil {
return errors.Wrap(err, "failed to open image")
}
defer f.Close()
logger.V(2).Infof("Loading Docker Image from archive %s to node %s", imageTarName, node.String())
return nodeutils.LoadImageArchive(node, f)
}
Loading