-
Notifications
You must be signed in to change notification settings - Fork 167
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
buildextend-live: Add support OSBUILD #3861
base: main
Are you sure you want to change the base?
Conversation
ravanelli
commented
Aug 19, 2024
- Add support for OSBUILD via COSA_USE_LIVEISO env var for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks sane overall! Want to fold the commits in #3847 into this one?
We run osbuild inside of supermin, so for building the live ISO we'll need all the tools it needs in there. This includes `genisoimage` and `syslinux-nonlinux`. We don't currently name `squashfs-tools` as a toplevel dependency. It's currently pulled in by libguestfs, but we need it in supermin too, so explicitly list it there.
This adds a new osbuild pipeline for building the live ISO using the new `org.osbuild.coreos.live-iso` mega stage. Still missing is changing `cmd-buildextend-live` to call osbuild instead. I think we'll need to ratchet this in using e.g. an env var like we did for the other osbuild artifacts. E.g. `COSA_OSBUILD_LIVE` would control whether to use the old logic or the new one. And then turn it on in rawhide, etc... The new `cmd-buildextend-live` would also take care of extracting the live artifacts from the live ISO to insert into `meta.json`.
…ze naming - Rename `ignition_img_size` to `IGNITION_IMG_SIZE` and `miniso_data_file_size` to `MINISO_DATA_FILE_SIZE` for consistent naming convention; - Move these definitions to the top level for better visibility and maintainability Signed-off-by: Renata Ravanelli <[email protected]>
…creation - Moved variable declarations and directories creation into generate_iso() to enhance clarity and maintainability. Signed-off-by: Renata Ravanelli <[email protected]>
- Moved variable declarations to top to enhance clarity. Signed-off-by: Renata Ravanelli <[email protected]>
4bbcf0a
to
75f616f
Compare
75f616f
to
954c139
Compare
- Add support for OSBUILD via COSA_OSBUILD_LIVEISO env var for now. Once we finish the OSBUILD integration we can disable it via var. Signed-off-by: Renata Ravanelli <[email protected]>
954c139
to
543fdf6
Compare
@ravanelli: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a review here but I'm not sure we should really merge this until we get a better idea of what the actual stages will look like (i.e. the manifests and the inputs will change).
initramfs_file = os.path.join(builddir, initramfs_name) | ||
rootfs_file = os.path.join(builddir, rootfs_name) | ||
|
||
if COSA_OSBUILD_LIVEISO != "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just put this if statement down in the logic at the bottom of this file.
# Extract live artifacts from ISO. OSBUILD does not support an output with multiple files | ||
command = ["coreos-installer", "iso", "extract", "pxe", tmpisofile] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be fine for now, but really we should restructure the way we use osbuild today to be able to support multiple outputs. I think the limitation you mention is just part of how we wired in osbuild here.
tmpisofile | ||
] | ||
subprocess.run(command) | ||
update_buildmeta() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have to call this function for the osbuild case but not for the other case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think another way to say this is ideally we get rid of the very similar code in the legacy path and have it call update_buildmeta()
also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two of the commits have a leading space in their titles.
The last commit is missing a blank line between the title and the body, which messes up git log --oneline
.
Agree we should wait until we finalize the stage before adding the pipeline here. Though if you'd like, we can at least get the prep commits in for now in a separate PR.
] | ||
subprocess.run(command) | ||
update_buildmeta() | ||
# Extract live artifacts from ISO. OSBUILD does not support an output with multiple files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, as discussed I think it'd be more accurate to say e.g.
Extract live artifacts from ISO until we rework runvm-osbuild to support outputting multiple files
tmpisofile | ||
] | ||
subprocess.run(command) | ||
update_buildmeta() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think another way to say this is ideally we get rid of the very similar code in the legacy path and have it call update_buildmeta()
also.
update_buildmeta() | ||
# Extract live artifacts from ISO. OSBUILD does not support an output with multiple files | ||
command = ["coreos-installer", "iso", "extract", "pxe", tmpisofile] | ||
subprocess.run(command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we missing steps here to move the artifacts into the build dir?
Combining with the previous comment, I think maybe we can make update_buildmeta()
take as arguments the paths to the ISO, kernel, initramfs, and rootfs, and it takes care of moving them into the build dir and updating meta.json
.