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

livefs: Require deployment staging #1456

Closed
wants to merge 2 commits into from

Conversation

cgwalters
Copy link
Member

Staging fixes the /etc bug for livefs. There's actually more
we could do here around taking advantage of staging for livefs;
for example, I think once the livefs is complete, we could just delete
the staged deployment. And then we don't need to render on the next
boot the live status, etc.

Anyways, all that can come in the future. This is prep for
enabling staging by default.

@cgwalters
Copy link
Member Author

Hmm, although...this means if we crash during a livefs, the next reboot will still go back into the partially-written livefs root.

I think the fix here is simple: when we push the rollback deployment, actually make it the default at first. To clarify things I'll call this the "livefs safety deployment".

So the flow would be:

  • clone current deployment root as livefs safety deployment, make it default
  • create new deployment (package installs, whatever) as staged

If we're interrupted here it's a bit weird, since we'll reboot into the livefs safety deployment. But eh.

  • livefs (and this can/should simultaneously consume staged deployment, e.g. rather than link files we can just move)
  • Now there are just two deployments; we do what's effectively rpm-ostree rollback to swap the bootloader order again, so the livefs safety is second

Optional enhancement: Don't prune the original rollback deployment here either; the livefs safety deployment is fully transient if everything is successful.

@ashcrow
Copy link
Member

ashcrow commented Jul 13, 2018

Both are failing due to:

FAIL (rc 1): layering-scripts
INFO: scheduled layering-unified on host vmcheck1
PASS: layering-relayer
INFO: scheduled livefs on host vmcheck2
FAIL (rc 1): livefs

@cgwalters
Copy link
Member Author

Hmm. I think the problem here is "leakage" of staging state across the current vmcheck suite - we have the STI approach which runs each test in its own VM.

@ashcrow
Copy link
Member

ashcrow commented Jul 13, 2018

FWIW the code change looks good to me :-)

@jlebon
Copy link
Member

jlebon commented Jul 18, 2018

Hmm, although...this means if we crash during a livefs, the next reboot will still go back into the partially-written livefs root.

Weren't we at some point talking about livefs not touching the bootloader at all? Ahh yes, that's in #802. That would solve this problem nicely. I'd be fine with not implementing the rollback dance until that's done and just go with what this patch is doing.

@jlebon
Copy link
Member

jlebon commented Jul 27, 2018

@rh-atomic-bot r+ 5e8479f

@rh-atomic-bot
Copy link

⌛ Testing commit 5e8479f with merge 7119e1d...

rh-atomic-bot pushed a commit that referenced this pull request Jul 27, 2018
Staging fixes the `/etc` bug for livefs.  There's actually more
we could do here around taking advantage of staging for livefs;
for example, I think once the livefs is complete, we could just delete
the staged deployment.  And then we don't need to render on the next
boot the live status, etc.

Anyways, all that can come in the future.  This is prep for
enabling staging by default.

Closes: #1456
Approved by: jlebon
@rh-atomic-bot
Copy link

💥 Test timed out

@jlebon
Copy link
Member

jlebon commented Jul 30, 2018

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

⌛ Testing commit 5e8479f with merge c09be06...

rh-atomic-bot pushed a commit that referenced this pull request Jul 30, 2018
Staging fixes the `/etc` bug for livefs.  There's actually more
we could do here around taking advantage of staging for livefs;
for example, I think once the livefs is complete, we could just delete
the staged deployment.  And then we don't need to render on the next
boot the live status, etc.

Anyways, all that can come in the future.  This is prep for
enabling staging by default.

Closes: #1456
Approved by: jlebon
@jlebon
Copy link
Member

jlebon commented Jul 30, 2018

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

⌛ Testing commit 5e8479f with merge a524ce5...

rh-atomic-bot pushed a commit that referenced this pull request Jul 30, 2018
Staging fixes the `/etc` bug for livefs.  There's actually more
we could do here around taking advantage of staging for livefs;
for example, I think once the livefs is complete, we could just delete
the staged deployment.  And then we don't need to render on the next
boot the live status, etc.

Anyways, all that can come in the future.  This is prep for
enabling staging by default.

Closes: #1456
Approved by: jlebon
@rh-atomic-bot
Copy link

💔 Test failed - status-atomicjenkins

@jlebon
Copy link
Member

jlebon commented Jul 30, 2018

+ ssh -o User=root -o ControlMaster=auto -o ControlPath=/var/tmp/ssh-vmcheck3-1532972744476751317.sock -o ControlPersist=yes vmcheck3 env ASAN_OPTIONS=detect_leaks=false rpm-ostree ex livefs
error: livefs requires staged deployments

Hmm, looks like tests might need to be adjusted some more?

Staging fixes the `/etc` bug for livefs.  There's actually more
we could do here around taking advantage of staging for livefs;
for example, I think once the livefs is complete, we could just delete
the staged deployment.  And then we don't need to render on the next
boot the live status, etc.

Anyways, all that can come in the future.  This is prep for
enabling staging by default.
@cgwalters
Copy link
Member Author

Maybe it's simpler to have this one live with the #1430 commit - having this merged without that means that ex livefs will fail for people out of the box on clean systems and I'm sure there are some uses of that.

Closing this one.

@cgwalters cgwalters closed this Jul 30, 2018
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.

4 participants