cmd-build: Conditionally change the packing structure of container-image #3445

RishabhSaini · 2023-05-02T15:45:29Z

Use the prior-build flag to conditionally change packing structure
A part of rpm-ostree:pull/4271

src/cmd-build

cgwalters · 2023-05-08T14:44:12Z

It sounds like we're saying we should have the OCI manifest and config in the cosa s3 storage.

I'd say this should appear as e.g. container-manifest.json and container-config.json or so? But we could have a single toplevel ocimeta.json too with those as child properties (don't have a strong opinion).

And that's a change that we can just make now in coreos-assembler to start writing that data. Then in later changes we start reading it.

jlebon · 2023-05-08T16:57:09Z

I'd say this should appear as e.g. container-manifest.json and container-config.json or so? But we could have a single toplevel ocimeta.json too with those as child properties (don't have a strong opinion).

Slightly leaning towards a single file to keep the number of "untracked" metadata files down, but no strong opinion either.

And that's a change that we can just make now in coreos-assembler to start writing that data. Then in later changes we start reading it.

SGTM!

cgwalters · 2023-05-15T15:10:37Z

We started on this together

diff --git a/pkg/builds/cosa_v1.go b/pkg/builds/cosa_v1.go
index 595b9b8e7..ba78c2536 100644
--- a/pkg/builds/cosa_v1.go
+++ b/pkg/builds/cosa_v1.go
@@ -1,7 +1,7 @@
 package builds
 
 // generated by 'make schema'
-// source hash: 2cd4fe2a0c72e389ee6cb2b906653cdfd33f87b87d7e2e261fce4615a8c0272a
+// source hash: d29ccb6c6d8efae001398a074be18d125ab5e72312924af7494c9bdb3eb49dbf
 
 type AdvisoryDiff []AdvisoryDiffItems
 
@@ -106,6 +106,7 @@ type BuildArtifacts struct {
 	Metal                         *Artifact `json:"metal,omitempty"`
 	Metal4KNative                 *Artifact `json:"metal4k,omitempty"`
 	Nutanix                       *Artifact `json:"nutanix,omitempty"`
+	OciManifest                   *Artifact `json:"oci-manifest,omitempty"`
 	OpenStack                     *Artifact `json:"openstack,omitempty"`
 	Ostree                        Artifact  `json:"ostree"`
 	PowerVirtualServer            *Artifact `json:"powervs,omitempty"`
diff --git a/pkg/builds/schema_doc.go b/pkg/builds/schema_doc.go
index 2716cf339..4cef41c02 100644
--- a/pkg/builds/schema_doc.go
+++ b/pkg/builds/schema_doc.go
@@ -1,5 +1,5 @@
 // Generated by ./generate-schema.sh
-// Source hash: 2cd4fe2a0c72e389ee6cb2b906653cdfd33f87b87d7e2e261fce4615a8c0272a
+// Source hash: d29ccb6c6d8efae001398a074be18d125ab5e72312924af7494c9bdb3eb49dbf
 // DO NOT EDIT
 
 package builds
@@ -479,7 +479,8 @@ var generatedSchemaJSON = `{
         "vmware",
         "vultr",
         "qemu-secex",
-        "ignition-gpg-key"
+        "ignition-gpg-key",
+        "oci-manifest"
       ],
       "properties": {
         "ostree": {
@@ -488,6 +489,12 @@ var generatedSchemaJSON = `{
           "title": "OSTree",
           "$ref": "#/definitions/artifact"
         },
+        "oci-manifest": {
+          "$id": "#/properties/images/properties/oci-manifest",
+          "type": "object",
+          "title": "OCI manifest",
+          "$ref": "#/definitions/artifact"
+        },
         "dasd": {
           "$id": "#/properties/images/properties/dasd",
           "type": "object",
diff --git a/src/cmd-build b/src/cmd-build
index fd375997c..5ee23184f 100755
--- a/src/cmd-build
+++ b/src/cmd-build
@@ -464,6 +464,11 @@ else
         oci-archive:"${ostree_tarfile_path}".tmp:latest
     /usr/lib/coreos-assembler/finalize-artifact "${ostree_tarfile_path}"{.tmp,}
     ostree_tarfile_sha256=$(sha256sum "${ostree_tarfile_path}" | awk '{print$1}')
+    ostree_oci_manifest_path="${name}-${buildid}-ostree.${basearch}-manifest.json"
+    runv skopeo inspect --raw oci-archive:"${ostree_tarfile_path}" > tmp/manifest.json
+    /usr/lib/coreos-assembler/finalize-artifact tmp/manifest.json "${ostree_oci_manifest_path}"
+    ostree_oci_manifest_sha256=$(sha256sum "${ostree_oci_manifest_path}" | awk '{print$1}')
+    ostree_oci_manifest_size=$(stat --format=%s "${ostree_oci_manifest_path}")
 fi
 
 # The base metadata, plus locations for code sources.
@@ -507,6 +512,11 @@ cat > tmp/images.json <<EOF
         "sha256": "${ostree_tarfile_sha256}",
         "size": ${ostree_tarfile_size},
         "skip-compression": true
+    },
+    "oci-manifest" {
+        "path": "${ostree_oci_manifest_path}",
+        "sha256": "${ostree_oci_manifest_sha256}"
+        "size": "${ostree_oci_manifest_size}"
     }
   }
 }
