feat: support push with soci #2467

ningziwen · 2023-09-02T10:40:21Z

Issue

High level approach

The current approach integrates both soci create and soci push to nerdctl push when soci is specified in nerdctl push. The change is based on the assumption that we want to integrate whole soci workflow to nerdctl seamlessly without letting users run any soci commands by themselves.

One alternative is to integrate soci create to nerdctl build and soci push to nerdctl push. nerdctl build makes sense to be independent with nerdctl push because the image can ran or exported locally without pushing later, but soci index is for lazy pull, which doesn't make sense without pushing to a registry later. The soci getting-started guide also puts soci create after pushing image to registry. So didn't choose this approach.

SOCI flags

When calling soci commands in nerdctl push. There are many flags in soci create and soci push to be set properly. There are several possible ways of setting them:

If Nerdctl has the same flag and it is reasonable to pass through, pass throught the flags from Nerdctl input to soci.
If the flag is reasonable to expose to Nerdctl users but not available in Nerdctl, add new flags to Nerdctl prefixed with soci. (Similar to the style in Cosign)
Hardcode to specific values in soci commands.
Keep the flags as default in soci commands if we don't see obvious benefit of exposing them to Nerdctl users. It is a two-way-door to expose them later.

SOCI flag	SOCI command	Recommended Value
--address	global	Pass through from Nerdctl global flag --address
--namespace	global	Pass through from Nerdctl global flag --namespace
--timeout	global	Keep default as no timeout
--debug	global	Keep default as false, which is consistent with the pattern of other dependencies such as Cosign
--content-store	global	Keep default. The content store is anyways managed by SOCI. Discussion
--all-platforms	create and push	Pass through from Nerdctl push flag --all-platforms
--platform	create and push	Pass through from Nerdctl push flag --platform
--span-size	create	Add new Nerdctl push flag --soci-span-size to expose it
--min-layer-size	create	Add new Nerdctl push flag --soci-min-layer-size to expose it
--skip-verify	push	Pass through from Nerdctl push flag --insecure-registry
--plain-http	push	Pass through from Nerdctl push flag --insecure-registry (from this discussion, both are needed)
--user	push	Keep default as empty. Soci could and should share nerdctl's credentials
--refresh	push	Keep default as empty. Soci could and should share nerdctl's credentials
--hosts-dir	push	Unsupported by SOCI now. awslabs/soci-snapshotter#839)
--tlscacert	push	Keep default. The config has too low level to be exposed in Nerdctl
--tlscert	push	Keep default. The config has too low level to be exposed in Nerdctl
--tlskey	push	Keep default. The config has too low level to be exposed in Nerdctl
--http-dump	push	Keep default. The config has too low level to be exposed in Nerdctl
--http-trace	push	Keep default. The config has too low level to be exposed in Nerdctl
--label	push	Keep default. SOCI index should not need special labels and any labels in Nerdctl should not apply to index
--snapshotter	push	Keep default. The user input snapshotter in Nerdctl is soci. SOCI index should use the default one instead of using SOCI snapshotter again
--existing-index	push	Keep default as "warn". Can expose later if needed.
--max-concurrent-uploads	push	Keep default as "10". Can expose later if needed.
--quiet	push	Keep default as false. No matter whether `quiet` is true in soci, Nerdctl quiet should already suppress anything

Kern-- · 2023-09-05T20:10:20Z

pkg/snapshotterutil/sociutil.go

+
+// CreateSoci creates a SOCI index(`rawRef`)
+func CreateSoci(rawRef string) error {
+	sociExecutable, err := exec.LookPath("soci")


Is shelling out to the SOCI CLI a design goal or is it just because the CLI has non-trivial logic?

I would think using SOCI as a library here would be better long term.

If I understand correctly, the pros of using SOCI library include:

Does not require SOCI CLI to be installed in Nerdctl env

Cleaner code to call library instead of calling commands

However, there is already a common practice of calling commands of dependencies in Nerdctl, such as Buildkit, Cosign, Notation, and soci-snapshotter-grpc is anyways needed to be pre-configured as part of SOCI provisioning. So the two pros don't seem significant.

Here are the reasons of leaning to using SOCI CLI instead of library for now.

SOCI CLI has well-defined documentation and the backward compatibility is respected by default. The library code doesn't have documentation and it is not clear about how much backward compatibility it will have. It may also be difficult for SOCI maintainers to promise and define that.

SOCI CLI layer has some business logic which can call multiple methods of library code, which is tedious to duplicate in Nerdctl.

For both above, some refactoring in SOCI similar to what Nerdctl is doing could help (with that, the library code can include all the business logic and can be clearly defined and promised backward compatibility) but may not deserve.

The SOCI CLI does not guarantee backwards compatibility at this point https://github.com/awslabs/soci-snapshotter/blob/main/RELEASES.md#api-stability, but you're probably right that the CLI is more likely to be backwards compatible than the library.

I agree that the CLI is the right thing to do for this PR. I was curious what the longer term vision was, but it seems that there's precedence for relying on the CLI indefinitely.

Kern-- · 2023-09-05T20:14:39Z

pkg/snapshotterutil/sociutil.go

+		return err
+	}
+	ref := rawRef
+	if !strings.Contains(ref, "@") {


Is there a use-case where this is true? The current call site always contains a specific digest reference, right?

You are right. Fixed.

ningziwen · 2023-09-05T21:20:30Z

@AkihiroSuda @djdongjin May I get basic concensus of the high level approach (See PR description) before polishing the implementation and adding tests?

AkihiroSuda · 2023-09-05T21:23:01Z

@AkihiroSuda @djdongjin May I get basic concensus of the high level approach (See PR description) before polishing the implementation and adding tests?

👍 on exec-ing the soci binary.
We have been doing the same for cosign, notation, IPFS, buildg, etc., to avoid inflating go.mod dependencies.

Kern-- · 2023-09-14T23:49:12Z

pkg/snapshotterutil/sociutil.go

+		return err
+	}
+
+	sociCmd := exec.Command(sociExecutable, []string{"create"}...)


You should also pass --namespace, --address (containerd address), and maybe --timeout.

ginglis13 · 2023-09-15T22:28:44Z

pkg/snapshotterutil/sociutil.go

+		sociCmd.Args = append(sociCmd.Args, "--namespace", gOpts.Namespace)
+	}
+	if sOpts.SpanSize != -1 {
+		sociCmd.Args = append(sociCmd.Args, "--span-size", strconv.FormatInt(sOpts.SpanSize, 10))


Looks like values are in bytes in soci cli. See https://github.com/awslabs/soci-snapshotter/blob/f556b45e8e241ff243776e15352a6b7bd38f2161/soci/soci_index.go#L57-L58

defaultSpanSize = int64(1 << 22) // 4MiB defaultMinLayerSize = 10 << 20 // 10MiB

Maybe room for improved UX to specify value with mib, m, kib, k suffix and do the conversion?

Yeah. Maybe this can contribute to soci CLI later.

ginglis13 · 2023-09-15T22:28:25Z

cmd/nerdctl/image_push_linux_test.go

@@ -174,3 +176,69 @@ func TestPushNonDistributableArtifacts(t *testing.T) {
 	}
 	assert.Equal(t, resp.StatusCode, http.StatusOK, "non-distributable blob should be available")
 }
