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

Sysroot auto cleanup #2511

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Makefile-boot.am
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ systemdsystemunit_DATA = src/boot/ostree-prepare-root.service \
src/boot/ostree-remount.service \
src/boot/ostree-finalize-staged.service \
src/boot/ostree-finalize-staged.path \
src/boot/ostree-auto-cleanup.service \
$(NULL)
systemdtmpfilesdir = $(prefix)/lib/tmpfiles.d
dist_systemdtmpfiles_DATA = src/boot/ostree-tmpfiles.conf
Expand Down Expand Up @@ -68,6 +69,7 @@ EXTRA_DIST += src/boot/dracut/module-setup.sh \
src/boot/ostree-finalize-staged.path \
src/boot/ostree-remount.service \
src/boot/ostree-finalize-staged.service \
src/boot/ostree-auto-cleanup.service \
src/boot/grub2/grub2-15_ostree \
src/boot/grub2/ostree-grub-generator \
$(NULL)
6 changes: 3 additions & 3 deletions Makefile-libostree.am
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,9 @@ endif # USE_GPGME
symbol_files = $(top_srcdir)/src/libostree/libostree-released.sym

# Uncomment this include when adding new development symbols.
# if BUILDOPT_IS_DEVEL_BUILD
# symbol_files += $(top_srcdir)/src/libostree/libostree-devel.sym
# endif
if BUILDOPT_IS_DEVEL_BUILD
symbol_files += $(top_srcdir)/src/libostree/libostree-devel.sym
endif

# http://blog.jgc.org/2007/06/escaping-comma-and-space-in-gnu-make.html
wl_versionscript_arg = -Wl,--version-script=
Expand Down
1 change: 1 addition & 0 deletions apidoc/ostree-sections.txt
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,7 @@ ostree_sysroot_get_deployment_directory
ostree_sysroot_get_deployment_dirpath
ostree_sysroot_get_deployment_origin_path
ostree_sysroot_cleanup
ostree_sysroot_auto_cleanup
ostree_sysroot_prepare_cleanup
ostree_sysroot_cleanup_prune_repo
ostree_sysroot_repo
Expand Down
25 changes: 22 additions & 3 deletions man/ostree-admin-cleanup.xml
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ License along with this library. If not, see <https://www.gnu.org/licenses/>.
</refnamediv>

<refsynopsisdiv>
<cmdsynopsis>
<command>ostree admin cleanup </command>
</cmdsynopsis>
<cmdsynopsis>
<command>ostree admin cleanup <arg choice="opt" rep="repeat">OPTIONS</arg></command>
</cmdsynopsis>
</refsynopsisdiv>

<refsect1>
Expand All @@ -61,6 +61,25 @@ License along with this library. If not, see <https://www.gnu.org/licenses/>.
</para>
</refsect1>

<refsect1>
<title>Options</title>

<variablelist>
<varlistentry>
<term><option>--auto</option></term>

<listitem><para>
Cleanup the sysroot only when needed. Whether to
cleanup or not is determined by the presence of the
<filename>.cleanup</filename> file in the sysroot.
When the <filename>.cleanup</filename> file exists,
the sysroot will be cleaned and the file will be
removed if it is successful.
</para></listitem>
</varlistentry>
</variablelist>
</refsect1>

<refsect1>
<title>Example</title>
<para><command>$ ostree admin cleanup</command></para>
Expand Down
43 changes: 43 additions & 0 deletions src/boot/ostree-auto-cleanup.service
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Copyright © 2022 Endless OS Foundation LLC
#
# This library is free software; you can redistribute it and/or
# modify it under the terms of the GNU Lesser General Public
# License as published by the Free Software Foundation; either
# version 2 of the License, or (at your option) any later version.
#
# This library is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
# Lesser General Public License for more details.
#
# You should have received a copy of the GNU Lesser General Public
# License along with this library. If not, see <https://www.gnu.org/licenses/>.

[Unit]
Description=OSTree Automatic Sysroot Cleanup
Documentation=man:ostree-admin-cleanup(1)
ConditionPathExists=/sysroot/.cleanup
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, we do something similar, but with the condition inverted:

# This is deleted by deploy.sh:
ConditionPathExists=!/sysroot/.ostree-cleaned

This made migration easier. If you're upgrading ostree with the deploy then the old version of ostree won't know to create this file, so when booting into the new deployment cleaning won't run.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a nice property. However, doesn't that mean you're running a prune on every boot, even if you haven't made a new deployment?

Copy link
Member

Choose a reason for hiding this comment

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

No, we delete .ostree-cleaned only just before ostree admin deploy.


# We want this to be triggered by multi-user.target but not block it via
# the default After added to target units since pruning the repo can be
# slow. See the Default Dependencies sections in systemd.service(5) and
# systemd.target(5).
DefaultDependencies=no
Requires=sysinit.target
After=sysinit.target basic.target
Conflicts=shutdown.target
Before=shutdown.target

[Service]
Type=oneshot
RemainAfterExit=yes
ExecStop=/usr/bin/ostree admin cleanup --auto
ProtectSystem=strict
ReadWritePaths=/sysroot /boot
Comment on lines +35 to +36
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I like this. But don't we also have to touch /var and/or /etc for the .updated files? IIRC I hit that last time trying to restrict things.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that happens when finalizing a deployment, not when cleaning up. See the comments in ostree-finalize-staged.service. I tested this in a downstream way and it worked fine.

Over in endlessm/eos-updater#298 @wjt pointed out that having /sysroot writable means essentially the whole system is writable. One thing that could be nicer would be to limit this whole exercise to just pruning since finalizing also does ostree_sysroot_prepare_cleanup. Then you could limit the writable paths to just /sysroot/repo.


# This will be allowed to run in the background, so try to make it less
# disruptive while it prunes the repo.
IOSchedulingClass=idle

[Install]
WantedBy=multi-user.target
9 changes: 7 additions & 2 deletions src/libostree/libostree-devel.sym
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,17 @@
- uncomment the include in Makefile-libostree.am
*/

LIBOSTREE_2022.2 {
global:
ostree_sysroot_auto_cleanup;
Copy link
Member

Choose a reason for hiding this comment

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

New public API feels overkill for this if the expectation is that it will be invoked by a separate systemd unit. Related below:

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this was with an eye toward having the systemd unit disabled and letting the management daemon handle it itself. But I did do this in endlessm/eos-updater#298 without needing the API. I do think it's cleaner to have the semantics contained within libostree, though.

} LIBOSTREE_2021.5;

