buildextend-live: add karg embed area offsets to header #1637

jlebon · 2020-08-04T21:18:12Z

Collect ISO files which include karg embed areas, then inject their
offsets into the System Area right behind the initramfs header. This
information will be used by coreos-installer iso embed-kargs.

jlebon · 2020-08-04T21:18:39Z

Requires: coreos/fedora-coreos-config#545

jlebon · 2020-08-04T21:25:20Z

Split out prep patches in #1639.

cgwalters

I think this looks good. I tried to review this one first but was baffled by what "karg files" are until I read coreos/fedora-coreos-config#545

Maybe "bootloader files"?

One of these projects (probably this one) needs a nice big block comment describing this setup with cross links after they merge.

It'd also be good to hoist some of these magic numbers up as constants like ISO_SYSTEM_AREA_SIZE = 32768, MAX_BOOTLOADER_FILES = 6 etc.

jlebon · 2020-08-06T21:20:55Z

Rebased!

Maybe "bootloader files"?

I went the super explicit way and changed it to files_with_karg_embed_areas. :)

One of these projects (probably this one) needs a nice big block comment describing this setup with cross links after they merge.

Yeah, will do that in coreos-installer.

It'd also be good to hoist some of these magic numbers up as constants like ISO_SYSTEM_AREA_SIZE = 32768, MAX_BOOTLOADER_FILES = 6 etc.

I did this (and sprinkled a few more asserts), but didn't hoist it completely up because I think it lives better near the massive comment block where it's all explained.

darkmuggle

/lgtm

openshift-ci-robot · 2020-08-11T22:46:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, darkmuggle, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,darkmuggle,jlebon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

darkmuggle · 2020-08-11T22:48:55Z

I rather enjoy the clever trick of using the iso9660 system area. The only downside is that we need document it in case someone re-masters the ISO.

darkmuggle · 2020-08-11T22:49:11Z

@jlebon needs a rebase

openshift-ci-robot · 2020-10-22T18:05:49Z

New changes are detected. LGTM label has been removed.

nikita-dubrovskii · 2020-10-22T18:14:12Z

This was updated together with coreos/coreos-installer#341

cgwalters · 2020-10-23T20:01:08Z

Try rebasing 🏄 to fix CI

src/cmd-buildextend-live

jlebon · 2020-10-28T19:47:30Z

src/cmd-buildextend-live

@@ -341,6 +342,13 @@ def generate_iso():
                buf = newbuf
                karg_area_end = re.search(r'(#+) COREOS_KARG_EMBED_AREA\n', buf)
                if karg_area_end is not None:
+                    if len(default_kargs) == 0:
+                        default_kargs = buf[karg_area_start.start():karg_area_end.end()]


Shouldn't this be

default_kargs = buf[karg_area_start.start():karg_area_end.start()]

?

In this case length will be less than for other karg files. I though, that it's better to have similar files (ending with COREOS_KARG_EMBED_AREA), than handing different lengths in coreos-installer.
Or, we can modify header format and store there TLV - like structure for each karg file, where Type can be ro or rw

src/cmd-buildextend-live

jlebon

OK, this LGTM now! Had one comment, but not worth the respin.

jlebon · 2020-12-07T20:02:12Z

src/cmd-buildextend-live


            for i, fn in enumerate(files_with_karg_embed_areas):
                offset_in_file = files_with_karg_embed_areas[fn]
-                offsets[i] = file_offset_in_iso(isoinfo, fn) + offset_in_file
+                offsets[i + 1] = file_offset_in_iso(isoinfo, fn) + offset_in_file


Minor/optional: instead of doing + 1 here, I think it'd be cleaner if in the struct.pack() call below we explicitly split out an arg for the offset to .cmdline.

Collect ISO files which include karg embed areas, then inject their offsets into the System Area right behind the initramfs header. This information will be used by `coreos-installer iso embed-kargs`.

Add constants for some of our magic numbers, and make it clear that the formatting of these files cannot be changed without first changing coreos-installer.

…karg This allows installer to modify all kargs backed into iso image Signed-off-by: Nikita Dubrovskii <nikita@linux.ibm.com>

…arg reset' Signed-off-by: Nikita Dubrovskii <nikita@linux.ibm.com>

jlebon · 2020-12-23T18:00:28Z

Rebased this now! Since it already had two approvals, just going to add the lgtm label.

openshift-ci-robot requested review from arithx and cgwalters August 4, 2020 21:18

openshift-ci-robot added the approved label Aug 4, 2020

cgwalters approved these changes Aug 6, 2020

View reviewed changes

jlebon force-pushed the pr/iso-kargs branch from 2e79980 to d8b26a0 Compare August 6, 2020 21:19

openshift-ci-robot added the needs-rebase label Aug 8, 2020

jlebon force-pushed the pr/iso-kargs branch from 62d3155 to bfcbb1b Compare August 8, 2020 15:21

openshift-ci-robot removed the needs-rebase label Aug 8, 2020

