osbuild: support building multiple platforms at once #3930

dustymabe · 2024-11-09T21:00:08Z

This is done by adding cosa osbuild qemu metal....

There is a lot of refactor/cleanup in this PR. Please review commits individually.

src/cmd-buildextend-metal

With coreos/coreos-assembler#3930 we can now build multiple artifacts/platforms in a single OSBuild call. Let's add support here for: 1. Detecting if an artifact supports building via OSBuild by looking to see if the buildextend-{artifact} API points to cmd-osbuild. i.e. is OSBuild the default for this artifact? 2. Allowing the pipecfg to specify additional artifacts to build using OSBuild. This is useful if OSBuild is not yet the default way to build this particular artifact.

jlebon

Very nice! Overall LGTM.

src/cmd-buildextend-metal

src/cmd-osbuild

dustymabe · 2024-11-13T04:42:33Z

Pushed an update to address code review comments and also update a few missing things from the gcp/applehv/hyperv platforms.

dustymabe · 2024-11-13T13:18:40Z

ideas on this shellcheck error?

./tests/check_one.sh src/cmd-osbuild src/.cmd-osbuild.shellchecked
if ${skip_compress}:
Possible unbracketed conditional in src/cmd-osbuild
make: *** [Makefile:39: src/.cmd-osbuild.shellchecked] Error 1

The code it's complaining about is embedded python.

dustymabe · 2024-11-13T14:05:05Z

ideas on this shellcheck error?

./tests/check_one.sh src/cmd-osbuild src/.cmd-osbuild.shellchecked
if ${skip_compress}:
Possible unbracketed conditional in src/cmd-osbuild
make: *** [Makefile:39: src/.cmd-osbuild.shellchecked] Error 1

It's not actually shellcheck that is complaining. It's hitting this check that we added in #3155

coreos-assembler/tests/check_one.sh

Lines 30 to 33 in 48fba72

    
           if grep -E '(^|[[:space:]])if[[:space:]]+"?\$' "$f"; then 
        
               echo "Possible unbracketed conditional in $f" 
        
               exit 1 
        
           fi

The results of running this were never used and we stopped running it in coreos/fedora-coreos-pipeline@81304c0

We never officially supported this and when we switched to OSBuild in d37958a and dropped create_disk.sh this became dead code.

No longer used since we dropped create_disk.sh in d37958a.

This will enable us to generate the input config for runvm-osbuild once and re-use it for all platforms. In other words, the input JSON to runvm-osbuild shouldn't change between invocations.

- Add getconfig() helper functions to reduce boilerplate - Rename variable for config passed to runvm-osbuild to make it more clear - Create less files - Use variables and pipes instead of creating multiple files - Limit the config for runvm-osbuild - Only pass in the variables that are used there and not the entire image.json

…results Let's refactor the generation of the runvm-osbuild config into a function and cache the results since we don't need to compute these values every time we run OSBuild.

…nfig() This only needs to happen when we're generating the config.

This will help my mental model a bit.

In this case we'll just copy out the entire exported tree for a given pipeline (which we specify as --export=platform). For individual artifacts we'll just name the file the same name as the platform and then copy them into the right place in the calling script. The benefits of this can be seen immediately because now we don't have to know about or copy around qemu-secex bootfs_hash and rootfs_hash inside the supermin VM.

Allows for a little more reasoning on what is happening when.

From what I can tell this is only used in the case where we are delaying meta.json merging until later. For example, when parallel building and we want meta.json snippets (i.e. meta.aws.json) to be what gets merged at the end. Let's remove it from here to simplify.

This separates them. It adds a call to `cosa meta` but now the code stands on its own and the ordering doesn't matter.

Reduces duplicate code by making a function to handle the meta.json update and finalization of the artifact.

Improves code readability.

This script isn't set -x to begin with so this doesn't really do anything right now.

It's unlikely to change. This is prep to make a future change easier.

And also refactor to drop the rc variable.

This is a direct copy of cmd-buildextend-metal for now. Will update it to be functional in the next commit.

dustymabe · 2024-11-13T17:09:33Z

It's not actually shellcheck that is complaining. It's hitting this check that we added in #3155

OK. I was able to figure out a workaround.. rebased the PR and added a commit with the workaround.

Now you can run `cosa osbuild qemu metal gcp...` it will build them all. The following files are now symlinks to `cmd-osbuild` and all backwards compatibility should be preserved. - cmd-buildextend-metal - cmd-buildextend-metal4k - cmd-buildextend-qemu - cmd-buildextend-qemu-secex - cmd-buildextend-secex

Turns out we can stencil in the name of the artifacts we want to create and just have OSBuild go ahead and write files to the right name. Also dropped an extra copy that's not really necessary.

This leverages the featurework in upstream OSBuild [1] and matches the compression level we use today for gzip compression. [1] osbuild/osbuild@cfaabe6

In qemuvariants.py applehv is gzip compressed so let's do the same here and tell postprocess_artifact to skip compression in meta.json. While we are here also have gcp set to skip compression in meta.json.

This reverts commit 4155975. The upstream fix [1] was released in OSBuild v132 that is in Fedora 40 now. [1] osbuild/osbuild#1908

The artifact we create in the pipeline today is .vhdx.zip. Let's build on the work upstream in [1] and use the new OSBuild zip stage to encapsulate the artifact. [1] osbuild/osbuild@ac8a2a4

The check is invalid for this code because it's embedded python. Let's just workaround it for now by putting it in a elif so it won't match the string being looked for in https://github.com/coreos/coreos-assembler/blob/48fba72a62c739f6eaf4d7991052516fd574acc7/tests/check_one.sh#L30-L33

jlebon

LGTM!

jlebon · 2024-11-13T19:17:51Z

src/cmd-osbuild

+# backwards conditional here because of a invalid (for this code) CI check
+# https://github.com/coreos/coreos-assembler/pull/3930#issuecomment-2473714222
+if False:
+    pass
+elif ${skip_compress}:
+    j['images']['${artifact_name}']['skip-compression'] = True


Heh, that's actually kinda funny.

I think another way around this is:

if True == ${skip_compression}: ...

Not worth the respin

With coreos/coreos-assembler#3930 we can now build multiple artifacts/platforms in a single OSBuild call. Let's add support here for: 1. Detecting if an artifact supports building via OSBuild by looking to see if the buildextend-{artifact} API points to cmd-osbuild. i.e. is OSBuild the default for this artifact? 2. Allowing the pipecfg to specify additional artifacts to build using OSBuild. This is useful if OSBuild is not yet the default way to build this particular artifact.

With coreos/coreos-assembler#3930 we can now build multiple artifacts/platforms in a single OSBuild call. Let's add support here for detecting what artifacts are supported to be built by OSBuild and build them using OSBuild. We segregate here "experimental" versus "stable" OSbuild built artifacts and add a pipecfg knob for opting into the "experimental" ones being built by OSBuild.

foo With coreos/coreos-assembler#3930 we can now build multiple artifacts/platforms in a single OSBuild call. Let's add support here for detecting what artifacts are supported to be built by OSBuild and build them using OSBuild. We segregate here "experimental" versus "stable" OSbuild built artifacts and add a pipecfg knob for opting into the "experimental" ones being built by OSBuild.

With coreos/coreos-assembler#3930 we can now build multiple artifacts/platforms in a single OSBuild call. Let's add support here for detecting what artifacts are supported to be built by OSBuild and build them using OSBuild. We segregate here "experimental" versus "stable" OSbuild built artifacts and add a pipecfg knob for opting into the "experimental" ones being built by OSBuild.

dustymabe mentioned this pull request Nov 9, 2024

osbuild: more refactor to prep for calling osbuild to generate multiple artifacts #3925

Closed

dustymabe force-pushed the dusty-cmd-osbuild branch from c71b1d2 to 7c4be32 Compare November 10, 2024 18:06

ravanelli reviewed Nov 12, 2024

View reviewed changes

src/cmd-buildextend-metal Outdated Show resolved Hide resolved

jlebon previously approved these changes Nov 12, 2024

View reviewed changes

src/cmd-buildextend-metal Outdated Show resolved Hide resolved

src/cmd-osbuild Outdated Show resolved Hide resolved

src/cmd-osbuild Outdated Show resolved Hide resolved

dustymabe dismissed jlebon’s stale review via af630d1 November 13, 2024 04:41

dustymabe force-pushed the dusty-cmd-osbuild branch from 7c4be32 to af630d1 Compare November 13, 2024 04:41

dustymabe mentioned this pull request Nov 13, 2024

qemu-secex: improve localbuild #3927

Merged

dustymabe added 18 commits November 13, 2024 12:08

drop buildextend-hashlist-experimental

15c5efa

The results of running this were never used and we stopped running it in coreos/fedora-coreos-pipeline@81304c0

cmd-buildextend-metal: drop ext4 fsverify handling

d3ed003

We never officially supported this and when we switched to OSBuild in d37958a and dropped create_disk.sh this became dead code.

cmd-buildextend-metal: drop platforms.json code

269b1f1

No longer used since we dropped create_disk.sh in d37958a.

runvm-osbuild: get platform information from --platform option