/* Stub section for the stable release *after* this development one; don't
* edit this other than to update the year. This is just a copy/paste
* source. Replace $LASTSTABLE with the last stable version, and $NEWVERSION
* with whatever the next version with new symbols will be.
LIBOSTREE_2021.$NEWVERSION {
LIBOSTREE_2022.$NEWVERSION {
global:
someostree_symbol_deleteme;
} LIBOSTREE_2021.$LASTSTABLE;
} LIBOSTREE_2022.$LASTSTABLE;
*/
2 changes: 2 additions & 0 deletions src/libostree/ostree-impl-system-generator.c
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@ require_internal_units (const char *normal_dir,
return FALSE;
if (symlinkat (SYSTEM_DATA_UNIT_PATH "/ostree-finalize-staged.path", normal_dir_dfd, "multi-user.target.wants/ostree-finalize-staged.path") < 0)
return glnx_throw_errno_prefix (error, "symlinkat");
if (symlinkat (SYSTEM_DATA_UNIT_PATH "/ostree-auto-cleanup.service", normal_dir_dfd, "multi-user.target.wants/ostree-auto-cleanup.service") < 0)
return glnx_throw_errno_prefix (error, "symlinkat");

return TRUE;
#else
Expand Down
42 changes: 42 additions & 0 deletions src/libostree/ostree-sysroot-cleanup.c
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,48 @@ ostree_sysroot_cleanup (OstreeSysroot *self,
return _ostree_sysroot_cleanup_internal (self, TRUE, cancellable, error);
}

/**
* ostree_sysroot_auto_cleanup:
* @self: Sysroot
* @out_cleaned: (out): Whether a cleanup was performed
* @cancellable: Cancellable
* @error: Error
*
* Cleanup @self when needed. Whether to cleanup or not is determined by
* the presence of the .cleanup file in the sysroot. When the .cleanup
* file exists, ostree_sysroot_cleanup() will be called and the file
* will be removed if it is successful.
*
* Locking: exclusive
* Since: 2022.2
*/
gboolean
ostree_sysroot_auto_cleanup (OstreeSysroot *self,
gboolean *out_cleaned,
GCancellable *cancellable,
GError **error)
{
struct stat stbuf;
if (!glnx_fstatat_allow_noent (self->sysroot_fd, _OSTREE_SYSROOT_AUTOCLEANUP, &stbuf, AT_SYMLINK_NOFOLLOW, error))
Copy link
Member

Choose a reason for hiding this comment

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

Since the systemd unit is already doing this, do we need a new API at all versus simply ExecStart=ostree admin cleanup?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe not. I think you could do this all in the CLI or via systemd unit settings. It felt cleaner to encapsulate it in libostree, but it's not necessary. I could go either way.

return FALSE;
if (errno == ENOENT)
{
g_debug ("Did not find %s in sysroot; skipping auto cleanup", _OSTREE_SYSROOT_AUTOCLEANUP);
return TRUE;
}

if (!ostree_sysroot_cleanup (self, cancellable, error))
return FALSE;

if (!ot_ensure_unlinked_at (self->sysroot_fd, _OSTREE_SYSROOT_AUTOCLEANUP, error))
return FALSE;

if (out_cleaned != NULL)
*out_cleaned = TRUE;

return TRUE;
}

/**
* ostree_sysroot_prepare_cleanup:
* @self: Sysroot
Expand Down
9 changes: 9 additions & 0 deletions src/libostree/ostree-sysroot-deploy.c
Original file line number Diff line number Diff line change
Expand Up @@ -3282,6 +3282,15 @@ _ostree_sysroot_finalize_staged (OstreeSysroot *self,
if (!ostree_sysroot_prepare_cleanup (self, cancellable, error))
return FALSE;

/* Touch the autocleanup file so the repo pruning can be run on the
* next boot, but ignore errors.
*/
glnx_autofd int autocleanup_fd =
openat (self->sysroot_fd, _OSTREE_SYSROOT_AUTOCLEANUP, O_CREAT | O_WRONLY | O_NOCTTY | O_CLOEXEC, 0644);
if (autocleanup_fd == -1)
ot_journal_print (LOG_WARNING, "Failed to create sysroot autocleanup file: %s",
g_strerror (errno));

return TRUE;
}

Expand Down
1 change: 1 addition & 0 deletions src/libostree/ostree-sysroot-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ struct OstreeSysroot {
};

#define OSTREE_SYSROOT_LOCKFILE "ostree/lock"
#define _OSTREE_SYSROOT_AUTOCLEANUP ".cleanup"
/* We keep some transient state in /run */
#define _OSTREE_SYSROOT_RUNSTATE_STAGED "/run/ostree/staged-deployment"
#define _OSTREE_SYSROOT_RUNSTATE_STAGED_LOCKED "/run/ostree/staged-deployment-locked"
Expand Down
6 changes: 6 additions & 0 deletions src/libostree/ostree-sysroot.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,12 @@ gboolean ostree_sysroot_cleanup (OstreeSysroot *self,
GCancellable *cancellable,
GError **error);

_OSTREE_PUBLIC
gboolean ostree_sysroot_auto_cleanup (OstreeSysroot *self,
gboolean *out_cleaned,
GCancellable *cancellable,
GError **error);

_OSTREE_PUBLIC
gboolean ostree_sysroot_prepare_cleanup (OstreeSysroot *self,
GCancellable *cancellable,
Expand Down
18 changes: 16 additions & 2 deletions src/ostree/ot-admin-builtin-cleanup.c
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@

#include <glib/gi18n.h>

static gboolean opt_auto;

static GOptionEntry options[] = {
{ "auto", 0, 0, G_OPTION_ARG_NONE, &opt_auto, "Cleanup sysroot when needed", NULL },
{ NULL }
};

Expand All @@ -43,8 +46,19 @@ ot_admin_builtin_cleanup (int argc, char **argv, OstreeCommandInvocation *invoca
invocation, &sysroot, cancellable, error))
return FALSE;

