DO NOT MERGE: Run tests with Zstd compression default

[mtrmac]

Inspired by #20633

Does this PR introduce a user-facing change?

do not merge this

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac

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:

• OWNERS [mtrmac]

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

[mtrmac]

Legitimate problems:

• podman … push --format=v2s2 … fails with Error: compression using zstd required together with format application/vnd.docker.distribution.manifest.v2+json, which does not support it. Arguably an explicit option on the CLI should override config-file compression defaults, the intent here is clear.
• podman … push … docker-archive:… fails with compression using zstd required but the destination only supports MIME types [application/vnd.docker.distribution.manifest.v2+json], none of which support it. Same as above, but there isn’t even an explicit CLI option.

[mtrmac]

@giuseppe FYI. I’ll continue investigating this, just noting the Podman-side UI concerns above for now.

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[packit-as-a-service]

Cockpit tests failed for commit 9ce62ea83c221979aff1bd91afe0c002cc73f270. @martinpitt, @jelly, @mvollmer please check.

[packit-as-a-service]

Cockpit tests failed for commit 1a9566f043cf084167e5ef3e8e90a1690dc7bead. @martinpitt, @jelly, @mvollmer please check.

[giuseppe]

CI is green!

[rhatdan]

Does this require a change in podman or contianers/image?

[mtrmac]

This is specific to containers.conf. So not c/image; Podman or maybe c/common/libimage.

[mtrmac]

🎉 green!

#21793 now contains the test updates we do want to make; the other changes from this PR should not be merged, and instead we should change Podman’s behavior.

[rhatdan]

@mtrmac needs a rebase, since latest containers/image is now in podman.

[rhatdan]

Any suggestion on how to fix
UNWANTED: Don't assume that (podman manifest push) doesn't modify images

We want to fix this test and then merge in common, then we can say zstd is supported.

Is it time to test zstd:chunked and see if it passes this test?

[mtrmac]

Any suggestion on how to fix UNWANTED: Don't assume that (podman manifest push) doesn't modify images

I think replacing the checks for specific SHA256 values with checks that all those architectures are present would keep the spirit of that test.

---

Is it time to test zstd:chunked and see if it passes this test?

*shrug* the checklist still contains unresolved issues I’m working on. Triggering a test run now, to see what else might be outstanding, can’t hurt.

[rhatdan]

@giuseppe can you update the test above so we can remove it from this list.

[giuseppe]

@mtrmac would something like the following work for you?

diff --git a/test/e2e/manifest_test.go b/test/e2e/manifest_test.go
index 3a05c4f5c..01249e726 100644
--- a/test/e2e/manifest_test.go
+++ b/test/e2e/manifest_test.go
@@ -2,6 +2,8 @@ package integration
 
 import (
 	"encoding/json"
+	"fmt"
+	"io/ioutil"
 	"os"
 	"path/filepath"
 	"strings"
@@ -16,6 +18,50 @@ import (
 	imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
+// validateManifestHasAllArchs checks that the specified manifest has all
+// the archs in `imageListInstance`
+func validateManifestHasAllArchs(path string) error {
+	data, err := ioutil.ReadFile(path)
+	if err != nil {
+		return err
+	}
+
+	var result map[string]interface{}
+	if err := json.Unmarshal(data, &result); err != nil {
+		return err
+	}
+
+	manifests, ok := result["manifests"].([]interface{})
+	if !ok {
+		return fmt.Errorf("invalid manifest file")
+	}
+	archs := map[string]bool{
+		"amd64":   false,
+		"arm64":   false,
+		"ppc64le": false,
+		"s390x":   false,
+	}
+	for _, manifestItem := range manifests {
+		manifest := manifestItem.(map[string]interface{})
+		platform, ok := manifest["platform"].(map[string]interface{})
+		if !ok {
+			continue
+		}
+
+		if arch, ok := platform["architecture"].(string); ok {
+			if _, exists := archs[arch]; exists {
+				archs[arch] = true
+			}
+		}
+	}
+	for arch, present := range archs {
+		if !present {
+			return fmt.Errorf("architecture %q is missing in the manifest", arch)
+		}
+	}
+	return nil
+}
+
 // Internal function to verify instance compression
 func verifyInstanceCompression(descriptor []imgspecv1.Descriptor, compression string, arch string) bool {
 	for _, instance := range descriptor {
@@ -415,23 +461,12 @@ add_compression = ["zstd"]`), 0o644)
 		defer func() {
 			os.RemoveAll(dest)
 		}()
		session = podmanTest.Podman([]string{"manifest", "push", "-q", "--all", "foo", "dir:" + dest})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
 
-		files, err := filepath.Glob(dest + string(os.PathSeparator) + "*")
-		Expect(err).ShouldNot(HaveOccurred())
-		check := SystemExec("sha256sum", files)
-		check.WaitWithDefaultTimeout()
-		Expect(check).Should(ExitCleanly())
-		prefix := "sha256:"
-		Expect(check.OutputToString()).To(
-			And(
-				ContainSubstring(strings.TrimPrefix(imageListAMD64InstanceDigest, prefix)),
-				ContainSubstring(strings.TrimPrefix(imageListPPC64LEInstanceDigest, prefix)),
-				ContainSubstring(strings.TrimPrefix(imageListS390XInstanceDigest, prefix)),
-				ContainSubstring(strings.TrimPrefix(imageListARM64InstanceDigest, prefix)),
-			))
+		err = validateManifestHasAllArchs(filepath.Join(dest, "manifest.json"))
+		Expect(err).ToNot(HaveOccurred())
 	})
 
 	It("push", func() {
@@ -451,20 +486,9 @@ add_compression = ["zstd"]`), 0o644)
 		session = podmanTest.Podman([]string{"push", "-q", "foo", "dir:" + dest})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
-		files, err := filepath.Glob(dest + string(os.PathSeparator) + "*")
-		Expect(err).ToNot(HaveOccurred())
-		check := SystemExec("sha256sum", files)
-		check.WaitWithDefaultTimeout()
-		Expect(check).Should(ExitCleanly())
 
-		prefix := "sha256:"
-		Expect(check.OutputToString()).To(
-			And(
-				ContainSubstring(strings.TrimPrefix(imageListAMD64InstanceDigest, prefix)),
-				ContainSubstring(strings.TrimPrefix(imageListPPC64LEInstanceDigest, prefix)),
-				ContainSubstring(strings.TrimPrefix(imageListS390XInstanceDigest, prefix)),
-				ContainSubstring(strings.TrimPrefix(imageListARM64InstanceDigest, prefix)),
-			))
+		err = validateManifestHasAllArchs(filepath.Join(dest, "manifest.json"))
+		Expect(err).ToNot(HaveOccurred())
 	})
 
 	It("push with compression-format and compression-level", func() {

[giuseppe]

opened a PR: #21878

[rhatdan]

Should be able to remove first and last PRs from this main PR. Which means zstd passes, now lets test zstd:chunked and start working through those issues.

[mtrmac]

Yes, this PR is now a pure setting change to test behavior with Zstd.

I have separately filed #21903 for testing zstd:chunked .

[rhatdan]

Nice work @mtrmac and @giuseppe

[mtrmac]

For the record, 7879d59 is all green, with no Podman changes (other than the compression algorithm).

[rhatdan]

Yup now we need to ship podman 5.0 and change rawhide to zstd:chunked by default.