0f9d157

This will enable us to generate the input config for runvm-osbuild once and re-use it for all platforms. In other words, the input JSON to runvm-osbuild shouldn't change between invocations.

cmd-buildextend-metal: create generate_runvm_osbuild_config(); cache …

70f67f2

…results Let's refactor the generation of the runvm-osbuild config into a function and cache the results since we don't need to compute these values every time we run OSBuild.

cmd-buildextend-metal: drain more code into generate_runvm_osbuild_co…

7031e93

…nfig() This only needs to happen when we're generating the config.

cmd-buildextend-metal: variable rename s/image_type/platform

ff4e509

This will help my mental model a bit.

cmd-buildextend-metal: move most of script into main() function

0b7016c

Allows for a little more reasoning on what is happening when.

cmd-buildextend-metal: move ignition secex key into secex if conditional

4f73cd8

This separates them. It adds a call to `cosa meta` but now the code stands on its own and the ordering doesn't matter.

cmd-buildextend-metal: create postprocess_artifact() function

7a93031

Reduces duplicate code by making a function to handle the meta.json update and finalization of the artifact.

cmd-buildextend-metal: move qemu-secex postprocessing into a function

3329c12

Improves code readability.

cmd-buildextend-metal: remove set +x

ff12531

This script isn't set -x to begin with so this doesn't really do anything right now.

cmd-buildextend-metal: hardcode image format for secex VM

1ab2869

It's unlikely to change. This is prep to make a future change easier.

cmd-buildextend-metal: set variables at top of main

9099abd

And also refactor to drop the rc variable.

cmd-osbuild: add foundation for what will be cosa osbuild

f0c6c92

This is a direct copy of cmd-buildextend-metal for now. Will update it to be functional in the next commit.

dustymabe force-pushed the dusty-cmd-osbuild branch from af630d1 to b45d0fe Compare November 13, 2024 17:09

dustymabe added 7 commits November 13, 2024 12:27

osbuild: create artifact files with final filenames

cfcf81c

Turns out we can stencil in the name of the artifacts we want to create and just have OSBuild go ahead and write files to the right name. Also dropped an extra copy that's not really necessary.

osbuild/gcp: use gzip compression level 9

0f60219

This leverages the featurework in upstream OSBuild [1] and matches the compression level we use today for gzip compression. [1] osbuild/osbuild@cfaabe6

osbuild/applehv: output raw.gz

dcd65ff

In qemuvariants.py applehv is gzip compressed so let's do the same here and tell postprocess_artifact to skip compression in meta.json. While we are here also have gcp set to skip compression in meta.json.

Revert "vmdeps: add python3-dnf for now"

0551270

This reverts commit 4155975. The upstream fix [1] was released in OSBuild v132 that is in Fedora 40 now. [1] osbuild/osbuild#1908

osbuild/hyperv: run through zip

cbecdeb

The artifact we create in the pipeline today is .vhdx.zip. Let's build on the work upstream in [1] and use the new OSBuild zip stage to encapsulate the artifact. [1] osbuild/osbuild@ac8a2a4

dustymabe force-pushed the dusty-cmd-osbuild branch from b45d0fe to bb17179 Compare November 13, 2024 17:29

jlebon approved these changes Nov 13, 2024

View reviewed changes

dustymabe merged commit 7496933 into coreos:main Nov 13, 2024
5 checks passed

dustymabe deleted the dusty-cmd-osbuild branch November 13, 2024 21:54

dustymabe mentioned this pull request Nov 13, 2024

support conditionally building artifacts in an OSBuild stage coreos/fedora-coreos-pipeline#1055

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

osbuild: support building multiple platforms at once #3930

osbuild: support building multiple platforms at once #3930

Uh oh!

dustymabe commented Nov 9, 2024 •

edited

Loading

Uh oh!

Uh oh!

jlebon left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dustymabe commented Nov 13, 2024

Uh oh!

dustymabe commented Nov 13, 2024

Uh oh!

dustymabe commented Nov 13, 2024

Uh oh!

dustymabe commented Nov 13, 2024

Uh oh!

jlebon left a comment

Uh oh!

jlebon Nov 13, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

osbuild: support building multiple platforms at once #3930

osbuild: support building multiple platforms at once #3930

Uh oh!

Conversation

dustymabe commented Nov 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dustymabe commented Nov 13, 2024

Uh oh!

dustymabe commented Nov 13, 2024

Uh oh!

dustymabe commented Nov 13, 2024

Uh oh!

dustymabe commented Nov 13, 2024

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

jlebon Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dustymabe commented Nov 9, 2024 •

edited

Loading