if (!ostree_sysroot_cleanup (sysroot, cancellable, error))
return FALSE;
if (opt_auto)
{
gboolean cleaned = FALSE;
if (!ostree_sysroot_auto_cleanup (sysroot, &cleaned, cancellable, error))
return FALSE;
if (!cleaned)
g_print ("No cleanup needed.\n");
}
else
{
if (!ostree_sysroot_cleanup (sysroot, cancellable, error))
return FALSE;
}

return TRUE;
}
4 changes: 3 additions & 1 deletion tests/inst/src/destructive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ fn upgrade_and_finalize() -> Result<()> {
bash!(
"rpm-ostree upgrade
systemctl start ostree-finalize-staged
systemctl stop ostree-finalize-staged"
systemctl stop ostree-finalize-staged
test -f /sysroot/.cleanup"
)
.context("Upgrade and finalize failed")?;
Ok(())
Expand Down Expand Up @@ -465,6 +466,7 @@ fn impl_transaction_test<M: AsRef<str>>(
systemctl stop ostree-finalize-staged
ostree reset testrepo:{testref} {booted_commit}
rpm-ostree cleanup -pbrm
rm -f /sysroot/.cleanup
",
testref,
booted_commit,
Expand Down
7 changes: 7 additions & 0 deletions tests/kolainst/destructive/staged-deploy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ case "${AUTOPKGTEST_REBOOT_MARK:-}" in
"")
# Test our generator
test -f /run/systemd/generator/multi-user.target.wants/ostree-finalize-staged.path
test -f /run/systemd/generator/multi-user.target.wants/ostree-auto-cleanup.service
test -f /run/systemd/generator/local-fs.target.requires/ostree-remount.service

cat >/etc/systemd/system/sock-to-ignore.socket << 'EOF'
Expand Down Expand Up @@ -77,6 +78,12 @@ EOF
rm -f svc.txt
# And there should not be a staged deployment
test '!' -f /run/ostree/staged-deployment
# Check that auto cleanup ran
assert_not_has_file /sysroot/.cleanup
journalctl -b 0 -u ostree-auto-cleanup.service > svc.txt
assert_file_has_content svc.txt 'Starting ostree-auto-cleanup.service'
assert_not_file_has_content svc.txt 'No cleanup needed.'
rm -f svc.txt

# Upgrade with staging
test '!' -f /run/ostree/staged-deployment
Expand Down
13 changes: 12 additions & 1 deletion tests/test-admin-deploy-2.sh
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ set -euo pipefail
# Exports OSTREE_SYSROOT so --sysroot not needed.
setup_os_repository "archive" "syslinux"

echo "1..8"
echo "1..9"

${CMD_PREFIX} ostree --repo=sysroot/ostree/repo pull-local --remote=testos testos-repo testos/buildmain/x86_64-runtime
rev=$(${CMD_PREFIX} ostree --repo=sysroot/ostree/repo rev-parse testos/buildmain/x86_64-runtime)
Expand Down Expand Up @@ -62,6 +62,17 @@ assert_file_has_content sysroot/ostree/deploy/testos/deploy/${newrev}.0/etc/os-r

echo "ok manual cleanup"

assert_not_has_file sysroot/.cleanup
${CMD_PREFIX} ostree admin cleanup --auto > cleanup.txt
assert_file_has_content cleanup.txt "No cleanup needed."
assert_not_has_file sysroot/.cleanup
touch sysroot/.cleanup
${CMD_PREFIX} ostree admin cleanup --auto > cleanup.txt
assert_not_file_has_content cleanup.txt "No cleanup needed."
assert_not_has_file sysroot/.cleanup

echo "ok auto cleanup"

# Commit + upgrade twice, so that we'll rotate out the original deployment
os_repository_new_commit "1"
${CMD_PREFIX} ostree admin upgrade --os=testos
Expand Down