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

Turn staged deployments on by default #1430

Closed
wants to merge 4 commits into from

Conversation

cgwalters
Copy link
Member

Let's use this PR to track turning it on by default. I'd like to
do that after the next release. So far, everything seems fine.

@jlebon
Copy link
Member

jlebon commented Jun 27, 2018

I haven't hit any issues either and haven't heard of anyone else hitting issues. So I'm +1 for enabling it next release!

@cgwalters
Copy link
Member Author

OK, looks like various things are going to need fixes, for example livefs. Which actually gets easier with staging enabled by default I think.

Some interesting questions here - the misc-2 failure around rollback - do we error out on that if the deployment is staged? Probably. So we should make that bit conditional on !staged.

@dustymabe
Copy link
Member

could we possibly enable this in rawhide for a month before we enable by default?

@cgwalters
Copy link
Member Author

could we possibly enable this in rawhide for a month before we enable by default?

Sure, would just need some fairly trivial build-time machinery here (--enable-staged) and in the spec.

@jlebon
Copy link
Member

jlebon commented Jun 28, 2018

Hmm, OK yeah just hit the rollback one locally. Let's say we fix those before releasing? Will have a look in a bit.

@jlebon
Copy link
Member

jlebon commented Jun 28, 2018

So, apart from #1434, it seems like all the other failures are either pure test adaptations or due to livefs handling of staged deployments. Given that livefs and StagedDeployments are both still experimental, I think we can leave the rest for after the release?

@cgwalters
Copy link
Member Author

I think we can leave the rest for after the release?

Yep - to make sure I understand, you want to fix staged+rollback before the release? That's fine by me. I definitely think we need to get the release out in order to smoke out the Rust bits and make sure that all works (before we do more of it).

@jlebon
Copy link
Member

jlebon commented Jun 28, 2018

Yeah, basically #1431, #1433, and #1434.

@cgwalters
Copy link
Member Author

Yeah, basically #1431, #1433, and #1434.

OK, that's fine by me!

@cgwalters cgwalters self-assigned this Jul 1, 2018
self->ex_stage_deployments = g_key_file_get_boolean (config, EXPERIMENTAL_CONFIG_GROUP,
"StageDeployments", NULL);
/* The default used to be `false`, so check for the key being present */
if (g_key_file_has_key (config, EXPERIMENTAL_CONFIG_GROUP, "StageDeployments", NULL))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just one release with that, and then we drop support for the key entirely? Or were you thinking of keeping around support for StageDeployments=false? I'm leaning more towards just not supporting turning it off.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this PR there's a build-time option; @dustymabe was arguing that this should go into rawhide first or something. I dunno if that's really worth it. We have FAHC for automated testing before release.

Copy link
Member

Choose a reason for hiding this comment

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

yeah preferably it would go to rawhide/f29 first.. especially since we have silverblue, IoT, and FAH to consider.

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinions either way. Seems relatively low risk at this point, but I'm fine with delaying f28 for one more release or so. (Maybe at the same time we drop support for the StageDeployments option completely),

@cgwalters
Copy link
Member Author

OK, so the livefs thing is harder to fix, basically when we go to push the rollback deployment, the current libostree code prunes the staged deployment.

I could imagine chaging livefs to e.g. install --live or so, then we do it all in one transaction, and we can push the rollback first.

That said, really we want to enhance libostree here to support this, there's no real blockers other than dealing with the complexity in ostree_sysroot_write_deployments_with_options().

@cgwalters
Copy link
Member Author

cgwalters commented Jul 13, 2018

@jlebon
Copy link
Member

jlebon commented Aug 10, 2018

bot, retest this please

@cgwalters
Copy link
Member Author

The thing confusing me is that the tests pass locally; that's why I hadn't made the change to the deployment count check.

@cgwalters
Copy link
Member Author

OK, I figured it out - the multitest.py approach has newly provisioned hosts with a rollback deployment, but not subsequent tests.

Nothing calls it today; looks like it was last used in
283b915
So it's easier to debug.

This inlines the helper into the only function that uses it.
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.
We've put a lot of work into staged deployments, it's time
to pull the trigger and turn them on by default.  This is
a key step for enabling `stage` mode automatic updates by
default in e.g. Fedora CoreOS/Silverblue.

We add a new `--disable-staged` build-time option to flip
things back.
@cgwalters cgwalters removed the WIP label Aug 20, 2018
@cgwalters cgwalters changed the title WIP: Turn staged deployments on by default Turn staged deployments on by default Aug 20, 2018
@cgwalters
Copy link
Member Author

Updated commit message, lifting WIP. Let's do this!

@@ -29,9 +29,9 @@ set -x
# This test suite enables the libostree staging feature, and executes
# the layering-relayer tests.
Copy link
Member

Choose a reason for hiding this comment

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

Seems like these comments should be updated now for s/enables/disables?

self->ex_stage_deployments = g_key_file_get_boolean (config, EXPERIMENTAL_CONFIG_GROUP,
"StageDeployments", NULL);
/* The default used to be `false`, so check for the key being present */
if (g_key_file_has_key (config, EXPERIMENTAL_CONFIG_GROUP, "StageDeployments", NULL))
Copy link
Member

Choose a reason for hiding this comment

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

No strong opinions either way. Seems relatively low risk at this point, but I'm fine with delaying f28 for one more release or so. (Maybe at the same time we drop support for the StageDeployments option completely),

@jlebon
Copy link
Member

jlebon commented Aug 20, 2018

Minor comment nit, though not worth a respin. Let's go!
@rh-atomic-bot r+ aa26b20

@rh-atomic-bot
Copy link

⚡ Test exempted: pull fully rebased and already tested.

rh-atomic-bot pushed a commit that referenced this pull request Aug 20, 2018
So it's easier to debug.

This inlines the helper into the only function that uses it.

Closes: #1430
Approved by: jlebon
rh-atomic-bot pushed a commit that referenced this pull request Aug 20, 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: #1430
Approved by: jlebon
rh-atomic-bot pushed a commit that referenced this pull request Aug 20, 2018
We've put a lot of work into staged deployments, it's time
to pull the trigger and turn them on by default.  This is
a key step for enabling `stage` mode automatic updates by
default in e.g. Fedora CoreOS/Silverblue.

We add a new `--disable-staged` build-time option to flip
things back.

Closes: #1430
Approved by: jlebon
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.

4 participants