build-extensions-container: add command to build the extensions container #2985

jmarrero · 2022-07-14T20:29:43Z

COS-1646: deliver the extensions-container with meta.json as explained in: openshift/os#763 (comment)

openshift-ci · 2022-07-14T20:29:46Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

cgwalters · 2022-08-15T17:50:14Z

Now that #2919 landed - this code could start out in Go. Just an option to consider.

cgwalters · 2022-08-15T17:58:21Z

src/cmd-buildextend-extensions-container

+    return parser.parse_args()
+
+def run_container_build(context_dir):
+    print("call runvm and podman inside it...")


Yeah, all this logic is in shell (cmdlib.sh) today unfortunately 😢

But, combining this and my comment about #2919 - today the cosa Go library has good support for running code from cmdlib.sh.

dustymabe · 2022-08-26T00:59:13Z

cmd/buildextend-extensions-container.go

@@ -0,0 +1,43 @@
+// See usage below


probably should rename this file build-extensions-container.go

dustymabe · 2022-08-26T01:01:11Z

Is this going to need to be run on different architectures? or just on x86_64?

jmarrero · 2022-08-26T12:27:12Z

Is this going to need to be run on different architectures? or just on x86_64?

This is part of the effort for Openshift 4.12, I don't see why it would be limited to only x86_64. I assume this will be built for all the architectures we support on Openshift/RHCOS.

cgwalters · 2022-08-26T18:57:24Z

src/build-extensions-oscontainer.sh

+cd src/config
+#Replace the FROM line with the ociarchive
+#FROM oci-archive:/srv/builds/VERSION/x86_64/rhcos-VERSION.ociarchive as os
+sed -i "s|$RHCOS_IMAGE|oci-archive:$RHCOS_OCIARCHIVE|" extensions/Dockerfile


I think we can just use podman build --from oci-archive:$RHCOS_OCIARCHIVE

