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

core: Finish implementing `override replace ./kernel*.x86_64.rpm #1346

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@cgwalters
Member

cgwalters commented Apr 25, 2018

Previously we merged: #1228 AKA 12dc565
My recollection is that was working on it the background, while doing
something else, and I clearly didn't get to the point of testing it "for real".

There are many interlocking issues here to make this work. For example,
the "remove RPM" logic needs special handling for the kernel, because
we also inject content into /usr/lib/ostree-boot and also generate
the initramfs, etc.

The architecture I chose is to have the core detect when a kernel
is changed, and also call into the kernel processing code when removing
a kernel package. But the logic for doing kernel reinstallation client-side
is best alongside the initramfs generation logic which already existed
in the sysroot upgrader.

I extended the test suite to cover what was failing before, and I
tested this interactively. But I'm uncertain about adding a test
for actually booting into the GA kernel as it's quite possible
some bits in userspace rely on a newer kernel. Fixing this properly
really wants some infrastructure to better "re-version" an existing
package without changing its content.

Closes: #1334

cgwalters added some commits Apr 23, 2018

core: Finish implementing `override replace ./kernel*.x86_64.rpm
Previously we merged: #1228 AKA 12dc565
My recollection is that was working on it the background, while doing
something else, and I clearly didn't get to the point of testing it "for real".

There are many interlocking issues here to make this work.  For example,
the "remove RPM" logic needs special handling for the kernel, because
we also inject content into `/usr/lib/ostree-boot` and also generate
the initramfs, etc.

The architecture I chose is to have the core *detect* when a kernel
is changed, and also call into the kernel processing code when removing
a kernel package.  But the logic for doing kernel reinstallation client-side
is best alongside the initramfs generation logic which already existed
in the sysroot upgrader.

I extended the test suite to cover what was failing before, and I
tested this interactively.  But I'm uncertain about adding a test
for actually *booting* into the GA kernel as it's quite possible
some bits in userspace rely on a newer kernel.  Fixing this properly
really wants some infrastructure to better "re-version" an existing
package without changing its content.

Closes: #1334

@cgwalters cgwalters force-pushed the cgwalters:kernel-override-error branch from 9be351f to 2adeeef Apr 25, 2018

@cgwalters

This comment has been minimized.

Member

cgwalters commented Apr 25, 2018

error: Multiple subdirectories found in: usr/lib/modules

Hum. I'm not reproducing this locally yet, though it does bring up an issue I haven't yet debugged: In a traditional librpm+yum stack, what is responsible for deleting script-generated content like /usr/lib/modules/4.15.17-300.fc27.x86_64/modules.dep.bin ? AFAICS those files aren't deleted by rpm.

(A few minutes pass)

OK, after playing around with strace -f -e execve,unlink,unlinkat yum remove kernel in a container...it's actually depmod -a which nukes stale cache files. Ugly.

if (rebuild_from_initramfs)
{
g_assert (argv == NULL);

This comment has been minimized.

@jlebon

jlebon Apr 25, 2018

Member

This can be non-NULL in the initramfs --enable --arg=ARG case, no?

@@ -3874,6 +3912,9 @@ rpmostree_context_assemble (RpmOstreeContext *self,
if (pkg == filesystem_package)
continue;
if (rpmte_is_kernel (te))
self->kernel_changed = TRUE;

This comment has been minimized.

@jlebon

jlebon Apr 25, 2018

Member

We already do this higher up, right?

This comment has been minimized.

@cgwalters

cgwalters Apr 26, 2018

Member

Yeah, I was just being conservative. I can't offhand think of a situation where we're installing a kernel (and not removing one) and a higher level would want to react to it in some way, but it doesn't hurt to make the flag correct right?

This comment has been minimized.

@jlebon

jlebon Apr 26, 2018

Member

I can't offhand think of a situation where we're installing a kernel (and not removing one)

We forbid this anyway right? In prepare, I see dnf_sack_set_installonly_limit. And indeed trying out a rpm-ostree install ./kernel-*.rpm triggers the Forbidden base package replacements: error.

but it doesn't hurt to make the flag correct right?

Yeah, definitely minor :). It's more that having it there implies it could be meaningful. It's like my preference for g_str_equal vs g_strcmp0 when possible.

This comment has been minimized.

@cgwalters

cgwalters Apr 26, 2018

Member

Yeah, but think about the buildroot case where the kernel may end up being pulled in as a dependency.

This comment has been minimized.

@jlebon

jlebon Apr 26, 2018

Member

Ahh yep, good point.

@jlebon

This comment has been minimized.

Member

jlebon commented Apr 26, 2018

@rh-atomic-bot

This comment has been minimized.

rh-atomic-bot commented Apr 26, 2018

⌛️ Testing commit cbbd509 with merge 4e69bda...

rh-atomic-bot added a commit that referenced this pull request Apr 26, 2018

core: Finish implementing `override replace ./kernel*.x86_64.rpm
Previously we merged: #1228 AKA 12dc565
My recollection is that was working on it the background, while doing
something else, and I clearly didn't get to the point of testing it "for real".

There are many interlocking issues here to make this work.  For example,
the "remove RPM" logic needs special handling for the kernel, because
we also inject content into `/usr/lib/ostree-boot` and also generate
the initramfs, etc.

The architecture I chose is to have the core *detect* when a kernel
is changed, and also call into the kernel processing code when removing
a kernel package.  But the logic for doing kernel reinstallation client-side
is best alongside the initramfs generation logic which already existed
in the sysroot upgrader.

I extended the test suite to cover what was failing before, and I
tested this interactively.  But I'm uncertain about adding a test
for actually *booting* into the GA kernel as it's quite possible
some bits in userspace rely on a newer kernel.  Fixing this properly
really wants some infrastructure to better "re-version" an existing
package without changing its content.

Closes: #1334

Closes: #1346
Approved by: jlebon
@cgwalters

This comment has been minimized.

Member

cgwalters commented Apr 26, 2018

@rh-atomic-bot

This comment has been minimized.

rh-atomic-bot commented Apr 26, 2018

⌛️ Testing commit cbbd509 with merge 8726be6...

@rh-atomic-bot

This comment has been minimized.

rh-atomic-bot commented Apr 26, 2018

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 8726be6 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment