build-chunked-oci: Correctly label files #5502
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the logic for generating an ostree commit from a rootfs to correctly handle file labeling. The main change is to stop iterating through subdirectories and instead process the entire rootfs at once using repo.write_dfd_to_mtree, with a filter to exclude /sysroot content. This fixes an issue where files in subdirectories were being labeled with incorrect paths. The implementation is cleaner and the test-specific logic is now handled via a no_attrs boolean flag in generate_commit_from_rootfs, which is a good simplification. My review includes one minor suggestion to improve code style by marking an unused parameter.
alexlarsson
force-pushed
the
chunked-oci-relabeling-fix
branch
from
b39b890 to
b98ce81
Compare
|
Both this and #5497 fails with the same issues, so this seem unrelated to these changes. |
|
Yeah, we need to consistently clean up the GHA runner to free up disk space. I started on this with #5504 |
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Ouch, the mislabeling thing is a fairly bad bug that's just been effectively masked by bootc always doing the client side relabeling.
I think this looks sane...but we're really going to need to add some tests here. I can help look at that; basically "unit-style integration tests" for build-chunked-oci that verify the output without booting it. But also a full e2e workflow that boots from a chunking output.
|
I'm a bit busy with other stuff, but will have time to look at the test for this later in the week. |
Running repo.write_dfd_to_mtree() on the subdirs individually will cause the modifier to label files incorrectly. For example when calling it on the "etc" subdirectory, it will try to label /etc/foo as /foo. Instead we relabel the entire image, but use a filter to strip out /sysroot content. This is slightly complicated by the tests also wanting to use a modifiers, so we add the no_attr argument to generate_commit_from_rootfs() that does what the tests need.
alexlarsson
force-pushed
the
chunked-oci-relabeling-fix
branch
from
b98ce81 to
44aa74a
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
alexlarsson
force-pushed
the
chunked-oci-relabeling-fix
branch
from
44aa74a to
74ee76a
Compare
…d image This ensures that basic bootc re-chunked images boot and contain the layered files.
This ensures all intermediate images are removed to save disk space. I tested running this and there are no leftover images afterward. This is nice because we have had some out-of-diskspace issues in the CI system when running this.
alexlarsson
force-pushed
the
chunked-oci-relabeling-fix
branch
3 times, most recently
from
26d8282 to
16e14e7
Compare
This validates that chunking and image ends up with the expected files, and that they have the correct selinux labels.
alexlarsson
force-pushed
the
chunked-oci-relabeling-fix
branch
from
16e14e7 to
3d3e30c
Compare
|
@alexlarsson: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
NOTE: I split this out of #5497 to see what is causing the issues. These are pretty independent anyway.
Running repo.write_dfd_to_mtree() on the subdirs individually will cause the modifier to label files incorrectly. For example when calling it on the "etc" subdirectory, it will try to label /etc/foo as /foo.
Instead we relabel the entire image, but use a filter to strip out /sysroot content.
This is slightly complicated by the tests also wanting to use a modifiers, so we add the no_attr argument to
generate_commit_from_rootfs() that does what the tests need.