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

rhel9: systemd service to create mountpoints for filesystem customizations for edge images #399

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

say-paul
Copy link
Member

Filesystem customization mountpoints does not persists any upgrade due to the property of ostree which only persists data on /var and /etc. This feature will ingest the filesystem customization and create a unit file which can create the required mountpoints if it doesnot exist.

pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved
@runcom
Copy link
Member

runcom commented Jan 24, 2024

this is a silly cross-check from me, have we made sure that data is properly persisted within these mountpoints?

@say-paul
Copy link
Member Author

this is a silly cross-check from me, have we made sure that data is properly persisted within these mountpoints?

I tested this out, data persists.

pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved
pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved
pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved
@achilleas-k
Copy link
Member

this is a silly cross-check from me, have we made sure that data is properly persisted within these mountpoints?

I tested this out, data persists.

@henrywang When this is added to composer, can we get a test for this added? We will need to add custom mountpoints to an edge image and then upgrade it, so I think it can be included in the existing test scripts.

@say-paul say-paul force-pushed the custom-service-to-create-mountpoints branch from da244b5 to 48e5ed9 Compare January 29, 2024 12:06
@say-paul say-paul marked this pull request as ready for review January 29, 2024 12:07
@say-paul say-paul force-pushed the custom-service-to-create-mountpoints branch from 48e5ed9 to bc4e8bc Compare January 29, 2024 12:08
7flying
7flying previously approved these changes Jan 29, 2024
Copy link
Member

@7flying 7flying left a comment

Choose a reason for hiding this comment

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

The changes that I requested were added, LGTM

@say-paul say-paul force-pushed the custom-service-to-create-mountpoints branch 2 times, most recently from 823900d to 925468b Compare January 29, 2024 17:39
@say-paul
Copy link
Member Author

The CI failure is expected as the, the unit condition fails on first boot, it runs when there is an upgrade when the mount-points gets deleted. Will be doing some manual testing on "edge-ami" to confirm the functionality.
@achilleas-k @thozza do you suggest removing the "edge-ami" as it will fail every time for first boot.

@say-paul say-paul force-pushed the custom-service-to-create-mountpoints branch 3 times, most recently from 9775def to 5fed2b5 Compare February 2, 2024 10:01
pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved
pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved
pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved
@say-paul say-paul force-pushed the custom-service-to-create-mountpoints branch from 5fed2b5 to 0244275 Compare February 5, 2024 10:20
cgwalters
cgwalters previously approved these changes Feb 5, 2024
Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I'm OK with this as is, we can't do better as far as I know without reworking how blueprints work for builds.

pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved
pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved
@7flying
Copy link
Member

7flying commented Feb 5, 2024

(We need to discuss a couple of things about this PR, please do not merge yet)

thozza
thozza previously requested changes Feb 5, 2024
Copy link
Member

@thozza thozza 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 for the fix.

Unfortunately, I'm not in favor of using *fsnode.File for this purpose. I propose to extend https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.systemd.unit and use it instead.

The rest looks OK to me.

In general, *fsnode.File is not intended as a workaround for not creating / enhancing osbuild stages, unless the problem is a special / corner case.

pkg/distro/rhel9/images.go Outdated Show resolved Hide resolved
@cgwalters
Copy link
Contributor

See also ostreedev/ostree#2681

@say-paul say-paul force-pushed the custom-service-to-create-mountpoints branch from 79f3af0 to 286eb7b Compare March 11, 2024 12:23
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

Looks good in general. The git history needs some work though.
The first commit adds a config to the config-map (ostree-filesystem-customizations-installer.json) that doesn't get added until the fourth commit.
Also the commit message in the fourth commit doesn't match the patch anymore. It mentions disabling the edge-ami boot test which it doesn't do.

@say-paul say-paul force-pushed the custom-service-to-create-mountpoints branch 2 times, most recently from a6d35ee to 9576748 Compare March 13, 2024 09:07
@say-paul say-paul dismissed achilleas-k’s stale review March 13, 2024 09:09

the commit history is cleaned up to make it in accordance to the changes made.

@say-paul say-paul force-pushed the custom-service-to-create-mountpoints branch 2 times, most recently from 28116f7 to b173fab Compare March 14, 2024 14:20
test/config-map.json Outdated Show resolved Hide resolved
pkg/manifest/ostree_deployment.go Outdated Show resolved Hide resolved
@say-paul say-paul force-pushed the custom-service-to-create-mountpoints branch from b173fab to 22bfa38 Compare March 14, 2024 19:11
@say-paul
Copy link
Member Author

blocked on : osbuild/osbuild#1666

@say-paul say-paul marked this pull request as draft March 15, 2024 10:53
@say-paul say-paul force-pushed the custom-service-to-create-mountpoints branch from 22bfa38 to 27265a1 Compare March 20, 2024 07:29
@say-paul say-paul marked this pull request as ready for review March 20, 2024 08:41
@say-paul say-paul dismissed achilleas-k’s stale review March 20, 2024 09:06

rhel version fixed and added service description

Filesystem customizations is enabled for edge-raw-image,
edge-ami,edge-vsphere, edge-simplified-installer
relevent testing config is added to run in CI

Signed-off-by: Sayan Paul <paul.sayan@gmail.com>
Create systemd unit using the system-unit-create stage
This new stage adds the ability to create a systemd service unit file.
It provides customization to choose the unit-type and unit-path, defaults to '/usr/lib/systemd/system/'
Filename is validated using the same rules as specified by systemd.unit(5) and they must contain the
'.service' suffix (other types of unit files are not supported). config accepts sections: `Unit`,
`Service`, `Install` and each section accepts list of options that is in accordance to systemd.service
documentation. Relavent testing is also added.

Signed-off-by: Sayan Paul <paul.sayan@gmail.com>
Hooked up the create-unit-stage to add create-mountpoint.service to
ostree deployment.
Automatically add and enable osbuild-create-mountpoint.service
whenever user adds filesystem customizationi, to ensure that the
mountpoints are available post rpm-ostree upgrade.

Signed-off-by: Sayan Paul <paul.sayan@gmail.com>
Signed-off-by: Sayan Paul <paul.sayan@gmail.com>
@bcl bcl force-pushed the custom-service-to-create-mountpoints branch from 27265a1 to 56c05f3 Compare March 21, 2024 00:18
@bcl bcl added this pull request to the merge queue Mar 21, 2024
Merged via the queue into osbuild:main with commit e00940b Mar 22, 2024
16 checks passed
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.

8 participants