(Also tangentially in theory this isn't specific to RHCOS, so in the variable names and such we can just say e.g. ostree_ociarchive or something)

cgwalters · 2022-08-26T19:16:44Z

cmd/build-extensions-container.go

+)
+
+func buildExtensionContainer()  error {
+	fmt.Println("Calling cmdlib")


Can remove these leftover debug bits I assume?

cgwalters · 2022-08-26T19:17:42Z

cmd/build-extensions-container.go

+	if _, err := sh.PrepareBuild(); err != nil {
+		return err
+	}
+    sh.Process("runvm -- /usr/lib/coreos-assembler/build-extensions-oscontainer.sh $tmp_builddir/output.txt")


Just going to call out I think this CosaSh thing is demonstrating its value here in allowing us to reuse all the shell script logic.

cgwalters · 2022-08-30T16:57:15Z

src/cmd-push-container

+extensions_ociarchive = os.path.join(latest_build_path, meta['images']['extensions-container']['path'])
+if not extensions_ociarchive:
+    ociarchives = ["ostree", "extensions-container"]


Here extensions_ociarchive will always be a string i.e. truthy. I think you may have meant to do e.g.:

containers_to_push = ["ostree"] extensions = meta['images'].get('extensions-container') if extensions: containers_to_push.append('extensions-container')

or so?

big woops, thanks!

cgwalters · 2022-08-30T16:59:00Z

src/cmd-push-container

-        json.dump(meta, f, sort_keys=True)
-    shutil.move(metapath_new, metapath)
+    ociarchives = ["ostree"]
+for archive_id in ociarchives:


Ah, interesting. I'd been thinking we'd have two separate calls to push-container in the pipeline, like push-container --ostree and push-container --extensions or something as separate calls. But, I'm OK doing both at once too.

Ahh I see, the idea I had was since both need to be pushed together going forward then always check if the extensions container was there and push it. But changing it to a flag is a quick change.

I'm definitely OK having one command push two containers; it becomes slightly odd since it's now really push-containers plural but...eh. (Renaming it would require ratcheting into the pipelines, let's not do that)

I'll change it. Thinking about it this might be bad if in the pipeline we are doing build and then push right away. Basically I guess that we would call this script twice either way. It will also minimize the changes on the code. No loops, etc.

I guess if we don't want to adjust the pipelines then I added --extensions parameter only and then ostree is the default behavior.

cgwalters · 2022-08-30T19:50:06Z

cmd/build-extensions-container.go

+	//"os/exec"
+)
+
+type MetaJSON struct {


We should have all this defined in "github.com/coreos/coreos-assembler-schema/cosa"

all makes sense now...

cgwalters · 2022-08-30T19:51:24Z

cmd/build-extensions-container.go

+
+	file, err := os.Open(ociarchive)
+	if err != nil {
+		log.Fatal(err)


Let's do return err here for consistentency.

cgwalters · 2022-08-30T19:51:32Z

cmd/build-extensions-container.go

+
+	stat, err := file.Stat()
+	if err != nil {
+		//file no here?


I think this should be return err too.

cgwalters · 2022-09-01T15:05:02Z

If you rebase on #3063 it should avoid a lot of the "vendor duplication" when trying to pull in schema/ into the toplevel.

jmarrero · 2022-09-01T22:09:44Z

This is looking good to me so far!

If you haven't yet tried this; one problem today is that no CI covers push-container. I've been testing it manually by doing e.g.
$ cosa push-container quay.io/cgwalters/fcos
I'd do the same for your extensions container.

Thanks, checked and is working as expected:

[coreos-assembler]$ cosa push-container --image=extensions-container quay.io/jmarrero_rh/my-custom-fcos
skopeo copy --authfile /development/config.json --digestfile=/srv/tmp/push-container-digestfilei3hvgmq3 oci-archive:builds/412.86.202209012000-0/x86_64/extensions-container-412.86.202209012000-0-x86_64.ociarchive docker://quay.io/jmarrero_rh/my-custom-fco4
Getting image source signatures
Copying blob 0c5aff2d7eef done  
Copying blob 7b7a31b480d6 done  
Copying blob 7fa2ab540ed8 done  
Copying blob a1ec893e47ea done  
Copying config 79e8f2318b done  
Writing manifest to image destination
Storing signatures
[coreos-assembler]$

jmarrero · 2022-09-01T23:09:40Z

Testing I found that somehow I am dropping coreos-assembler.image-genver & delayed-meta-merge when writing the JSON. they are part of the schema so I am not sure why yet.

➜  builds rg coreos-assembler.image-genver
412.86.202209012254-0/x86_64/meta.json
14:    "coreos-assembler.image-genver": 0,

412.86.202209012240-0/x86_64/meta.json
14:    "coreos-assembler.image-genver": 0,
➜  builds cd ..
➜  rhcos cd buil-bad 
➜  buil-bad rg coreos-assembler.image-genver
➜  buil-bad

jmarrero · 2022-09-02T00:17:16Z

Testing I found that somehow I am dropping coreos-assembler.image-genver & delayed-meta-merge when writing the JSON. they are part of the schema so I am not sure why yet.
➜  builds rg coreos-assembler.image-genver
412.86.202209012254-0/x86_64/meta.json
14:    "coreos-assembler.image-genver": 0,

412.86.202209012240-0/x86_64/meta.json
14:    "coreos-assembler.image-genver": 0,
➜  builds cd ..
➜  rhcos cd buil-bad 
➜  buil-bad rg coreos-assembler.image-genver
➜  buil-bad 

After Unmarshaling the value is present in the object, but somehow dropped a line after on the Marshal.

	fmt.Printf("image ver %d \n",cosaBuild.CosaImageVersion)

	newBytes, err := json.MarshalIndent(cosaBuild, "", "    ")

wonder if I am hitting some kind of reference issue.

cgwalters · 2022-09-02T13:13:55Z

OK first trying this out the find call seems to be traversing the entire cosa directory, which is really expensive and prints errors for me. I think what you were running into here is that because we need to cd src/config for podman, we want the absolute path of the input file. We can do that by just adding $PWD into the filename before we do the cd. I did that and some other cleanups here:

EDIT: OK made some further changes, can you take a look?

index 5f238d5c5..209fada28 100644
--- a/cmd/build-extensions-container.go
+++ b/cmd/build-extensions-container.go
@@ -11,10 +11,16 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"strings"
 )
 
 func buildExtensionContainer() error {
+	lastBuild, buildPath, err := cosa.ReadBuild("builds", "", "")
+	if err != nil {
+		return err
+	}
+	buildID := lastBuild.BuildID
+	fmt.Printf("Generating extensions container for build: %s\n", buildID)
+
 	arch := cosa.BuilderArch()
 	sh, err := cosash.NewCosaSh()
 	if err != nil {
@@ -23,29 +29,23 @@ func buildExtensionContainer() error {
 	if _, err := sh.PrepareBuild(); err != nil {
 		return err
 	}
-	process := "runvm -- /usr/lib/coreos-assembler/build-extensions-oscontainer.sh " + arch + " $tmp_builddir/output.txt"
-	sh.Process(process)
-	tmpdir, err := sh.ProcessWithReply("echo $tmp_builddir>&3\n")
-	if err != nil {
-		return err
-	}
-	content, err := ioutil.ReadFile(filepath.Join(tmpdir, "output.txt"))
-	if err != nil {
+	targetname := "extensions-container-" + buildID + "." + arch + ".ociarchive"
+	process := "runvm -- /usr/lib/coreos-assembler/build-extensions-oscontainer.sh " + arch + " $tmp_builddir/" + targetname
+	if err := sh.Process(process); err != nil {
 		return err
 	}
-	ociarchive := strings.TrimSpace(string(content))
-	workdir := getWorkDir(ociarchive)
-	lastBuild, _, err := cosa.ReadBuild(workdir+"/builds", "latest", arch)
+	// Find the temporary directory allocated by the shell process, and put the OCI archive in its final place
+	tmpdir, err := sh.ProcessWithReply("echo $tmp_builddir>&3\n")
 	if err != nil {
 		return err
 	}
-	buildID := lastBuild.BuildID
-	renamedArchive := filepath.Join(filepath.Dir(ociarchive), "extensions-container-"+buildID+"."+arch+".ociarchive")
-	err = os.Rename(ociarchive, renamedArchive)
+	targetPath := filepath.Join(buildPath, targetname)
+	err = os.Rename(filepath.Join(tmpdir, targetname), targetPath)
 	if err != nil {
 		return err
 	}
-	file, err := os.Open(renamedArchive)
+	// Gather metadata of the OCI archive (sha256, size)
+	file, err := os.Open(targetPath)
 	if err != nil {
 		return err
 	}
@@ -59,9 +59,9 @@ func buildExtensionContainer() error {
 		return err
 	}
 	sha256 := fmt.Sprintf("%x", hash.Sum(nil))
-	builddir := filepath.Join(workdir, "builds", "latest", arch)
-	metapath := filepath.Join(builddir, "meta.json")
 
+	// Update the meta.json to include metadata for our OCI archive
+	metapath := filepath.Join(buildPath, "meta.json")
 	jsonFile, err := os.Open(metapath)
 	if err != nil {
 		fmt.Println(err)
@@ -78,7 +78,7 @@ func buildExtensionContainer() error {
 	}
 
 	cosaBuild.BuildArtifacts.ExtensionsContainer = &cosa.Artifact{
-		Path:            filepath.Base(renamedArchive),
+		Path:            targetname,
 		Sha256:          sha256,
 		SizeInBytes:     float64(stat.Size()),
 		SkipCompression: false,
@@ -95,9 +95,3 @@ func buildExtensionContainer() error {
 	}
 	return nil
 }
-
-func getWorkDir(path string) string {
-	directories := strings.Split(path, "/")
-	//expects path starts with /.
-	return "/" + directories[1]
-}
diff --git a/src/build-extensions-oscontainer.sh b/src/build-extensions-oscontainer.sh
index 0afa76499..99b81d424 100755
--- a/src/build-extensions-oscontainer.sh
+++ b/src/build-extensions-oscontainer.sh
@@ -1,14 +1,19 @@
 #!/bin/bash
 #Used by cmd/build-extensions-container.go
 #Find the RHCOS ociarchive.
-path="*/builds/latest/${1}/*-ostree*.ociarchive"
-ostree_ociarchive=$(find -L ~+ -path "${path}")
-cd src/config || exit
-#Start the build replacing the from line.
-podman build --from oci-archive:"$ostree_ociarchive" --network=host --build-arg COSA=true -t localhost/extensions-container -f extensions/Dockerfile .
-#Call skopeo to generate a extensions container ociarchive
-extensions_ociarchive_dir=$(dirname "$ostree_ociarchive")
-extensions_ociarchive="${extensions_ociarchive_dir}/extensions-container.ociarchive"
-skopeo copy containers-storage:localhost/extensions-container oci-archive:"$extensions_ociarchive"
+set -euo pipefail
+buildid=$1
+shift
+filename=$1
+shift
+builddir="$PWD/builds/latest/${buildid}"
+ostree_ociarchive=$(ls ${builddir}/*-ostree*.ociarchive)
+# Build the image, replacing the FROM directive with the local image we have
+(cd src/config
+ set -x
+ podman build --from oci-archive:"$ostree_ociarchive" --network=host --build-arg COSA=true -t localhost/extensions-container -f extensions/Dockerfile .
+)
+# Call skopeo to export it from the container storage to an oci-archive.
+(set -x
+ skopeo copy containers-storage:localhost/extensions-container oci-archive:"$filename" )
 
-output=$2; echo "$extensions_ociarchive" > "$output"

e.g. parsing the options up front, and with the (set -x; podman ...) we get the command line printed which is useful for debugging, etc.

cgwalters · 2022-09-02T13:41:55Z

OK I think the problem here is that the meta.json we want to read is in the "final" builddir, not the temporary one we allocate.

cgwalters · 2022-09-02T13:46:55Z

Actually sorry I'm not sure that was related to the delayed-meta-merge problem.

Testing I found that somehow I am dropping coreos-assembler.image-genver & delayed-meta-merge when writing the JSON. they are part of the schema so I am not sure why yet.

This is probably the use of omitempty in the schema...but is it a problem?

jmarrero · 2022-09-02T13:51:04Z

Well the issue I hit was the dropping of:

coreos-assembler.image-genver

That is used when we generate a new build on:

src/cosalib/builds.py
131:        genver_key = 'coreos-assembler.image-genver'

Without that key the script crashes.

cgwalters · 2022-09-02T14:00:40Z

OK right, notice here we have drift between the expected semantics in Python and Go code.

How about

diff --git a/src/cosalib/builds.py b/src/cosalib/builds.py
index 4a378d7ba..4e8ebe41c 100644
--- a/src/cosalib/builds.py
+++ b/src/cosalib/builds.py
@@ -136,7 +136,7 @@ class Builds:  # pragma: nocover
                 with open(metapath) as f:
                     previous_buildmeta = json.load(f)
                 previous_commit = previous_buildmeta['ostree-commit']
-                previous_image_genver = int(previous_buildmeta[genver_key])
+                previous_image_genver = int(previous_buildmeta.get(genver_key, 0))
                 if previous_commit == ostree_commit:
                     image_genver = previous_image_genver + 1
                     buildid = f"{version}-{image_genver}"

?

jmarrero · 2022-09-02T14:04:44Z

diff --git a/src/cosalib/builds.py b/src/cosalib/builds.py
index 4a378d7ba..4e8ebe41c 100644
--- a/src/cosalib/builds.py
+++ b/src/cosalib/builds.py
@@ -136,7 +136,7 @@ class Builds:  # pragma: nocover
                 with open(metapath) as f:
                     previous_buildmeta = json.load(f)
                 previous_commit = previous_buildmeta['ostree-commit']
-                previous_image_genver = int(prev

testing

cgwalters · 2022-09-02T15:04:59Z

go.mod

+	github.com/google/uuid v1.1.1 // indirect
+	github.com/json-iterator/go v1.1.10 // indirect
+	github.com/klauspost/cpuid v1.3.1 // indirect
+	github.com/minio/md5-simd v1.1.0 // indirect


Can you rebase on #3063 ?

…iner Co-authored-by: Colin Walters <[email protected]>

cgwalters

Thanks so much for all your work on this!

jmarrero · 2022-09-02T15:33:25Z

Thanks so much for all your work on this!

thank you for all the help!

openshift-ci bot added the do-not-merge/work-in-progress label Jul 14, 2022

openshift-merge-robot added the needs-rebase label Aug 7, 2022

cgwalters reviewed Aug 15, 2022

View reviewed changes

jmarrero force-pushed the build-extensions-container branch from 6ecced8 to 8033aa6 Compare August 18, 2022 19:58

openshift-merge-robot removed the needs-rebase label Aug 18, 2022

jmarrero mentioned this pull request Aug 25, 2022

schema: Add entry for extensions-container #3046

Merged

jmarrero force-pushed the build-extensions-container branch from 0b3c953 to 8aeb7d4 Compare August 25, 2022 21:49

dustymabe reviewed Aug 26, 2022

View reviewed changes

jmarrero force-pushed the build-extensions-container branch from 8aeb7d4 to 0d71c5d Compare August 26, 2022 18:55

cgwalters reviewed Aug 26, 2022

View reviewed changes

jmarrero force-pushed the build-extensions-container branch from 0d71c5d to 6e57507 Compare August 26, 2022 19:10

cgwalters reviewed Aug 26, 2022

View reviewed changes

jmarrero force-pushed the build-extensions-container branch 2 times, most recently from 42247de to fa1eac7 Compare August 26, 2022 20:00

jmarrero changed the title ~~cmd-buildextend: add command to build extensions container~~ build-extensions-container: add command to build the extensions container Aug 26, 2022

jmarrero force-pushed the build-extensions-container branch 4 times, most recently from 63a3f4a to 868e27d Compare August 30, 2022 13:25

cgwalters reviewed Aug 30, 2022

View reviewed changes

jmarrero force-pushed the build-extensions-container branch 2 times, most recently from 996dc18 to 96c6933 Compare August 30, 2022 17:45

cgwalters reviewed Aug 30, 2022

View reviewed changes

jmarrero force-pushed the build-extensions-container branch 2 times, most recently from c71d4b3 to ff49289 Compare September 1, 2022 13:14

jmarrero force-pushed the build-extensions-container branch from da8c905 to b14d14a Compare September 1, 2022 21:13

jmarrero marked this pull request as ready for review September 1, 2022 21:51

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 1, 2022

jmarrero force-pushed the build-extensions-container branch 2 times, most recently from 40c7e06 to 9f3dbd1 Compare September 1, 2022 22:56

jmarrero force-pushed the build-extensions-container branch from 9f3dbd1 to 1b48ed9 Compare September 2, 2022 13:43

jmarrero force-pushed the build-extensions-container branch 3 times, most recently from 935dec4 to 423aabe Compare September 2, 2022 14:40

cgwalters reviewed Sep 2, 2022

View reviewed changes

jmarrero force-pushed the build-extensions-container branch from 423aabe to 80eecad Compare September 2, 2022 15:05

jmarrero and others added 2 commits September 2, 2022 11:09

build-extensions-container: add command to build the extensions conta…

cd3e5c7

…iner Co-authored-by: Colin Walters <[email protected]>

vendor: update vendors to use coreos-assembler-schema on main package

674f39a

jmarrero force-pushed the build-extensions-container branch from 80eecad to 674f39a Compare September 2, 2022 15:10

cgwalters enabled auto-merge (rebase) September 2, 2022 15:29

cgwalters approved these changes Sep 2, 2022

View reviewed changes

cgwalters mentioned this pull request Sep 2, 2022

Move the schema/ package to the toplevel #3069

Merged

cgwalters merged commit 2bc5c08 into coreos:main Sep 2, 2022

This was referenced Sep 13, 2022

container pushing updates #3096

Merged

4.12: legacy upload-oscontainer not tested in unprivileged context openshift/os#1009

Closed

build-extensions-container: add command to build the extensions container #2985

build-extensions-container: add command to build the extensions container #2985

Uh oh!

Conversation

jmarrero commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jul 14, 2022

Uh oh!

cgwalters commented Aug 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dustymabe commented Aug 26, 2022

Uh oh!

jmarrero commented Aug 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrero Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Sep 1, 2022

Uh oh!

jmarrero commented Sep 1, 2022

Uh oh!

jmarrero commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarrero commented Sep 2, 2022

Uh oh!

cgwalters commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters commented Sep 2, 2022

Uh oh!

cgwalters commented Sep 2, 2022

Uh oh!

jmarrero commented Sep 2, 2022

Uh oh!

cgwalters commented Sep 2, 2022

Uh oh!

jmarrero commented Sep 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

jmarrero commented Sep 2, 2022

Uh oh!

Reviewers

Assignees

jmarrero commented Jul 14, 2022 •

edited

Loading

jmarrero Aug 30, 2022 •

edited

Loading

jmarrero commented Sep 1, 2022 •

edited

Loading

cgwalters commented Sep 2, 2022 •

edited

Loading