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

Add Xfs package to Bottlerocket #3198

Merged
merged 4 commits into from
Jul 13, 2023
Merged

Conversation

yeazelm
Copy link
Contributor

@yeazelm yeazelm commented Jun 13, 2023

Issue number: 3036

Description of changes:
This adds xfsprogs and it's dependencies to Bottlerocket. There is more work to properly make the variant boundary work but I have a rough set of changes to force the data partition into xfs.

Note: We may hold back the aws-dev variant until we feel we are ready but for the time being, it simply makes the tools available, the data partition still remains ext4 for the time being.

Testing done:
I build my own variant with xfs hard coded into the data partition. It boots, resizes, and you can see xfs data with xfs_* tools:

bash-5.1# xfs_info /local
meta-data=/dev/nvme1n1p1         isize=512    agcount=401, agsize=130816 blks
         =                       sectsz=4096  attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=1    bigtime=1 inobtcount=1
data     =                       bsize=4096   blocks=52428288, imaxpct=25
         =                       sunit=128    swidth=128 blks
naming   =version 2              bsize=16384  ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=16384, version=2
         =                       sectsz=4096  sunit=1 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

packages/libinih/Cargo.toml Show resolved Hide resolved
packages/libinih/libinih.spec Outdated Show resolved Hide resolved
packages/libinih/libinih.spec Outdated Show resolved Hide resolved
packages/libuserspace_rcu/Cargo.toml Outdated Show resolved Hide resolved
packages/libuserspace_rcu/libuserspace_rcu.spec Outdated Show resolved Hide resolved
packages/xfsprogs/xfsprogs.spec Outdated Show resolved Hide resolved
packages/xfsprogs/xfsprogs.spec Outdated Show resolved Hide resolved
packages/xfsprogs/xfsprogs.spec Outdated Show resolved Hide resolved
packages/xfsprogs/xfsprogs.spec Outdated Show resolved Hide resolved
packages/xfsprogs/xfsprogs.spec Outdated Show resolved Hide resolved
@yeazelm yeazelm force-pushed the xfs_package branch 5 times, most recently from 0fc23bf to 96d0d17 Compare June 21, 2023 20:28
@yeazelm yeazelm marked this pull request as ready for review June 21, 2023 21:21
packages/libinih/Cargo.toml Outdated Show resolved Hide resolved
packages/liburcu/liburcu.spec Outdated Show resolved Hide resolved
packages/xfsprogs/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

lgtm

variants/aws-dev/Cargo.toml Outdated Show resolved Hide resolved
packages/liburcu/liburcu.spec Outdated Show resolved Hide resolved
variants/aws-dev/Cargo.toml Outdated Show resolved Hide resolved
packages/xfsprogs/xfsprogs.spec Outdated Show resolved Hide resolved
packages/liburcu/liburcu.spec Outdated Show resolved Hide resolved
packages/liburcu/liburcu.spec Outdated Show resolved Hide resolved
packages/libinih/libinih.spec Outdated Show resolved Hide resolved
packages/libinih/libinih.spec Outdated Show resolved Hide resolved
variants/Cargo.lock Outdated Show resolved Hide resolved
packages/xfsprogs/xfsprogs.spec Outdated Show resolved Hide resolved
@yeazelm yeazelm force-pushed the xfs_package branch 2 times, most recently from 650ec5b to 0687157 Compare July 6, 2023 15:17
@yeazelm yeazelm requested a review from arnaldo2792 July 6, 2023 15:18
@@ -102,6 +103,7 @@ RUN rpmdev-setuptree \
&& echo -e -n "${GRUB_SET_PRIVATE_VAR:+%bcond_without grub_set_private_var\n}" >> .bconds \
&& echo -e -n "${SYSTEMD_NETWORKD:+%bcond_without systemd_networkd\n}" >> .bconds \
&& echo -e -n "${UNIFIED_CGROUP_HIERARCHY:+%bcond_without unified_cgroup_hierarchy\n}" >> .bconds \
&& echo -e -n "${XFS_DATA_PARTITION:+%bcond_without xfs_data_partition\n}" >> .bconds \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this for if we need to do conditional specfile things. I didn't end up adding xfsprogs conditionally to the image based upon this but we could if we want. If we don't see the need for this I can remove it as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be something more generic? Like DATA_PARTITION_FORMAT=<the-format>? Wouldn't OOTB benefit from this? Say I'm a weird developer that wants to use btrfs for my data partition in my OOT variant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't end up adding xfsprogs conditionally to the image based upon this but we could if we want.

It's fine to add the bcond support here even if we aren't using it, just for consistency. I did the same in #3097.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm having a hard time replying to the other threads about @arnaldo2792's comment:

Should it be something more generic? Like DATA_PARTITION_FORMAT=? Wouldn't OOTB benefit from this? Say I'm a weird developer that wants to use btrfs for my data partition in my OOT variant.