darkmuggle approved these changes Aug 11, 2020

View reviewed changes

openshift-ci-robot assigned darkmuggle Aug 11, 2020

openshift-ci-robot added the lgtm label Aug 11, 2020

openshift-ci-robot added the needs-rebase label Aug 13, 2020

cgwalters mentioned this pull request Sep 17, 2020

Publish meta-data for use with virt-manager coreos/fedora-coreos-tracker#221

Open

4 tasks

nikita-dubrovskii force-pushed the pr/iso-kargs branch from bfcbb1b to 09d559b Compare October 22, 2020 18:05

openshift-ci-robot removed the lgtm label Oct 22, 2020

openshift-ci-robot removed the needs-rebase label Oct 22, 2020

nikita-dubrovskii requested review from darkmuggle and cgwalters October 23, 2020 06:52

nikita-dubrovskii force-pushed the pr/iso-kargs branch from 09d559b to d43fe60 Compare October 23, 2020 07:03

jlebon commented Oct 23, 2020

View reviewed changes

src/cmd-buildextend-live Show resolved Hide resolved

nikita-dubrovskii force-pushed the pr/iso-kargs branch 2 times, most recently from 3a9aa13 to 912f10f Compare October 27, 2020 13:37

jlebon commented Oct 28, 2020

View reviewed changes

nikita-dubrovskii force-pushed the pr/iso-kargs branch 3 times, most recently from 2b99e8c to a6239dd Compare October 29, 2020 10:11

jlebon commented Nov 5, 2020

View reviewed changes

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

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

nikita-dubrovskii force-pushed the pr/iso-kargs branch 2 times, most recently from 1fd4084 to f5aa598 Compare November 10, 2020 17:55

nikita-dubrovskii force-pushed the pr/iso-kargs branch from f5aa598 to 1926304 Compare November 17, 2020 18:29

nikita-dubrovskii force-pushed the pr/iso-kargs branch from 1926304 to 7f624b5 Compare December 11, 2020 10:54

jlebon commented Dec 23, 2020

View reviewed changes

jlebon and others added 4 commits December 23, 2020 12:59

buildextend-live: add karg embed area offsets to header

856d557

Collect ISO files which include karg embed areas, then inject their offsets into the System Area right behind the initramfs header. This information will be used by `coreos-installer iso embed-kargs`.

buildextend-live: add constants and solidify format

2fe5b64

Add constants for some of our magic numbers, and make it clear that the formatting of these files cannot be changed without first changing coreos-installer.

buildextend-live: set offset for karg embed area to the first backed …

c66e354

…karg This allows installer to modify all kargs backed into iso image Signed-off-by: Nikita Dubrovskii <nikita@linux.ibm.com>

buildextend-live: add readonly kargs file for 'coreos-installer iso k…

e0145f8

…arg reset' Signed-off-by: Nikita Dubrovskii <nikita@linux.ibm.com>

jlebon force-pushed the pr/iso-kargs branch from 7f624b5 to e0145f8 Compare December 23, 2020 18:00

jlebon added the lgtm label Dec 23, 2020

jlebon closed this Dec 23, 2020

jlebon reopened this Dec 23, 2020

openshift-merge-robot merged commit 77bf671 into coreos:master Dec 23, 2020

jlebon deleted the pr/iso-kargs branch April 22, 2023 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buildextend-live: add karg embed area offsets to header #1637

buildextend-live: add karg embed area offsets to header #1637

jlebon commented Aug 4, 2020

jlebon commented Aug 4, 2020

jlebon commented Aug 4, 2020

cgwalters left a comment

jlebon commented Aug 6, 2020

darkmuggle left a comment

openshift-ci-robot commented Aug 11, 2020

darkmuggle commented Aug 11, 2020

darkmuggle commented Aug 11, 2020

openshift-ci-robot commented Oct 22, 2020

nikita-dubrovskii commented Oct 22, 2020

cgwalters commented Oct 23, 2020

jlebon Oct 28, 2020

nikita-dubrovskii Oct 29, 2020

jlebon left a comment

jlebon Dec 7, 2020

jlebon commented Dec 23, 2020

buildextend-live: add karg embed area offsets to header #1637

buildextend-live: add karg embed area offsets to header #1637

Conversation

jlebon commented Aug 4, 2020

jlebon commented Aug 4, 2020

jlebon commented Aug 4, 2020

cgwalters left a comment

Choose a reason for hiding this comment

jlebon commented Aug 6, 2020

darkmuggle left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 11, 2020

darkmuggle commented Aug 11, 2020

darkmuggle commented Aug 11, 2020

openshift-ci-robot commented Oct 22, 2020

nikita-dubrovskii commented Oct 22, 2020

cgwalters commented Oct 23, 2020

jlebon Oct 28, 2020

Choose a reason for hiding this comment

nikita-dubrovskii Oct 29, 2020

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

jlebon Dec 7, 2020

Choose a reason for hiding this comment

jlebon commented Dec 23, 2020