+
+func TestPushSoci(t *testing.T) {


test LGTM. maybe could deduplicate some of https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/container_run_soci_linux_test.go

ginglis13 · 2023-09-15T22:38:06Z

pkg/snapshotterutil/sociutil.go

+		logrus.Warn("soci: " + err.Error())
+	}
+	if err := sociCmd.Start(); err != nil {
+		// only return err if it's critical (soci start failed.)


nit:

Suggested change

// only return err if it's critical (soci start failed.)

// only return err if it's critical (soci command failed to start.)

AkihiroSuda · 2023-09-16T06:04:18Z

pkg/testutil/testutil_linux.go

@@ -60,6 +60,7 @@ var (
 const (
 	FedoraESGZImage = "ghcr.io/stargz-containers/fedora:30-esgz"            // eStargz
 	FfmpegSociImage = "public.ecr.aws/soci-workshop-examples/ffmpeg:latest" // SOCI
+	UbuntuImage     = "public.ecr.aws/docker/library/ubuntu:latest"         // Large enough for testing soci index creation


Please pin the version

AkihiroSuda

Thanks, LGTM if CI is green

AkihiroSuda · 2023-09-18T10:28:18Z

Failing

=== RUN   TestPushSoci
    testregistry_linux.go:49: hostIP="10.4.0.1", listenIP="0.0.0.0", listenPort=5000
    image_push_linux_test.go:188: testImageRef="10.4.0.1:5000/nerdctl-testpushsoci:23.10"
    image_push_linux_test.go:191: assertion failed: res.ExitCode is not exitCode: time="2023-09-18T10:26:25Z" level=fatal msg="invalid argument \"1<<21\" for \"--soci-span-size\" flag: strconv.ParseInt: parsing \"1<<21\": invalid syntax"
        
--- FAIL: TestPushSoci (1.50s)
FAIL cmd/nerdctl.TestPushSoci (re-run 2) (1.50s)

Signed-off-by: Ziwen Ning <ningziwe@amazon.com>

ningziwen force-pushed the build branch from bc65a40 to 5101fb9 Compare September 2, 2023 11:21

ningziwen changed the title ~~feat: support build and push with soci~~ feat: support push with soci Sep 2, 2023

Kern-- reviewed Sep 5, 2023

View reviewed changes

ningziwen force-pushed the build branch 13 times, most recently from b1aa161 to da8220d Compare September 11, 2023 06:02

ningziwen force-pushed the build branch 2 times, most recently from cd84ab1 to ea46b02 Compare September 14, 2023 22:27

Kern-- reviewed Sep 14, 2023

View reviewed changes

ningziwen force-pushed the build branch 6 times, most recently from d4fd880 to fc75f2a Compare September 15, 2023 21:52

ginglis13 reviewed Sep 15, 2023

View reviewed changes

ningziwen force-pushed the build branch 2 times, most recently from 012f353 to 374a376 Compare September 16, 2023 02:54

ningziwen force-pushed the build branch 3 times, most recently from da21d43 to 6420080 Compare September 16, 2023 05:45

AkihiroSuda reviewed Sep 16, 2023

View reviewed changes

ningziwen force-pushed the build branch 7 times, most recently from bf32896 to 6dd044b Compare September 17, 2023 17:48

ningziwen marked this pull request as ready for review September 17, 2023 17:59

AkihiroSuda added this to the v1.5.2 milestone Sep 17, 2023

ningziwen force-pushed the build branch from 6dd044b to f8eadc7 Compare September 18, 2023 09:57

AkihiroSuda approved these changes Sep 18, 2023

View reviewed changes

ningziwen force-pushed the build branch 2 times, most recently from 928e0b6 to 412a0b7 Compare September 18, 2023 11:40

feat: support push with soci

4c82cde

Signed-off-by: Ziwen Ning <ningziwe@amazon.com>

ningziwen force-pushed the build branch from 412a0b7 to 4c82cde Compare September 18, 2023 11:41

ginglis13 approved these changes Sep 18, 2023

View reviewed changes

AkihiroSuda approved these changes Sep 18, 2023

View reviewed changes

AkihiroSuda modified the milestones: v1.7.0 (tentative), v1.6.0 Sep 18, 2023

AkihiroSuda merged commit c17026f into containerd:main Sep 18, 2023
21 checks passed

ningziwen deleted the build branch September 18, 2023 18:01

ollypom mentioned this pull request Nov 16, 2023

Create SOCI Indexes when pushing multiple platforms #2644

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support push with soci #2467

feat: support push with soci #2467

ningziwen commented Sep 2, 2023 •

edited

Loading

Kern-- Sep 5, 2023

ningziwen Sep 5, 2023

Kern-- Sep 5, 2023

Kern-- Sep 5, 2023

ningziwen Sep 7, 2023

ningziwen commented Sep 5, 2023

AkihiroSuda commented Sep 5, 2023

Kern-- Sep 14, 2023

ginglis13 Sep 15, 2023

ningziwen Sep 16, 2023

ginglis13 Sep 15, 2023

ginglis13 Sep 15, 2023

AkihiroSuda Sep 16, 2023

AkihiroSuda left a comment

AkihiroSuda commented Sep 18, 2023

	// only return err if it's critical (soci start failed.)
	// only return err if it's critical (soci command failed to start.)

feat: support push with soci #2467

feat: support push with soci #2467

Conversation

ningziwen commented Sep 2, 2023 • edited Loading

Issue

High level approach

SOCI flags

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

ningziwen commented Sep 5, 2023

AkihiroSuda commented Sep 5, 2023

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

AkihiroSuda left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Sep 18, 2023

ningziwen commented Sep 2, 2023 •

edited

Loading