Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

osbuild: add support for qemu-secex #3764

Merged
merged 7 commits into from
Jul 11, 2024
Merged

Conversation

nikita-dubrovskii
Copy link
Contributor

Discussion also goes in: dustymabe/osbuild#22

Copy link
Contributor

@jschintag jschintag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel qualified to comment on the osbuild part, though i looked over it and i did not find anything that looked wrong to me.

One question, how is the naming scheme of the patch files, since normally i would expect the numbers to show the order they need to be applied in and for them to unique? But here is see 0001 is used twice?

src/cmd-buildextend-metal Outdated Show resolved Hide resolved
src/cmd-buildextend-metal Outdated Show resolved Hide resolved
src/cmd-buildextend-metal Show resolved Hide resolved
src/cmd-buildextend-metal Outdated Show resolved Hide resolved
@dustymabe
Copy link
Member

One question, how is the naming scheme of the patch files, since normally i would expect the numbers to show the order they need to be applied in and for them to unique? But here is see 0001 is used twice?

they are written to stdout using cat so they just go in the order they are given to the cat command.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before going further, should we think about how we'll tackle the prod case since it's very different from the local build case?

src/osbuild-manifests/platform.qemu-secex.ipp.yaml Outdated Show resolved Hide resolved
src/runvm-osbuild Outdated Show resolved Hide resolved
src/cmd-buildextend-metal Show resolved Hide resolved
src/cmd-buildextend-metal Outdated Show resolved Hide resolved
src/osbuild-manifests/coreos.osbuild.s390x.secex.mpp.yaml Outdated Show resolved Hide resolved
@nikita-dubrovskii
Copy link
Contributor Author

osbuild pr: osbuild/osbuild#1806

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Mostly LGTM.

src/cmd-buildextend-metal Show resolved Hide resolved
src/cmd-buildextend-metal Outdated Show resolved Hide resolved
src/cmd-buildextend-metal Outdated Show resolved Hide resolved
src/secex-genprotimgvm-scripts/genprotimg-script.sh Outdated Show resolved Hide resolved
@nikita-dubrovskii nikita-dubrovskii changed the title WIP: osbuild: add support for qemu-secex osbuild: add support for qemu-secex Jul 1, 2024
@nikita-dubrovskii
Copy link
Contributor Author

@jschintag could you please check the scripts with official genprotimgvm ?

@jlebon
Copy link
Member

jlebon commented Jul 4, 2024

/retest

jlebon
jlebon previously approved these changes Jul 4, 2024
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments but LGTM overall.

Nice work on this @nikita-dubrovskii. It's been a long time coming!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth a comment here at the top of this file about what this Butane config is for.

Would be nice to put the instructions in the commit message somewhere more visible. Maybe in https://coreos.github.io/coreos-assembler/devel/ ?

Not a blocker/can be a follow-up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke OOB with Nikita about this. In a follow-up, he will add logic that will actually start up a VM with that Butane config for the local dev case.

src/runvm-osbuild Outdated Show resolved Hide resolved
src/cmd-buildextend-metal Outdated Show resolved Hide resolved
src/cmd-buildextend-metal Outdated Show resolved Hide resolved
src/cmd-buildextend-metal Outdated Show resolved Hide resolved
This template coul be used to create genprotimgvm locally. During
build this VM is used to generete 'sdboot' image and 'zipl' it on
coreos.qemu-secex.s390x.qcow2 image.

Put this template, your id_rsa.pub and valid secex-hostkey into
same directory and generate Ignition config:
```
butane --pretty --files-dir . genprotimg.bu -o genprotimg.ign
```

Than get rhcos.qemu-secex.s390x.qcow2 and customize it with
generated above ignition config.
@jschintag
Copy link
Contributor

@jschintag could you please check the scripts with official genprotimgvm ?

I have run it on the builder and i could build a secure execution test and run the associated kola tests successfully.

@nikita-dubrovskii
Copy link
Contributor Author

@jschintag could you please check the scripts with official genprotimgvm ?

I have run it on the builder and i could build a secure execution test and run the associated kola tests successfully.

Great, thx for testing!

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let's do it!

To be safe, I've created a :before-pr3764 tag in Quay.io from the current :latest tag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke OOB with Nikita about this. In a follow-up, he will add logic that will actually start up a VM with that Butane config for the local dev case.

@jlebon jlebon merged commit 1bbf52a into coreos:main Jul 11, 2024
5 checks passed
@nikita-dubrovskii nikita-dubrovskii deleted the secex branch July 12, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants