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

stages/dracut: Add functionality to build initoverlayfs with dracut #1586

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

ericcurtin
Copy link
Contributor

As an alternative to just initramfs. Upstream initoverlayfs project:

https://github.com/containers/initoverlayfs

@achilleas-k
Copy link
Member

Nice and clean but I'd love to see a test for this. Looks good, but I have no idea if it works 😆

stages/org.osbuild.dracut Outdated Show resolved Hide resolved
@ericcurtin
Copy link
Contributor Author

Nice and clean but I'd love to see a test for this. Looks good, but I have no idea if it works 😆

We will be testing thing as part of our sample-images CI builds once this is made available:

https://gitlab.com/CentOS/automotive/sample-images

@ericcurtin ericcurtin force-pushed the initoverlayfs branch 2 times, most recently from acbb60a to 271d636 Compare February 16, 2024 16:25
mvo5
mvo5 previously approved these changes Feb 19, 2024
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you

@mvo5
Copy link
Contributor

mvo5 commented Feb 19, 2024

Fwiw, I would love to see a small test for this functionality, I created
0001-stages-dracut-add-small-unittest-for-initoverlayfs.patch.txt

that validates that the option is running the expected thing. Ideally also a test in test/run/test_stages.py:test_dracut, I will look into this.

@mvo5 mvo5 self-requested a review February 19, 2024 09:29
@mvo5 mvo5 dismissed their stale review February 19, 2024 09:31

Running the integration test found some issues :/

@mvo5
Copy link
Contributor

mvo5 commented Feb 19, 2024

I wrote a small "stage" test for this now that does an end-to-end type test (it will need tweaks but is a useful starting point) for the feature (see attached patch)

0002-test-add-stage-integration-test-for-initoverlayfs-fe.patch.txt

Unfortunately this fails during the creation of the image with an error that --no-hostonly is unknown:

org.osbuild.dracut: e30a0aaa9351c35b3a70d8529fe33de68265af135daef250517b34296953684c {
  "kernel": [
    "6.6.4-200.fc39.x86_64"
  ],
  "initoverlayfs": true
}
Building initramfs for 6.6.4-200.fc39.x86_64
+ [[ 8 -gt 0 ]]
+ case $1 in
+ echo 'Unknown option --no-hostonly'
Unknown option --no-hostonly
+ exit 1
Traceback (most recent call last):
  File "/run/osbuild/bin/org.osbuild.dracut", line 228, in <module>
    r = main(args["tree"], args["options"])
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/run/osbuild/bin/org.osbuild.dracut", line 217, in main
    subprocess.run(["/usr/sbin/chroot", tree, bin,
  File "/usr/lib64/python3.12/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['/usr/sbin/chroot', '/run/osbuild/tree', '/usr/bin/initoverlayfs-install', '--no-hostonly', '--kver', '6.6.4-200.fc39.x86_64', '--force', '-v', '--show-modules', '--no-early-microcode', '--reproducible']' returned non-zero exit status 1.

⏱  Duration: 0s

-- END ---------------------------------
FAILED

so the way our dracut stage drives dracut seems to not quite align with the way initoverlayfs-install expects it?

@ericcurtin
Copy link
Contributor Author

I wrote a small "stage" test for this now that does an end-to-end type test (it will need tweaks but is a useful starting point) for the feature (see attached patch)

0002-test-add-stage-integration-test-for-initoverlayfs-fe.patch.txt

Unfortunately this fails during the creation of the image with an error that --no-hostonly is unknown:

org.osbuild.dracut: e30a0aaa9351c35b3a70d8529fe33de68265af135daef250517b34296953684c {
  "kernel": [
    "6.6.4-200.fc39.x86_64"
  ],
  "initoverlayfs": true
}
Building initramfs for 6.6.4-200.fc39.x86_64
+ [[ 8 -gt 0 ]]
+ case $1 in
+ echo 'Unknown option --no-hostonly'
Unknown option --no-hostonly
+ exit 1
Traceback (most recent call last):
  File "/run/osbuild/bin/org.osbuild.dracut", line 228, in <module>
    r = main(args["tree"], args["options"])
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/run/osbuild/bin/org.osbuild.dracut", line 217, in main
    subprocess.run(["/usr/sbin/chroot", tree, bin,
  File "/usr/lib64/python3.12/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['/usr/sbin/chroot', '/run/osbuild/tree', '/usr/bin/initoverlayfs-install', '--no-hostonly', '--kver', '6.6.4-200.fc39.x86_64', '--force', '-v', '--show-modules', '--no-early-microcode', '--reproducible']' returned non-zero exit status 1.

⏱  Duration: 0s

-- END ---------------------------------
FAILED

so the way our dracut stage drives dracut seems to not quite align with the way initoverlayfs-install expects it?

This package, the Fedora 39 rpm, is not up to date with upstream, it's expected to fail.

@ericcurtin
Copy link
Contributor Author

We are probably not going to backport either. I would suggest merging this as it's blocking other MRs and add the test at a later point once the corresponding changes have propagated to Fedora.

This change will not break any existing osbuild scripts as initoverlayfs defaults to false.

@achilleas-k
Copy link
Member

This package, the Fedora 39 rpm, is not up to date with upstream

Does that mean it's already available in F39? We can just update the repos used in the test to newer snapshots (we made new ones on Friday).

@ericcurtin
Copy link
Contributor Author

ericcurtin commented Feb 19, 2024

This package, the Fedora 39 rpm, is not up to date with upstream

Does that mean it's already available in F39? We can just update the repos used in the test to newer snapshots (we made new ones on Friday).

Integration of initoverlayfs with Fedora will probably be a Fedora 41 kind of thing... But we am currently using it with osbuild via the Automotive SIG osbuild manifests (this PR is blocking that)...

@mvo5
Copy link
Contributor

mvo5 commented Feb 19, 2024

This package, the Fedora 39 rpm, is not up to date with upstream

Does that mean it's already available in F39? We can just update the repos used in the test to newer snapshots (we made new ones on Friday).

Integration of initoverlayfs with Fedora will probably be a Fedora 41 kind of thing... But we am currently using it with osbuild via the Automotive SIG osbuild manifests (this PR is blocking that)...

Thanks for adding this background. I think it would be nice to add https://github.com/osbuild/osbuild/files/14329239/0002-test-add-stage-integration-test-for-initoverlayfs-fe.patch.txt to the PR and then we can maybe add a comment in there that references back to this PR that there is a end-to-end test that could can be pulled in once the rpms are available. I am okay with merging this then but would love to get a quick double check from @ondrejbudai to see if that is okay.

@ondrejbudai
Copy link
Member

@mvo5 isn't the test doomed to fail given that --no-hostonly is not yet available in Fedora? Or am I missing something?

@ericcurtin
Copy link
Contributor Author

Yeah it would just fail, I don't see the harm in this MR, the option is defaulted to False, so people who are gonna use this have to explicitly set it to True (the people with the correct rpms)...

@mvo5
Copy link
Contributor

mvo5 commented Feb 20, 2024

@mvo5 isn't the test doomed to fail given that --no-hostonly is not yet available in Fedora? Or am I missing something?

Yeah, I was just wanted to get confirmation from someone more experienced that we are okay with merging this even though we don't have a full end-to-end test (yet) :)

@ondrejbudai
Copy link
Member

I think I'm fine with that given that this is a new technology, not yet available downstream. We can of course pull the package from somewhere else, but that feels expensive... I think that not blocking this initiative is more important to me. I agree though that this test SHOULD be added when the package lands in a "mainstream" repository.

ondrejbudai
ondrejbudai previously approved these changes Feb 20, 2024
@mvo5 mvo5 enabled auto-merge (rebase) February 20, 2024 17:04
As an alternative to just initramfs. Upstream initoverlayfs project:

https://github.com/containers/initoverlayfs

Signed-off-by: Eric Curtin <ecurtin@redhat.com>
auto-merge was automatically disabled February 20, 2024 17:21

Head branch was pushed to by a user without write access

@ericcurtin
Copy link
Contributor Author

I changed a variable name to initfs_bin , think pylint didn't like the bin variable name

@ondrejbudai ondrejbudai enabled auto-merge (rebase) February 21, 2024 10:26
@ondrejbudai ondrejbudai merged commit 134a4cc into osbuild:main Feb 21, 2024
122 checks passed
@ericcurtin ericcurtin deleted the initoverlayfs branch February 21, 2024 11:33
mvo5 added a commit to mvo5/osbuild that referenced this pull request Feb 21, 2024
Small followup for osbuild#1586
that includes a basic check that the initoverlayfs option calls
the right binary.
mvo5 added a commit to mvo5/osbuild that referenced this pull request Feb 21, 2024
Small followup for osbuild#1586
that includes a basic check that the initoverlayfs option calls
the right binary.
ondrejbudai pushed a commit that referenced this pull request Feb 22, 2024
Small followup for #1586
that includes a basic check that the initoverlayfs option calls
the right binary.
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.

None yet

4 participants