diff --git a/src/v1.json b/src/v1.json
index ebae6ec3c..d0299edfe 100644
--- a/src/v1.json
+++ b/src/v1.json
@@ -473,7 +473,8 @@
         "vmware",
         "vultr",
         "qemu-secex",
-        "ignition-gpg-key"
+        "ignition-gpg-key",
+        "oci-manifest"
       ],
       "properties": {
         "ostree": {
@@ -482,6 +483,12 @@
           "title": "OSTree",
           "$ref": "#/definitions/artifact"
         },
+        "oci-manifest": {
+          "$id": "#/properties/images/properties/oci-manifest",
+          "type": "object",
+          "title": "OCI manifest",
+          "$ref": "#/definitions/artifact"
+        },
         "dasd": {
           "$id": "#/properties/images/properties/dasd",
           "type": "object",

jlebon · 2023-05-15T16:07:58Z

We started on this together

Hmm, was thinking more this would be another known metadata file in the build dir. Do you think we need to go all the way to making a new artifact for this?

cgwalters · 2023-05-31T14:12:58Z

Hmm I'm not finding useful logs in the rhcos test
/retest

cgwalters · 2023-05-31T14:13:56Z

The upgrade test in jenkins was just a flake, I've restarted it.

src/cmd-build

cgwalters · 2023-05-31T14:57:52Z

Hmm, was thinking more this would be another known metadata file in the build dir. Do you think we need to go all the way to making a new artifact for this?

I don't understand the difference between a "known metadata file" and an artifact in this context. Are you saying that we could upload stuff into the build directory that isn't in meta.json?

When the previous build exists, use its packing structure otherwise container-encapsulate generates a new one

RishabhSaini · 2023-06-01T19:03:28Z

/retest

cgwalters · 2023-06-05T14:48:56Z

Looking at the code more, basically what we want is the default buildfetch to fetch the manifest.json. I think because the fetcher doesn't go through the schema stuff, we could in fact bypass the schema bits for this? But it feels slightly unclean.

Another alternative is to switch to having the pipeline download the base container image by default, and extract the manifest from that. Something like

diff --git a/jobs/build-arch.Jenkinsfile b/jobs/build-arch.Jenkinsfile
index 7ce7b62..af7b384 100644
--- a/jobs/build-arch.Jenkinsfile
+++ b/jobs/build-arch.Jenkinsfile
@@ -216,7 +216,8 @@ lock(resource: "build-${params.STREAM}-${basearch}") {
                     cosa buildfetch --arch=${basearch} \
                         --build ${parent_version} \
                         --url s3://${s3_stream_dir}/builds \
-                        --aws-config-file \${AWS_BUILD_UPLOAD_CONFIG}
+                        --aws-config-file \${AWS_BUILD_UPLOAD_CONFIG} \
+                        --artifacts ostree
                     """)
                 }
             }

A third option is to teach rpm-ostree to accept --prior-container-image as a container image reference, and natively fetch that from a registry.

cgwalters

Overall looks sane to me; I don't quite see how we'd do this without adding a new artifact, so having one makes sense to me.

cgwalters · 2023-06-12T14:34:33Z

src/cmd-build

+    skopeo inspect --raw oci-archive:"${ostree_tarfile_path}" > tmp/manifest.json
+    /usr/lib/coreos-assembler/finalize-artifact tmp/manifest.json "${ostree_oci_manifest_path}"
+    ostree_oci_manifest_sha256=$(sha256sum "${ostree_oci_manifest_path}" | awk '{print$1}')
+    ostree_oci_manifest_size=$(stat --format=%s "${ostree_oci_manifest_path}")


This stuff looks duplicated? Do we need this here?

OK I see why now, we need it in both paths, but maybe extract a helper function for this?

Ah but that's hard because we currently set global variables, and fixing that is messy.

dustymabe · 2023-06-12T18:18:57Z

This is causing problems. I opened a revert in #3506

RishabhSaini mentioned this pull request May 2, 2023

Using prior builds for improving encapsulation coreos/rpm-ostree#4271

Merged

3 tasks

jlebon reviewed May 3, 2023

View reviewed changes

src/cmd-build Outdated Show resolved Hide resolved

src/cmd-build Outdated Show resolved Hide resolved

RishabhSaini mentioned this pull request May 8, 2023

Using historical build information for encapsulation coreos/rpm-ostree#4247

Closed

3 tasks

cgwalters added jira for syncing to jira and removed jira for syncing to jira labels May 30, 2023

RishabhSaini added the jira for syncing to jira label May 30, 2023

RishabhSaini self-assigned this May 30, 2023

RishabhSaini force-pushed the packingStructure branch from 7d88222 to ca5266f Compare May 31, 2023 05:02

cgwalters reviewed May 31, 2023

View reviewed changes

src/cmd-build Outdated Show resolved Hide resolved

src/cmd-build Show resolved Hide resolved

RishabhSaini force-pushed the packingStructure branch 2 times, most recently from 51e4bda to 599491e Compare May 31, 2023 19:06

cmd-build: Conditionally change the packing structure of container-image

50eea7d

When the previous build exists, use its packing structure otherwise container-encapsulate generates a new one

RishabhSaini force-pushed the packingStructure branch from 599491e to 50eea7d Compare June 1, 2023 16:50

cgwalters reviewed Jun 12, 2023

View reviewed changes

cgwalters approved these changes Jun 12, 2023

View reviewed changes

cgwalters enabled auto-merge (rebase) June 12, 2023 14:39

cgwalters merged commit a2e5edb into coreos:main Jun 12, 2023
3 checks passed

dustymabe mentioned this pull request Jun 12, 2023

[stable] tree: promote changes from testing at 4d8450fa491d94f92330d6e162c1556310a61af0 coreos/fedora-coreos-config#2463

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd-build: Conditionally change the packing structure of container-image #3445

cmd-build: Conditionally change the packing structure of container-image #3445

RishabhSaini commented May 2, 2023

cgwalters commented May 8, 2023

jlebon commented May 8, 2023

cgwalters commented May 15, 2023

jlebon commented May 15, 2023

cgwalters commented May 31, 2023

cgwalters commented May 31, 2023

cgwalters commented May 31, 2023

RishabhSaini commented Jun 1, 2023

cgwalters commented Jun 5, 2023

cgwalters left a comment

cgwalters Jun 12, 2023

cgwalters Jun 12, 2023

cgwalters Jun 12, 2023

dustymabe commented Jun 12, 2023

cmd-build: Conditionally change the packing structure of container-image #3445

cmd-build: Conditionally change the packing structure of container-image #3445

Conversation

RishabhSaini commented May 2, 2023

cgwalters commented May 8, 2023

jlebon commented May 8, 2023

cgwalters commented May 15, 2023

jlebon commented May 15, 2023

cgwalters commented May 31, 2023

cgwalters commented May 31, 2023

cgwalters commented May 31, 2023

RishabhSaini commented Jun 1, 2023

cgwalters commented Jun 5, 2023

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters Jun 12, 2023

Choose a reason for hiding this comment

cgwalters Jun 12, 2023

Choose a reason for hiding this comment

cgwalters Jun 12, 2023

Choose a reason for hiding this comment

dustymabe commented Jun 12, 2023