Add support for running container from OCI archive #3537

austinvazquez · 2024-10-14T16:55:17Z

Issue

Proposal for #3536

Description

This change adds the ability to create and run containers from OCI archive without performing separate load step.

Testing

As this behavior is not provided by Docker, this change adds a nerdctl-only test for this behavior.

austinvazquez · 2024-10-14T19:26:43Z

If maintainers would prefer, I can break 79e4e24 into its own PR. The change was trivial at first before I added docs changes and integration test. LMK your thoughts.

AkihiroSuda · 2024-10-15T07:54:08Z

cmd/nerdctl/container/container_create.go

@@ -409,12 +409,17 @@ func processContainerCreateOptions(cmd *cobra.Command) (types.ContainerCreateOpt
 	if err != nil {
 		return opt, err
 	}
+	quiet, err := cmd.Flags().GetBool("quiet")


Should be a separate PR

Does Docker or Podman have this flag?

Otherwise —pull-quiet might be less confusing? idk

👍🏼 Opened #3551
Docker has --quiet across load, run, and create.

AkihiroSuda · 2024-10-15T07:55:57Z

cmd/nerdctl/container/container_run.go

@@ -361,6 +363,33 @@ func runAction(cmd *cobra.Command, args []string) error {
 		return err
 	}

+	if imageRef := args[0]; strings.HasPrefix(imageRef, "oci-archive://") {


This has to be supported in nerdctl create too (See ipfs:// code)

Thanks will take a look.

AkihiroSuda · 2024-10-15T07:56:48Z

cmd/nerdctl/container/container_run.go

+
+		// Running multi-image OCI archives is not yet supported.
+		if len(images) != 1 {
+			return errors.New("multiple images in OCI archive found")


What about just defaulting to the first one? (With a warning)

AkihiroSuda · 2024-10-15T07:56:59Z

cmd/nerdctl/container/container_run_test.go

+func TestRunFromOCIArchive(t *testing.T) {
+	testutil.RequiresBuild(t)
+	testutil.RegisterBuildCacheCleanup(t)
+	testutil.DockerIncompatible(t)


Needs a comment

AkihiroSuda · 2024-10-15T07:57:47Z

docs/command-reference.md

oci-archive:// needs docs too

Addressed in d669f3d. Added documentation for both create and run.

austinvazquez · 2024-10-15T20:03:25Z

Going to let this CI run finish before rebasing for #3551

AkihiroSuda · 2024-10-17T07:28:27Z

pkg/cmd/container/create.go

@@ -123,6 +124,37 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
 	}
 	opts = append(opts, platformOpts...)

+	if imageRef := args[0]; strings.HasPrefix(imageRef, "oci-archive://") {


Protocol prefixes should be parsed in the same place as ipfs://

nerdctl/pkg/referenceutil/referenceutil.go

Line 91 in f240e58

func Parse(rawRef string) (*ImageReference, error) {

Thanks for the reference. I missed this when looking at the ipfs code.

apostasie · 2024-10-17T18:23:21Z

pkg/referenceutil/referenceutil_test.go

+			Path:         "/tmp/build/saved-image.tar",
+			Error:        "",
+			String:       "/tmp/build/saved-image.tar",
+			Suggested:    "oci-archive-/tmp/-abcde",


You likely need to do something for the suggested name of oci-archive://.

This suggested name is not valid for a container name (it is used when you do not specify a name for your container).

Maybe something in the line of:

func (ir *ImageReference) SuggestContainerName(suffix string) string { name := "untitled" if ir.Protocol != "" && ir.Domain == "" { if ir.Protocol == OCIArchive { name = string(ir.Protocol) + "-" + path.Base(ir.Path)[:shortIDLength] }else{ name = string(ir.Protocol) + "-" + ir.String()[:shortIDLength] } } else if ir.Path != "" { name = path.Base(ir.Path) } return name + "-" + suffix[:5] }

... although that is likely not sufficient, as filesystem might allow characters that are not safe to use as image identifiers.

Maybe we keep it simple and only suggest ‘oci-archive-container-id’ so that we do not have to deal with local path sanitization.

Thanks @apostasie. Let me take another look here. We load the image from the archive before and reset the image ref to the name in the image manifest before calling parse again on that reference. I shouldn't have just made this test work though. Maybe it would be better to have another type for oci-archive image references which doesn't support suggested container name as it doesn't make sense.

I’ll have a look around as well on how this is used.
thanks @austinvazquez !

Should be addressed with 4034998. @apostasie, I took another look and changed how I was looking at the problem. When parsing an raw image reference which is a path for an OCI archive, we should fail and let the caller (pkg/create) know the reference is a path to an image archive and needs to be loaded before (re)parsing. This should resolve the quirkiness of having a image reference object which was not really a valid image reference.

Yeah, reading through - it is a bit weird indeed, because, yep, it is not really an image ref, it is a filesystem location... - I think it is ok as you put it now.

ktock · 2024-10-18T05:27:16Z

pkg/cmd/container/create.go

+		image := images[0].Name
+		// Multi-image archive provided, default to first image found.
+		if len(images) != 1 {
+			log.L.Warnf("multi-image OCI archive provided, defaulting to image %s...", image)


Suggested change

log.L.Warnf("multi-image OCI archive provided, defaulting to image %s...", image)

log.L.Warnf("multiple images are found for the platform, defaulting to image %s...", image)

Applied in 932bfaf

ktock · 2024-10-18T05:28:20Z

pkg/imgutil/load/load.go

+	if options.Input != "" {
+		f, err := os.Open(options.Input)
+		if err != nil {
+			return []images.Image{}, err


why not return nil, err?

I feel like I picked this up early in my Go learning (maybe a project I worked did this) but yeah looks like nil is recommended to avoid memory alloc. Will update.

Addressed in d8ab512

Signed-off-by: Austin Vazquez <macedonv@amazon.com>

apostasie · 2024-10-21T22:17:52Z

cmd/nerdctl/container/container_create_linux_test.go

+	containerName := testutil.Identifier(t)
+
+	teardown := func() {
+		base.Cmd("rm", containerName).Run()


-f for both rm and rmi would be better in teardown.

apostasie · 2024-10-21T22:19:03Z

cmd/nerdctl/container/container_run_test.go

+	imageName := testutil.Identifier(t)
+
+	teardown := func() {
+		base.Cmd("rmi", imageName).Run()


apostasie · 2024-10-21T22:28:56Z

pkg/referenceutil/referenceutil_test.go

@@ -273,6 +273,10 @@ func TestReferenceUtil(t *testing.T) {
 			Tag:          "latest",
 			ExplicitTag:  "",
 		},
+		"oci-archive:///tmp/build/saved-image.tar": {
+			Protocol: "oci-archive",


Protocol is not necessary here, since the test stops as it errors (and since your latest version does not set the protocol)

apostasie · 2024-10-21T22:33:46Z

@austinvazquez thanks! - I left a few nits comments.

Signed-off-by: Austin Vazquez <macedonv@amazon.com>

AkihiroSuda

Thanks

austinvazquez force-pushed the feat-run-oci-archive branch from bc97ccd to b3e38d1 Compare October 14, 2024 19:01

austinvazquez marked this pull request as ready for review October 14, 2024 19:26

AkihiroSuda reviewed Oct 15, 2024

View reviewed changes

austinvazquez mentioned this pull request Oct 15, 2024

Add image load and container run quiet mode #3551

Merged

austinvazquez force-pushed the feat-run-oci-archive branch from b3e38d1 to 2159e7f Compare October 15, 2024 19:55

austinvazquez force-pushed the feat-run-oci-archive branch 2 times, most recently from d4a5e21 to d669f3d Compare October 15, 2024 20:38

chews93319 approved these changes Oct 16, 2024

View reviewed changes

AkihiroSuda reviewed Oct 17, 2024

View reviewed changes

austinvazquez force-pushed the feat-run-oci-archive branch from d669f3d to 006e403 Compare October 17, 2024 15:38

AkihiroSuda requested a review from ktock October 17, 2024 15:39

AkihiroSuda added this to the v2.0.0 milestone Oct 17, 2024

austinvazquez force-pushed the feat-run-oci-archive branch from 006e403 to 971d297 Compare October 17, 2024 16:06

apostasie reviewed Oct 17, 2024

View reviewed changes

ktock reviewed Oct 18, 2024

View reviewed changes

austinvazquez added 2 commits October 21, 2024 16:49

Move image load func to imgutil package

ec12b58

Signed-off-by: Austin Vazquez <macedonv@amazon.com>

Refactor load from archive functionality

d8ab512

Signed-off-by: Austin Vazquez <macedonv@amazon.com>

austinvazquez force-pushed the feat-run-oci-archive branch 2 times, most recently from 932bfaf to 4034998 Compare October 21, 2024 17:30

austinvazquez requested review from ktock and apostasie October 21, 2024 18:37

apostasie reviewed Oct 21, 2024

View reviewed changes

apostasie approved these changes Oct 21, 2024

View reviewed changes

Add container run from oci-archive

a4b2959

Signed-off-by: Austin Vazquez <macedonv@amazon.com>

austinvazquez force-pushed the feat-run-oci-archive branch from 4034998 to a4b2959 Compare October 22, 2024 04:31

AkihiroSuda approved these changes Oct 22, 2024

View reviewed changes

AkihiroSuda merged commit 0612a1d into containerd:main Oct 22, 2024
26 checks passed

austinvazquez deleted the feat-run-oci-archive branch October 22, 2024 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for running container from OCI archive #3537

Add support for running container from OCI archive #3537

austinvazquez commented Oct 14, 2024 •

edited

Loading

austinvazquez commented Oct 14, 2024

AkihiroSuda Oct 15, 2024

AkihiroSuda Oct 15, 2024

austinvazquez Oct 15, 2024

AkihiroSuda Oct 15, 2024 •

edited

Loading

austinvazquez Oct 15, 2024

AkihiroSuda Oct 15, 2024

AkihiroSuda Oct 15, 2024

AkihiroSuda Oct 15, 2024 •

edited

Loading

austinvazquez Oct 15, 2024

austinvazquez commented Oct 15, 2024

AkihiroSuda Oct 17, 2024

austinvazquez Oct 17, 2024

apostasie Oct 17, 2024 •

edited

Loading

apostasie Oct 17, 2024

austinvazquez Oct 18, 2024

apostasie Oct 18, 2024

austinvazquez Oct 21, 2024

apostasie Oct 21, 2024

ktock Oct 18, 2024

austinvazquez Oct 21, 2024

ktock Oct 18, 2024

austinvazquez Oct 18, 2024 •

edited

Loading

austinvazquez Oct 21, 2024

apostasie Oct 21, 2024

apostasie Oct 21, 2024

apostasie Oct 21, 2024

apostasie commented Oct 21, 2024

AkihiroSuda left a comment

	log.L.Warnf("multi-image OCI archive provided, defaulting to image %s...", image)
	log.L.Warnf("multiple images are found for the platform, defaulting to image %s...", image)

Add support for running container from OCI archive #3537

Add support for running container from OCI archive #3537

Conversation

austinvazquez commented Oct 14, 2024 • edited Loading

Issue

Description

Testing

austinvazquez commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvazquez commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apostasie Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvazquez Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apostasie commented Oct 21, 2024

AkihiroSuda left a comment

Choose a reason for hiding this comment

austinvazquez commented Oct 14, 2024 •

edited

Loading

AkihiroSuda Oct 15, 2024 •

edited

Loading

AkihiroSuda Oct 15, 2024 •

edited

Loading

apostasie Oct 17, 2024 •

edited

Loading

austinvazquez Oct 18, 2024 •

edited

Loading