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

switchroot: Allow letting ostree-prepare-root mount /var #1617

Closed
wants to merge 2 commits into from

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Jun 7, 2018

In some scenarios, it might make sense to let ostree-prepare-root do
the /var mount from the state root as before. For example, one may
want to do some system configuration before the switch root. This of
course comes at the expense of supporting /var as a mount point in
/etc/fstab.

In some scenarios, it might make sense to let `ostree-prepare-root` do
the `/var` mount from the state root as before. For example, one may
want to do some system configuration before the switch root. This of
course comes at the expense of supporting `/var` as a mount point in
`/etc/fstab`.
@@ -49,6 +49,13 @@ main(int argc, char *argv[])
exit (EXIT_SUCCESS);
}

/* We conflict with the magic ostree-mount-deployment-var file for ostree-prepare-root */
{ struct stat stbuf;
if (fstatat (AT_FDCWD, "/run/ostree-mount-deployment-var", &stbuf, 0) == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe be nice and unlinkat() here in the interest of having /run be less littered. Or in addition/alternatively, make the file /run/ostree/initramfs-mount-var since we already made /run/ostree/ for the deployment staging bits.

if (fstatat (AT_FDCWD, "/run/ostree-mount-deployment-var", &stbuf, 0) == 0)
exit (EXIT_SUCCESS);
}

Copy link
Member

Choose a reason for hiding this comment

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

Extra newline.

@jlebon
Copy link
Member Author

jlebon commented Jun 7, 2018

Added fixup! I also tested this successfully now.

@jlebon jlebon changed the title WIP: switchroot: Allow letting ostree-prepare-root mount /var switchroot: Allow letting ostree-prepare-root mount /var Jun 7, 2018
@jlebon
Copy link
Member Author

jlebon commented Jun 7, 2018

Hmm, it's not clear why FAH28-insttests failed:

PLAY RECAP *********************************************************************
/srv/code/tests/installed/fedora-atomic-host.qcow2 : ok=18   changed=8    unreachable=0    failed=0   


real	7m12.971s
user	4m35.846s
sys	3m8.840s
### EXITED WITH CODE 1 AFTER 1842s

@@ -49,6 +49,16 @@ main(int argc, char *argv[])
exit (EXIT_SUCCESS);
}

/* We conflict with the magic ostree-mount-deployment-var file for ostree-prepare-root */
{ struct stat stbuf;
if (fstatat (AT_FDCWD, INITRAMFS_MOUNT_VAR, &stbuf, 0) == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Could just do if (unlinkat (...) == 0) but eh.

@cgwalters
Copy link
Member

The insttest has been flaking a bit in PRs, will look at adding some debug info. Let's see if it bounces.

@rh-atomic-bot r+ b14c3ee

@rh-atomic-bot
Copy link

⌛ Testing commit b14c3ee with merge ecdebeb...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing ecdebeb to master...

@jlebon jlebon deleted the pr/mount-magic branch June 14, 2018 01:51
#ifndef HAVE_SYSTEMD_AND_LIBMOUNT
/* file in /run can override that behaviour */
if (lstat ("/run/ostree-mount-deployment-var", &stbuf) < 0)
mount_var = false;
Copy link
Member

Choose a reason for hiding this comment

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

@jlebon Isn't the logic backwards here? We should default mount_var = false, and set it true here?

Got a report on IRC that /var in /etc/fstab broke after a silverblue update.

Copy link
Member

Choose a reason for hiding this comment

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

I also think we need to have this bit outside of the #ifndef HAVE_SYSTEMD_AND_LIBMOUNT right? I'm not sure how this could have worked as we're building Fedora with that conditional set right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ouch, yes, you're correct this is the exact opposite. I think the double negation with #ifndef confused me. Fixup coming up!

Copy link
Member

Choose a reason for hiding this comment

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

Split this out into #1667

jlebon added a commit to jlebon/ostree that referenced this pull request Jul 3, 2018
I made a logical error in ostreedev#1617 which resulted in the exact *opposite*
behaviour we want when `/var` is a separate mount.

Split this out and lower the number of negations to make it more obvious
that it's correct.

Closes: ostreedev#1667
jlebon added a commit to jlebon/ostree that referenced this pull request Jul 3, 2018
I made a logical error in ostreedev#1617 which resulted in the exact *opposite*
behaviour we want when `/var` is a separate mount.

Split this out and lower the number of negations to make it more obvious
that it's correct.

Closes: ostreedev#1667
rh-atomic-bot pushed a commit that referenced this pull request Jul 4, 2018
I made a logical error in #1617 which resulted in the exact *opposite*
behaviour we want when `/var` is a separate mount.

Split this out and lower the number of negations to make it more obvious
that it's correct.

Closes: #1667

Closes: #1668
Approved by: cgwalters
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.

None yet

3 participants