-
Notifications
You must be signed in to change notification settings - Fork 290
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
Add concept of "staged" deployment #1503
Conversation
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.
Only did a high-level overview so far, but looks nice! 👍
* ostree_sysroot_stage_tree() API. | ||
*/ | ||
gboolean | ||
ostree_sysroot_deploy_tree (OstreeSysroot *self, |
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 start by splitting out this initialize
+ finalize
separation in a prep PR?
☔ The latest upstream changes (presumably 01717d7) made this pull request unmergeable. Please resolve the merge conflicts. |
49edd1e
to
c68f28f
Compare
💥 Invalid |
☔ The latest upstream changes (presumably 22cd178) made this pull request unmergeable. Please resolve the merge conflicts. |
3109757
to
feca528
Compare
TODO:
|
☔ The latest upstream changes (presumably 1f3f657) made this pull request unmergeable. Please resolve the merge conflicts. |
feca528
to
009dec7
Compare
☔ The latest upstream changes (presumably 7ec3d06) made this pull request unmergeable. Please resolve the merge conflicts. |
08cb290
to
e2838e7
Compare
e2838e7
to
cda2b06
Compare
Sigh, needed to move it into the main |
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.
Looks really nice, learned a lot from just reading it :D. Some minor comments and questions :p.
@@ -2388,6 +2398,7 @@ ostree_sysroot_deploy_tree (OstreeSysroot *self, | |||
return FALSE; | |||
|
|||
_ostree_deployment_set_bootcsum (new_deployment, kernel_layout->bootcsum); | |||
_ostree_deployment_set_bootconfig_from_kargs (new_deployment, override_kernel_argv); |
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 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.
Yep, that was all duplicate code. Fixed, thanks!
cancellable, error)) | ||
return FALSE; | ||
} | ||
|
||
/* Don't fsync here, as we assume that's all done in | ||
* ostree_sysroot_write_deployments(). |
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.
Minor: do we still want to keep this comment for non-staged case? /me assuming we are still supporting "non-staged" deployments (the old 3 way merge way)?
g_variant_builder_add (builder, "{sv}", "kargs", | ||
g_variant_new_strv ((const char *const*)override_kernel_argv, -1)); | ||
|
||
char *dnbuf = strdupa (_OSTREE_SYSROOT_RUNSTATE_STAGED); |
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.
Question: I am not too familiar with "/run" directories, wondering any reasons / benefits to store data there? :-) /me assuming we are writing deployment info into that location.
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.
The basic idea of /run
is it's guaranteed to go away if the system reboots (it's a tmpfs
, stored only in memory). Which I think helps simplify the logic around failure cases. I feel like we only want to try to process staging data once rather than having it be persistent.
GCancellable *cancellable, | ||
GError **error) | ||
{ | ||
/* The service has a ConditionPathExists=/run/ostree/staged-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.
Minor: seems like /run/ostree/staged-deployment
is not there in the service file. Only ConditionPathExists=/run/ostree-booted
seems to be there?
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.
That's not minor, that's a bug! The service would fail if there was no staged deployments. Fixed.
|
||
/* TODO: Proxy across flags too? */ | ||
OstreeSysrootSimpleWriteDeploymentFlags flags = 0; | ||
if (!ostree_sysroot_simple_write_deployment (self, ostree_deployment_get_osname (self->staged_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.
Question/minor: noticed in rpm-ostree, seems like we had rpmostree_syscore_write_deployment
to add some flags for livefs and rollback, do we need to consider a bit here to make it easier to integrate this into rpm-ostree in the future?(if needed :p)
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 think you're right that integrating this nicely will require pulling down some of that logic.
if (self->staged_deployment) | ||
{ | ||
char *deployment_path = ostree_sysroot_get_deployment_dirpath (self, self->staged_deployment); | ||
g_hash_table_replace (active_deployment_dirs, deployment_path, deployment_path); |
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.
Question: wanted to confirm, we don't include bootcsum here because the "staged deployment" is not ready for boot yet? ( i.e the merge and boot entries weren't ready)
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.
Nope, that was an oversight. One not revealed because we weren't installing a staged deployment with a new boot checksum.
I have kind of been going back and forth on whether /boot
should be modified during staging or finalization. Currently it's during staging.
cda2b06
to
164481d
Compare
a81672c
to
f56d41d
Compare
# https://lists.freedesktop.org/archives/systemd-devel/2018-March/040557.html | ||
[Unit] | ||
Description=OSTree Deploy Staged | ||
ConditionPathExists=/run/ostree-booted |
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 see we do tolerate no staged deployments in _ostree_sysroot_deploy_staged
, though we might as well short-circuit this from here and add a ConditionPathExists=/run/ostree/staged-deployment
here too, no? Making it explicit also makes it easier to discover how these pieces fit together.
/after reading more code
Oh right, this won't work with the multi-user.target
approach since it's evaluated on startup. Though it should work if we use shutdown.target
, no? (Related to next comment).
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, can't do that, see https://lists.freedesktop.org/archives/systemd-devel/2018-March/040557.html
Makefile-boot.am
Outdated
@@ -39,7 +39,7 @@ endif | |||
|
|||
if BUILDOPT_SYSTEMD | |||
systemdsystemunit_DATA = src/boot/ostree-prepare-root.service \ | |||
src/boot/ostree-remount.service | |||
src/boot/ostree-remount.service src/boot/ostree-deploy-staged.service |
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.
This is slightly bikeshed, though it feels like it would be more appropriate to name the service something like ostree-finalize-staged.service
. (And the private function _ostree_sysroot_finalize_staged
).
My reasoning is that a staged deployment is in some ways already deployed, right? And I see the rest of the new APIs/CLIs reflect that as well (e.g. ostree_sysroot_get_staged_deployment
, and ostree admin deploy --stage
).
[Service] | ||
Type=oneshot | ||
RemainAfterExit=yes | ||
ExecStop=/usr/bin/ostree admin deploy-staged |
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.
Hmm, this seems odd to me. I'm guessing the idea here is that the Before=multi-user.target
will ensure that most services will have been shut down, assuming services are stopped in reverse order. Is this the canonical way to run on shutdown? Why not WantedBy=shutdown.target
?
@@ -808,6 +809,7 @@ static gboolean | |||
write_origin_file_internal (OstreeSysroot *sysroot, | |||
OstreeSePolicy *sepolicy, | |||
OstreeDeployment *deployment, | |||
gboolean if_not_exists, |
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 why we need this. Why not just unconditionally overwrite it in finalize
? Then even in the stage path, we're sure of using the right label for it post-merge.
static GVariant * | ||
serialize_deployment_to_variant (OstreeDeployment *deployment) | ||
{ | ||
g_autoptr(GVariantBuilder) builder = g_variant_builder_new ((GVariantType*)"a{sv}"); |
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.
Should we stick with g_auto
where we can for good form?
g_autoptr(GVariantBuilder) builder = g_variant_builder_new ((GVariantType*)"a{sv}"); | ||
g_autofree char *name = | ||
g_strdup_printf ("%s.%d", ostree_deployment_get_csum (deployment), | ||
ostree_deployment_get_deployserial (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.
How about keeping those separate to make deconstruction lower down easier?
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.
Eh, we already need to parse them from the filesystem and had a function for it.
g_variant_new_strv ((const char *const*)override_kernel_argv, -1)); | ||
|
||
char *dnbuf = strdupa (_OSTREE_SYSROOT_RUNSTATE_STAGED); | ||
const char *parent = dirname (dnbuf); |
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.
Minor: any reason not to collapse these two lines? (i.e. dirname (strdupa (...))
)
g_autoptr(OstreeDeployment) deployment = NULL; | ||
if (!sysroot_initialize_deployment (self, osname, revision, origin, override_kernel_argv, | ||
&deployment, cancellable, error)) | ||
return FALSE; |
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.
Hmm, I wonder here if we should do something if there's a deployment already staged. E.g. in rpm-ostree, if we have a staged
auto-updates policy, we could just continuously be staging newer updates as we see them right? I guess we can put the cleanup there, though it does feel like someone calling ostree_sysroot_stage_tree
expects it to implicitly delete a previously staged deployment, rather than having them pile up.
if (self->ostree_booted && self->root_is_sysroot | ||
&& !self->booted_deployment) | ||
const gboolean root_is_ostree_booted = | ||
self->ostree_booted && self->root_is_sysroot; |
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.
At this point, we might as well make replace instances of those with a new field.
if (opt_retain) | ||
flags |= OSTREE_SYSROOT_SIMPLE_WRITE_DEPLOYMENT_FLAGS_RETAIN; | ||
else | ||
if (opt_stage) | ||
{ |
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.
Let's just error out here if any of the --retain
opts are given?
Pushed a fixup ⬆️ which addresses a lot of comments (but not all). In the meantime split out prep patches into #1535 |
☔ The latest upstream changes (presumably b9fc3ea) made this pull request unmergeable. Please resolve the merge conflicts. |
Add API to write a deployment state to `/run/ostree/staged-deployment`, along with a systemd service which runs at shutdown time. This is a big change to the ostree model for hosts, but it closes a longstanding set of bugs; many, many people have hit the "losing changes in /etc" problem. It also avoids the other problem of racing with programs that modify `/etc` such as LVM backups: https://bugzilla.redhat.com/show_bug.cgi?id=1365297 We need this in particular to go to a full-on model for automatically updated host systems where (like a dual-partition model) everything is fully prepared and the reboot can be taken asynchronously. Closes: ostreedev#545
0ba40db
to
7adfe24
Compare
Let's do this! |
☀️ Test successful - status-atomicjenkins |
Followup to: ostreedev#1503 After starting some more work on on this in rpm-ostree, it is actually simpler if the staged deployment just shows up in the list. It's effectively opt-in today; down the line we may make it the default, but I worry about breaking things that e.g. assume they can mutate the deployment before rebooting and have `/etc` already merged. There's not that many things in libostree that iterate over the deployment list. The biggest change here is around the `ostree_sysroot_write_deployments_with_options` API. I initially tried hard to support a use case like "push a rollback" while retaining the staged deployment, but everything gets very messy because that function truly is operating on the bootloader list. For now what I settled on is to just discard the staged deployment; down the line we can enhance things. Where we then have some new gymnastics is around implementing the finalization; we need to go to some effort to pull the staged deployment out of the list and mark it as unstaged, and then pass it down to `write_deployments()`.
Followup to: ostreedev#1503 After starting some more work on on this in rpm-ostree, it is actually simpler if the staged deployment just shows up in the list. It's effectively opt-in today; down the line we may make it the default, but I worry about breaking things that e.g. assume they can mutate the deployment before rebooting and have `/etc` already merged. There's not that many things in libostree that iterate over the deployment list. The biggest change here is around the `ostree_sysroot_write_deployments_with_options` API. I initially tried hard to support a use case like "push a rollback" while retaining the staged deployment, but everything gets very messy because that function truly is operating on the bootloader list. For now what I settled on is to just discard the staged deployment; down the line we can enhance things. Where we then have some new gymnastics is around implementing the finalization; we need to go to some effort to pull the staged deployment out of the list and mark it as unstaged, and then pass it down to `write_deployments()`.
Followup to: #1503 After starting some more work on on this in rpm-ostree, it is actually simpler if the staged deployment just shows up in the list. It's effectively opt-in today; down the line we may make it the default, but I worry about breaking things that e.g. assume they can mutate the deployment before rebooting and have `/etc` already merged. There's not that many things in libostree that iterate over the deployment list. The biggest change here is around the `ostree_sysroot_write_deployments_with_options` API. I initially tried hard to support a use case like "push a rollback" while retaining the staged deployment, but everything gets very messy because that function truly is operating on the bootloader list. For now what I settled on is to just discard the staged deployment; down the line we can enhance things. Where we then have some new gymnastics is around implementing the finalization; we need to go to some effort to pull the staged deployment out of the list and mark it as unstaged, and then pass it down to `write_deployments()`. Closes: #1539 Approved by: jlebon
Followup to: #1503 After starting some more work on on this in rpm-ostree, it is actually simpler if the staged deployment just shows up in the list. It's effectively opt-in today; down the line we may make it the default, but I worry about breaking things that e.g. assume they can mutate the deployment before rebooting and have `/etc` already merged. There's not that many things in libostree that iterate over the deployment list. The biggest change here is around the `ostree_sysroot_write_deployments_with_options` API. I initially tried hard to support a use case like "push a rollback" while retaining the staged deployment, but everything gets very messy because that function truly is operating on the bootloader list. For now what I settled on is to just discard the staged deployment; down the line we can enhance things. Where we then have some new gymnastics is around implementing the finalization; we need to go to some effort to pull the staged deployment out of the list and mark it as unstaged, and then pass it down to `write_deployments()`. Closes: #1539 Approved by: jlebon
Followup to: #1503 After starting some more work on on this in rpm-ostree, it is actually simpler if the staged deployment just shows up in the list. It's effectively opt-in today; down the line we may make it the default, but I worry about breaking things that e.g. assume they can mutate the deployment before rebooting and have `/etc` already merged. There's not that many things in libostree that iterate over the deployment list. The biggest change here is around the `ostree_sysroot_write_deployments_with_options` API. I initially tried hard to support a use case like "push a rollback" while retaining the staged deployment, but everything gets very messy because that function truly is operating on the bootloader list. For now what I settled on is to just discard the staged deployment; down the line we can enhance things. Where we then have some new gymnastics is around implementing the finalization; we need to go to some effort to pull the staged deployment out of the list and mark it as unstaged, and then pass it down to `write_deployments()`. Closes: #1539 Approved by: jlebon
Followup to: #1503 After starting some more work on on this in rpm-ostree, it is actually simpler if the staged deployment just shows up in the list. It's effectively opt-in today; down the line we may make it the default, but I worry about breaking things that e.g. assume they can mutate the deployment before rebooting and have `/etc` already merged. There's not that many things in libostree that iterate over the deployment list. The biggest change here is around the `ostree_sysroot_write_deployments_with_options` API. I initially tried hard to support a use case like "push a rollback" while retaining the staged deployment, but everything gets very messy because that function truly is operating on the bootloader list. For now what I settled on is to just discard the staged deployment; down the line we can enhance things. Where we then have some new gymnastics is around implementing the finalization; we need to go to some effort to pull the staged deployment out of the list and mark it as unstaged, and then pass it down to `write_deployments()`. Closes: #1539 Approved by: jlebon
Followup to: #1503 After starting some more work on on this in rpm-ostree, it is actually simpler if the staged deployment just shows up in the list. It's effectively opt-in today; down the line we may make it the default, but I worry about breaking things that e.g. assume they can mutate the deployment before rebooting and have `/etc` already merged. There's not that many things in libostree that iterate over the deployment list. The biggest change here is around the `ostree_sysroot_write_deployments_with_options` API. I initially tried hard to support a use case like "push a rollback" while retaining the staged deployment, but everything gets very messy because that function truly is operating on the bootloader list. For now what I settled on is to just discard the staged deployment; down the line we can enhance things. Where we then have some new gymnastics is around implementing the finalization; we need to go to some effort to pull the staged deployment out of the list and mark it as unstaged, and then pass it down to `write_deployments()`. Closes: #1539 Approved by: jlebon
Followup to: #1503 After starting some more work on on this in rpm-ostree, it is actually simpler if the staged deployment just shows up in the list. It's effectively opt-in today; down the line we may make it the default, but I worry about breaking things that e.g. assume they can mutate the deployment before rebooting and have `/etc` already merged. There's not that many things in libostree that iterate over the deployment list. The biggest change here is around the `ostree_sysroot_write_deployments_with_options` API. I initially tried hard to support a use case like "push a rollback" while retaining the staged deployment, but everything gets very messy because that function truly is operating on the bootloader list. For now what I settled on is to just discard the staged deployment; down the line we can enhance things. Where we then have some new gymnastics is around implementing the finalization; we need to go to some effort to pull the staged deployment out of the list and mark it as unstaged, and then pass it down to `write_deployments()`. Closes: #1539 Approved by: jlebon
Followup to: #1503 After starting some more work on on this in rpm-ostree, it is actually simpler if the staged deployment just shows up in the list. It's effectively opt-in today; down the line we may make it the default, but I worry about breaking things that e.g. assume they can mutate the deployment before rebooting and have `/etc` already merged. There's not that many things in libostree that iterate over the deployment list. The biggest change here is around the `ostree_sysroot_write_deployments_with_options` API. I initially tried hard to support a use case like "push a rollback" while retaining the staged deployment, but everything gets very messy because that function truly is operating on the bootloader list. For now what I settled on is to just discard the staged deployment; down the line we can enhance things. Where we then have some new gymnastics is around implementing the finalization; we need to go to some effort to pull the staged deployment out of the list and mark it as unstaged, and then pass it down to `write_deployments()`. Closes: #1539 Approved by: jlebon
Prep for ostreedev/ostree#1503 which will add `/usr/lib/ostree/ostree-deploy-staged`.
Add API to write a deployment state to
/run/ostree/staged-deployment
,along with a systemd service which runs at shutdown time.
Just compile tested, still WIP.
For: coreos/rpm-ostree#40