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

WIP: transient /etc #2970

Closed

Conversation

cgwalters
Copy link
Member

No description provided.

@openshift-ci
Copy link

openshift-ci bot commented Aug 4, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@cgwalters
Copy link
Member Author

This currently depends on #2958 (and changing how we build ostree images to include an empty /etc directory in addition to /usr/etc)

But, we can and probably should sever that dependency by detecting the missing etc and instead making the whole sysroot an overlayfs and making the etc directory there.

Copy link

@raballew raballew left a comment

Choose a reason for hiding this comment

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

But, we can and probably should sever that dependency by detecting the missing etc and instead making the whole sysroot an overlayfs and making the etc directory there.

At the moment we check if there is a lower etc dir, if true we mount an overlay to TMP_SYSROOT/etc. Is this what was meant with "making the etc directory there"?

The image boots without crashing but I did not have the time to investigate selinux policies or persisting of various files such as machine-id yet

}
else if (errno != ENOENT)
{
return glnx_throw_errno_prefix (error, "failed to stat etc");
Copy link

Choose a reason for hiding this comment

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

Suggested change
return glnx_throw_errno_prefix (error, "failed to stat etc");
return glnx_throw_errno_prefix (error, "failed to stat usr/etc");

return glnx_throw_errno_prefix (error, "failed to stat etc");
}
*out_path = NULL;
return TRUE;
Copy link

Choose a reason for hiding this comment

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

Suggested change
return TRUE;
return FALSE;

If there is no lower etc dir, shouldnt this function return false so that https://github.com/cgwalters/ostree/blob/c9a38dfb7fcebeb0b678d27ded78e92580be79a5/src/switchroot/ostree-prepare-root.c#L633-L637 evaluates to false?

      if (!find_etc (&etc_lower, &error))

Comment on lines +602 to +603
if (!ot_keyfile_get_value_with_default (config, SYSROOT_KEY, ETC_KEY, "persistent", &etc_config,
error))
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (!ot_keyfile_get_value_with_default (config, SYSROOT_KEY, ETC_KEY, "persistent", &etc_config,
error))
if (!ot_keyfile_get_value_with_default (config, SYSROOT_KEY, ETC_KEY, "persistent", &etc_config,
&error))

}

// Look for /usr/etc
if (fstatat (AT_FDCWD, "usr/etc") == 0)
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (fstatat (AT_FDCWD, "usr/etc") == 0)
if (!glnx_fstatat (AT_FDCWD, "usr/etc", &stbuf, AT_SYMLINK_NOFOLLOW, error))

Copy link

Choose a reason for hiding this comment

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

I am not entirely sure if I used the correct flag here.

find_etc (const char **out_path, GError **error)
{
// Look for /etc
if (fstatat (AT_FDCWD, "etc") == 0)
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (fstatat (AT_FDCWD, "etc") == 0)
if (!glnx_fstatat (AT_FDCWD, "etc", &stbuf, AT_SYMLINK_NOFOLLOW, error))

Copy link

Choose a reason for hiding this comment

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

I am not entirely sure if I used the correct flag here.

Copy link
Member

Choose a reason for hiding this comment

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

If using plain glnx_fstatat you need to use g_error_matches(error, ...), not errno. Easiest fix is to use
glnx_fstatat_allow_noent() which keeps errno for ENOENT.

static gboolean
find_etc (const char **out_path, GError **error)
{
// Look for /etc
Copy link

Choose a reason for hiding this comment

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

Suggested change
// Look for /etc
struct stat stbuf;
// Look for /etc

Comment on lines +637 to +641
if (etc_lower)
{
g_autofree char etc_ovl_options
= g_strdup_printf ("lowerdir=%s,upperdir=%s,workdir=%s", etc_lower, upper, work);
if (g_str_equal (""))
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (etc_lower)
{
g_autofree char etc_ovl_options
= g_strdup_printf ("lowerdir=%s,upperdir=%s,workdir=%s", etc_lower, upper, work);
if (g_str_equal (""))
g_autofree char *etc_ovl_options
= g_strdup_printf ("lowerdir=%s,upperdir=%s,workdir=%s", etc_lower, upper, work);
if (mount ("overlay", TMP_SYSROOT "/etc", "overlay", MS_SILENT, etc_ovl_options) < 0)
err (EXIT_FAILURE, "failed to mount transient etc overlayfs");

if (!ot_keyfile_get_value_with_default (config, SYSROOT_KEY, ETC_KEY, "persistent", &etc_config,
error))
errx (EXIT_FAILURE, "failed to parse %s.%s: %s", SYSROOT_KEY, ETC_KEY, error->message);
bool etc_transient = FALSE;
Copy link
Member

Choose a reason for hiding this comment

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

Weird mix of FALSE and false/true.

/* Do we have a persistent overlayfs for /usr? If so, mount it now. */
g_autofree char *etc_ovldir
= g_build_filename (OTCORE_RUN_OSTREE_PRIVATE, "etc-transient", NULL);
if (mkdirat (AT_FDCWD, etc_ovldir, 0700) < 0)
Copy link
Member

Choose a reason for hiding this comment

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

Did you boot test this? Because when I tried something like this the selinux context on the initramfs at this point was uninitialized, which was carried over into boot for the overlayfs lowerdir, and without etc_t selinux context on /tmp a bunch of stuff broke at boot.

@raballew raballew mentioned this pull request Aug 8, 2023
@cgwalters
Copy link
Member Author

Closing in favor of #2972

@cgwalters cgwalters closed this Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants