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.

Sign up for GitHub

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

Jump to bottom

manifest: Add bootupd for x86_64|aarch64 #595

Merged
merged 1 commit into from
Sep 25, 2020
Merged

manifest: Add bootupd for x86_64|aarch64 #595

merged 1 commit into from
Sep 25, 2020

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Sep 2, 2020 •
edited
Loading

This converts the EFI update directory from (rpm-)ostree specific
layout to bootupd layout. Required for both initial
provisioning with bootupd as well as later updates.

Required by: coreos/coreos-assembler#1695

@cgwalters cgwalters marked this pull request as draft September 2, 2020 20:25
@cgwalters cgwalters mentioned this pull request Sep 2, 2020
Merged
@cgwalters cgwalters marked this pull request as ready for review September 11, 2020 18:30
@cgwalters cgwalters changed the title WIP: Run bootupd generate-update-metadata bootable-rpm-ostree: Optionally run bootupd generate-update-metadata Sep 11, 2020
@cgwalters
Copy link
Member Author

OK I reworked this so that it's a no-op if bootupd isn't installed. Hopefully we can merge this now and testing out bootupd will involve less juggling of git branches.

@jlebon
Copy link
Member

jlebon commented Sep 11, 2020

I'm having trouble understanding how this all ties together. I would've expected bootupd on the compose side to interact in coreos-assembler somewhere in create_disk.sh. Doesn't it need to fiddle with /boot?

@cgwalters
Copy link
Member Author

I'm having trouble understanding how this all ties together.

No worries, me too 😄 I messed this all up initially.

Basically this part replaces /usr/lib/ostree-boot.

I would've expected bootupd on the compose side to interact in coreos-assembler somewhere in create_disk.sh. Doesn't it need to fiddle with /boot?

That's a separate phase in coreos/coreos-assembler#1695

This phase needs to happen for every single ostree commit build. The coreos-assembler one happens only when we make a bootimage.

@cgwalters
Copy link
Member Author

Side note: the reason this is useful now is I am testing bootupd via
make install DESTDIR=/var/srv/walters/builds/fcos/overrides/rootfs/ so it doesn't matter that it's not listed in the manifest too. But I do plan to package it soon.

@cgwalters cgwalters force-pushed the use-bootupd branch 3 times, most recently from fd3fdeb to 703da5d Compare September 21, 2020 13:40
@cgwalters cgwalters changed the title bootable-rpm-ostree: Optionally run bootupd generate-update-metadata manifest: Add bootupd for x86_64|aarch64 Sep 21, 2020
@cgwalters
Copy link
Member Author

OK bootupd is in the repos; I reworked this to make it explicitly required for x86_64 and aarch64 (though I need to fix the RPM ExclusiveArchitecture for the latter still).

jlebon
jlebon approved these changes Sep 21, 2020
View reviewed changes
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM overall!

manifests/bootupd.yaml Show resolved Hide resolved
manifests/bootupd.yaml Show resolved Hide resolved
@cgwalters
Copy link
Member Author

coreos/coreos-assembler#1695 (comment)
so that PR needs to land first.

cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request Sep 22, 2020
@cgwalters
buildextend-live: Support bootupd
e7c22fc 
Followup to coreos@5633655

Caught in CI for coreos/fedora-coreos-config#595
cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request Sep 22, 2020
@cgwalters
buildextend-live: Support bootupd
4df072f 
Followup to coreos@5633655

Caught in CI for coreos/fedora-coreos-config#595
@cgwalters cgwalters mentioned this pull request Sep 22, 2020
Merged
openshift-merge-robot pushed a commit to coreos/coreos-assembler that referenced this pull request Sep 22, 2020
@cgwalters @openshift-merge-robot
buildextend-live: Support bootupd
890db64 
Followup to 5633655

Caught in CI for coreos/fedora-coreos-config#595
@cgwalters
Copy link
Member Author

OK, ✔️ CI here. We should probably debate whether to run with this right now; bootupd is still a WIP but I know it works for the "install" case, and having this in will help a lot developing it. OTOH it does mean shipping it to all FCOS users in WIP status.

Hmm...maybe what we want is support for "build options" so that we can merge to testing-devel but turn off the option for stable? Not sure.

@jlebon
Copy link
Member

jlebon commented Sep 22, 2020

We should probably debate whether to run with this right now; bootupd is still a WIP but I know it works for the "install" case, and having this in will help a lot developing it. OTOH it does mean shipping it to all FCOS users in WIP status.

I'm personally fine with merging it. We don't publicize/document the new CLI tool anywhere super user visible right now, and otherwise AFAICT bootupd on its own doesn't change the behaviour of a booted FCOS in any meaningful way, right?

Do you see any risks/potential areas for regression from this change?

@darkmuggle
Copy link
Contributor

darkmuggle commented Sep 22, 2020 •
edited
Loading

OTOH it does mean shipping it to all FCOS users in WIP status.

How much of a WIP is bootupd right now? Is it "alpha" or "beta" quality? If is the former, then I think we should hold off. And what's the risk of bricking a user? FCOS is stable, but we would be introducing something that unstable by definition into the code.

AFAICT bootupd on its own doesn't change the behaviour of a booted FCOS in any meaningful way, right

This PR enables the socket. I would call that a meaningful change.

If we do no foresee a major refactor or changes, then I would be okay shipping this once we have Kola tests to make sure we're good. I think we're probably safe, but let's put up the guard rails first to catch our human mistakes.

@cgwalters
Copy link
Member Author

How much of a WIP is bootupd right now? Is it "alpha" or "beta" quality? If is the former, then I think we should hold off.

I'd say alpha.

And what's the risk of bricking a user?

If they somehow discover and run bootupctl update manually then non-zero, otherwise zero. So...effectively zero.

This PR enables the socket. I would call that a meaningful change.

That's mostly an implementation detail I think, it's a unix domain socket, not listening on TCP or anything like that.

But perhaps the simplest alternative is to keep the socket disabled? That would make it easy to turn on via Ignition or by hand and test out.

@cgwalters
Copy link
Member Author

But perhaps the simplest alternative is to keep the socket disabled? That would make it easy to turn on via Ignition or by hand and test out.

Updated with that change.

jlebon
jlebon approved these changes Sep 22, 2020
View reviewed changes
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM. Though to be safe let's hold until the current round of releases for this week is done. Otherwise, this'll get immediately promoted to testing and next e.g. tomorrow.

overlay.d/15fcos/usr/lib/systemd/system-preset/45-fcos.preset Outdated
@@ -3,3 +3,4 @@ enable fedora-coreos-pinger.service
# Provide information if no ignition is provided
enable coreos-check-ignition-config.service
enable coreos-check-ssh-keys.service

Copy link
Member

Choose a reason for hiding this comment

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

Minor/optional: spurious new line.

@cgwalters
Copy link
Member Author

Also did coreos/bootupd#27

@cgwalters
manifest: Add bootupd for x86_64|aarch64 (socket disabled)
a6d4b4c 
This converts the EFI update directory from (rpm-)ostree specific
layout to bootupd layout.  Required for both initial
provisioning with bootupd as well as later updates.

For now the `bootupd.socket` is disabled by default to further
discourage users from trying it until we've fully productized it.

This will be used by: coreos/coreos-assembler#1695
@jlebon
Copy link
Member

jlebon commented Sep 25, 2020

Releases are out now. Restarted CI here.

@jlebon jlebon merged commit 8be2720 into coreos:testing-devel Sep 25, 2020
@cgwalters cgwalters mentioned this pull request Sep 29, 2020
Closed
This was referenced Oct 28, 2020
Closed
Closed
cgwalters added a commit to cgwalters/os that referenced this pull request Nov 3, 2020
@cgwalters
manifest: Include bootupd
5c3855e 
Basically pulling in coreos/fedora-coreos-config#595
for RHCOS.  The immediate goal is allowing updating existing
systems in place for Boot Hole.
@cgwalters cgwalters mentioned this pull request Nov 3, 2020
Merged
cgwalters added a commit to cgwalters/os that referenced this pull request Nov 4, 2020
@cgwalters
manifest: Include bootupd
aea44ed 
Basically pulling in coreos/fedora-coreos-config#595
for RHCOS.  The immediate goal is allowing updating existing
systems in place for Boot Hole.
c4rt0 pushed a commit to c4rt0/fedora-coreos-config that referenced this pull request Mar 27, 2023
@openshift-merge-robot
Merge pull request coreos#595 from miabbott/disable_crio_base
a4570b2 
kola-denylist: disable crio.base tests for all arches
c4rt0 pushed a commit to c4rt0/fedora-coreos-config that referenced this pull request Mar 27, 2023
@miabbott
kola-denylist: use string instead of list for tracker
c4cef1e 
I strongly believe that I tested the use of a list in the denylist as
part of coreos#595, but evidence says otherwise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlebon jlebon jlebon approved these changes

@darkmuggle darkmuggle Awaiting requested review from darkmuggle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cgwalters @jlebon @darkmuggle