-
Notifications
You must be signed in to change notification settings - Fork 305
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: Add concept of deployment "pinning" 📌 #1464
Conversation
any chance this could be extended to support the use case described in #1460 (comment) and coreos/rpm-ostree#577 ? |
It's not clear what you want there - something like a config file where you can change |
yes.
To me this is basically the same thing as saying "keep around X number of snapshots". For example on my desktop system I have btrfs snapshots and I essentially keep them all; I have lots of disk so why not? It would be great if I could configure ostree/rpm-ostree to do something similar so I can start moving my systems over to FAW.
Not sure what that means exactly. So instead of a deployment the user could pin X where X represents the number of deployments to keep? |
But do you e.g. snapshot The way I think of it is; keeping lots of snapshots locally doesn't seem to make sense because you can re-download the from the Internet on-demand. (And yes, that's an experience we need to consistently enable with rpm-ostree and packages). But if you still want it, I'm not opposed to adding more policies, but it seems orthogonal to this PR. |
In my case I do (i actually snapshot the entire filesystem), but I get that ostree isn't going to be exactly what I currently have. I've actually thought about writing a snapper plugin for ostree, like the dnf one, that will call snapper and ask it to make snapshots (btrfs, or thin LV snapshots) automatically on configured filesystems whenever an rpm-ostree operation happens. Haven't really got far on that idea, just a thought so far.
That is true, but for some reason I lean towards wanting that functionality. if I rebase from f27 to rawhide AND then layer one package my f27 deployment is gone. I'd like it to be a little easier to keep around old deployments than that. The pinning helps, but only works if you proactively think about it.
Thank you! I guess it's orthogonal, seemed like a slight variation of the same thing to me. |
I think we'll edit the "rebase instructions" page to include |
* for e.g. older versions of libostree unaware of pinning to GC the deployment. | ||
* | ||
* This function does nothing and returns successfully if the deployment | ||
* is already in the desired pinning state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since:
?
tests/test-admin-deploy-2.sh
Outdated
assert_n_pinned 2 | ||
os_repository_new_commit | ||
${CMD_PREFIX} ostree admin upgrade --os=testos | ||
assert_n_pinned 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's explicitly also check that the pinned deployments are still there? (I.e. just grep for checksums? Or maybe just checking that the deployment count went up to three.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah...we can do a lot of that but it requires reinventing a lot of the things that were in rpm-ostree. Deduplicating that is going to be up at some point.
We'll get test coverage the other way though when we wrap this in rpm-ostree and do some tests there.
src/ostree/ot-admin-builtin-pin.c
Outdated
|
||
/* ATTENTION: | ||
* Please remember to update the bash-completion script (bash/ostree) and | ||
* man page (man/ostree-admin-undeploy.xml) when changing the option list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale comment. Also doesn't look like we added bash completions for this, right?
Let's at least change it to a TODO
?
if (is_pinned == current_pin) | ||
return TRUE; | ||
|
||
g_autoptr(OstreeDeployment) deployment_clone = ostree_deployment_clone (deployment); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused here. Why do we need ostree_deployment_clone
? Can't we just ostree_deployment_get_origin
and modify that? (Actually, shouldn't we do that so that the sysroot deployment array is up to date?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the sysroot array should be immutable. That actually reveals a bug, we need to bump the mtime so e.g. rpm-ostree will do a reload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh got it, that makes sense.
This ensures that e.g. `rpm-ostreed` will get notified of the changes.
The `origin/unlocked` and `origin/override-commit` keys are examples of state that's really transient; we don't want to maintain them across upgrades. Right now there are bits for this in both `ostree admin upgrade` as well as in rpm-ostree. This new API will slightly clean up both cases, but it's really prep for adding a concept of deployment "pinning" that will live in the new `libostree-transient` group.
fa64622
to
cdce079
Compare
Now with another prep commit, and squashed a few fixups ⬆️ |
Example user story: Jane rebases her OS to a new major version N, and wants to keep around N-1 even after a few upgrades for a while so she can easily roll back. I plan to add `rpm-ostree rebase --pin` to opt-in to this for example. Builds on the new `libostree-transient` group to store pinning state there. Closes: ostreedev#1460
cdce079
to
4374dcf
Compare
@@ -399,6 +399,37 @@ _ostree_admin_os_init() { | |||
return 0 | |||
} | |||
|
|||
_ostree_admin_pin() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, we also need a pin
entry in _ostree_admin()
.
@@ -399,6 +399,37 @@ _ostree_admin_os_init() { | |||
return 0 | |||
} | |||
|
|||
_ostree_admin_pin() { | |||
local boolean_options=" | |||
$main_boolean_options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--unpin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly don't grasp bash completion, I don't care too much about it myself at least for ostree admin
. Let's not try to polish it for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There has to be someone out there who has implemented actually generating this stuff from the cli --help
, as is it's an enormous maintenance pain doubled by the fact that we need to wrap it in rpm-ostree
at least too.
Sure, that's fine with me. |
⚡ Test exempted: pull fully rebased and already tested. |
The `origin/unlocked` and `origin/override-commit` keys are examples of state that's really transient; we don't want to maintain them across upgrades. Right now there are bits for this in both `ostree admin upgrade` as well as in rpm-ostree. This new API will slightly clean up both cases, but it's really prep for adding a concept of deployment "pinning" that will live in the new `libostree-transient` group. Closes: #1464 Approved by: jlebon
Example user story: Jane rebases her OS to a new major version N, and wants to keep around N-1 even after a few upgrades for a while so she can easily roll back. I plan to add `rpm-ostree rebase --pin` to opt-in to this for example. Builds on the new `libostree-transient` group to store pinning state there. Closes: #1460 Closes: #1464 Approved by: jlebon
Implemented in libostree in ostreedev/ostree#1464 Let's display it - wrapping the command will come later. I also just noticed `rpmostree_syscore_filter_deployments()` at least is going to have to learn about pinning; will need to improve the test suite around this too.
Implemented in libostree in ostreedev/ostree#1464 Let's display it - wrapping the command will come later. I also just noticed `rpmostree_syscore_filter_deployments()` at least is going to have to learn about pinning; will need to improve the test suite around this too. Closes: #1292 Approved by: jlebon
Followup to: ostreedev/ostree#1464 Ideally, we'd delegate more logic around these things to libostree, but we're not there yet. This means e.g. `rpm-ostree cleanup -r` for a pinned rollback will just silently skip it. It'd be nicer to emit an error probably, but that'd be quite a bit more work. Closes: coreos#1293
Followup to: ostreedev/ostree#1464 Ideally, we'd delegate more logic around these things to libostree, but we're not there yet. This means e.g. `rpm-ostree cleanup -r` for a pinned rollback will just silently skip it. It'd be nicer to emit an error probably, but that'd be quite a bit more work. Closes: #1293 Closes: #1309 Approved by: jlebon
See commits.
Also has a prep commit for transient state which I plan to use for "pending" state AKA #545