I did look at trying to make it work this way and discovered what @bcressey called out in his comment about image-layout. Since image-features are booleans these lend themselves as something like XFS_DATA_PARTITION vs DATA_PARTITION_FILESYSTEM or similar. As @bcressey mentioned, we probably would need to shift to using either image-layout (which I feel is a little awkward) or adding in yet another data structure for this. I actually like the idea of leaving the door open for other filesystems in the future but also feel like it might be over-indexing on something we have no concrete plans to do. I can shift to that approach if we think it's useful for OOTB work ensuring we have this flexibility in the future. Ultimately though, my opinion is that there is a lot of other work involved in adding something like btrfs and adding in another filesystem would be the right time to solve this more expansively but I could be overruled if that doesn't align with other features.

tools/rpm2img Outdated Show resolved Hide resolved
packages/liburcu/liburcu.spec Outdated Show resolved Hide resolved
make DIST_ROOT=%{buildroot} install install-dev \
PKG_ROOT_SBIN_DIR=%{_cross_sbindir} PKG_ROOT_LIB_DIR=%{_cross_libdir}

rm -f %{buildroot}/%{_cross_libdir}/*.{la,a}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as elsewhere, is there a flag you can use to disable compiling static libraries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see a specific flag for it for xfsprogs. Nonetheless, is the approach Fedora takes.

@@ -102,6 +103,7 @@ RUN rpmdev-setuptree \
&& echo -e -n "${GRUB_SET_PRIVATE_VAR:+%bcond_without grub_set_private_var\n}" >> .bconds \
&& echo -e -n "${SYSTEMD_NETWORKD:+%bcond_without systemd_networkd\n}" >> .bconds \
&& echo -e -n "${UNIFIED_CGROUP_HIERARCHY:+%bcond_without unified_cgroup_hierarchy\n}" >> .bconds \
&& echo -e -n "${XFS_DATA_PARTITION:+%bcond_without xfs_data_partition\n}" >> .bconds \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be something more generic? Like DATA_PARTITION_FORMAT=<the-format>? Wouldn't OOTB benefit from this? Say I'm a weird developer that wants to use btrfs for my data partition in my OOT variant.

@@ -10,8 +10,9 @@ RefuseManualStop=true
[Service]
Type=oneshot

EnvironmentFile=/usr/lib/data-partition-filesystem
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of an environment variable, what if prepare-local-fs.service becomes an in file, and in the ExecStart directive you use DATA_PARTITION_FILESYSTEM to replace it with sed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See @bcressey's comment below but I think I'll convert to his approach where its an image-features.env and make this a bit more generic for any other features. This allows them to be a bit more consistent. I think I like the EnvironmentFile approach a bit more. Is there a reason you would prefer the in file and sed?

packages/libinih/libinih.spec Outdated Show resolved Hide resolved
packages/liburcu/liburcu.spec Outdated Show resolved Hide resolved
packages/release/prepare-local-fs.service Outdated Show resolved Hide resolved
packages/xfsprogs/xfsprogs.spec Outdated Show resolved Hide resolved
packages/xfsprogs/xfsprogs.spec Outdated Show resolved Hide resolved
packages/xfsprogs/xfsprogs.spec Outdated Show resolved Hide resolved
packages/xfsprogs/Cargo.toml Outdated Show resolved Hide resolved
tools/buildsys/src/manifest.rs Outdated Show resolved Hide resolved
tools/rpm2img Outdated Show resolved Hide resolved
@bcressey
Copy link
Contributor

I can't seem to respond to @arnaldo2792 's question in the normal way:

Should it be something more generic? Like DATA_PARTITION_FORMAT=? Wouldn't OOTB benefit from this? Say I'm a weird developer that wants to use btrfs for my data partition in my OOT variant.

If we really want to do this, we can make data-image-filesystem a field in the image-layout struct (instead of a feature flag), and default it to "ext4".

inih is a library needed for programs like xfsprogs and provides a
small dependency for ini parsing.

Signed-off-by: Matthew Yeazel <[email protected]>
liburcu aka userspace-rcu is used for xfsprogs. This provides the libraries
needed to work with xfsprogs.

Signed-off-by: Matthew Yeazel <[email protected]>
Adding xfsprogs for manipulation of XFS filesystems on Bottlerocket.

Signed-off-by: Matthew Yeazel <[email protected]>
Add the ability to specify XFS for a variant's DATA partition. The
variants now allow xfs-data-partition to be set to enable this feature.
If not specified, the DATA partition defaults to ext4.

Signed-off-by: Matthew Yeazel <[email protected]>
tools/rpm2img Show resolved Hide resolved
@yeazelm yeazelm merged commit bcbc16d into bottlerocket-os:develop Jul 13, 2023
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.

5 participants