OCPBUGS-8113: daemon: Make switchKernel less stateful #3580
Conversation
openshift-ci
bot
added
the
approved
|
Now that we've branched, we can benefit from the fact that we can land PRs like this in master with much lower risk/impact. Once (OK, openshift/release#36937 just landed) so let's give that a go: /test e2e-gcp-ovn-rt-upgrade |
|
@cgwalters: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test e2e-gcp-ovn-rt-upgrade |
pkg/daemon/update.go
Outdated
|
|
||
| if canonicalizeKernelType(oldConfig.Spec.KernelType) == ctrlcommon.KernelTypeRealtime && canonicalizeKernelType(newConfig.Spec.KernelType) == ctrlcommon.KernelTypeDefault { | ||
| switchingToThroughput := oldKtype == ctrlcommon.KernelTypeRealtime && newKtype == ctrlcommon.KernelTypeDefault |
There was a problem hiding this comment.
question: Wouldn't it be clearer to use switchingToDefault instead of switchingToThroughput?
I'm a bit confused on where throughput comes from.
There was a problem hiding this comment.
I'm a bit confused on where throughput comes from.
Yeah sorry that's just me having scars from years of people saying e.g. "normal RHEL" with the implication that e.g. RHEL CoreOS is not-normal. (Or people say "normal Fedora" etc. versus "Silverblue"). Or less pejoratively they say "default RHEL"...which isn't bad but is also not super descriptive because, hey maybe one day what the default is changes 😉
From the kernel side you could certainly say kernel is the default. But it is really about latency (kernel-rt) versus throughput (kernel). And I personally find this is a better description.
(Also IMO the "realtime" kernel is a bit of a misnomer because it's really soft real-time which is actually quite different from hard real time, so I personally think calling it "latency optimized" is better)
There was a problem hiding this comment.
And to expand on this, personally I'd rename kernel -> kernel-throughput-optimized and kernel-rt -> kernel-latency-optimized, I'd also rename "rhel coreos" => "rhel (image mode)" and most cruically "rhel" => "rhel (package mode)" - but only where it matters; otherwise they're both just RHEL. Just like how both kernel-throughput-optimized and kernel-latency-optimized are both really just Linux (aka kernel) in different modes.
Or to say it another way, both are normal. We don't strictly think of one as "default" even. They both have qualifiers, but only where it matters.
There was a problem hiding this comment.
And to expand even more, of course today we say "OpenShift" and "Hypershift" - with the implication that the latter is the different/not-normal case. I've even seen people refer to current OpenShift as "normal" OpenShift! But in fact "hypershift" is (and should be!) well on its way to becoming the default, so it's also like OpenShift => OpenShift (standalone) and Hypershift => OpenShift (hosted control plane) etc.
There was a problem hiding this comment.
But all this aside, actually I was not consistent in trying to use "throughput" instead of "default" and the top patch stops using "throughput" anyways 😄
There was a problem hiding this comment.
Thanks for the very detailed clarification! Naming things is hard and even when we think it's easy, overloaded names, changing contexts, history, etc. all make things even more complicated. I'm OK with the name throughput now.
|
Overall this seems reasonable. I just have two minor concerns that might help clarify things that I've put inline. The first is where the word "throughput" came from. And the second, is what looks like an unfinished comment. The only other (non-blocking) thought is my surprise with how many dependencies were bumped solely from bumping coreos/rpmostree-client-go. |
Yeah, I think a lot of that is transitive deps from containers/image. But also, because we don't update vendored deps here regularly at all, every time we do there's usually a large set. |
|
/test e2e-gcp-ovn-rt-upgrade |
cgwalters
force-pushed
the
kernel-updates-refactor
branch
from
6121b6a
to
8954ae6
Compare
|
Overall this looks fine. I'll approve once the test suites pass, solely because of the large number of dependency changes. |
|
Hmm, the e2e-gcp-ovn-rt-upgrade job failed...but not for a reason I was expecting. First, one thing I notice in this job in that confusingly the -upgrade jobs actually just synthesize a "synthetic" upgrade from current CI without the PR to code with the PR. Consequently, we're not actually doing an OS update in this job, and that means we're not actually running this modified code because we aren't doing an OS update. |
|
Actually...I am confused by that failure since it seems to say that the |
|
/test ? |
|
@cgwalters: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
cgwalters
force-pushed
the
kernel-updates-refactor
branch
from
9ab3588
to
6ac06ee
Compare
|
/payload-job periodic-ci-openshift-release-master-ci-4.13-upgrade-from-stable-4.12-e2e-gcp-ovn-rt-upgrade xref #3485 (comment) |
|
@cgwalters: trigger 1 job(s) for the /payload-(job|aggregate) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/93cd3720-ba16-11ed-9286-0a1a70c20a75-0 |
cgwalters
force-pushed
the
kernel-updates-refactor
branch
2 times, most recently
from
a902c97
to
6d929c3
Compare
|
Man, I was so confused why the code wasn't working and yeah...I modified the legacy dead-code OS update path 😢 😢 Going to do a separate PR to excise that from existence 🪓 entirely. (Edit: done in #3583) |
|
/payload-job periodic-ci-openshift-release-master-ci-4.13-upgrade-from-stable-4.12-e2e-gcp-ovn-rt-upgrade |
|
@cgwalters: trigger 1 job(s) for the /payload-(job|aggregate) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c96e59b0-ba91-11ed-83e3-f9126a759ef7-0 |
cgwalters
force-pushed
the
kernel-updates-refactor
branch
from
6d929c3
to
42ebe46
Compare
|
🎉 Got a green payload run on that previous commit. I had to push a fixup to handle the case of going rt -> throughput without an OS update. I think this only happens in the MCO's CI runs, switching rt -> throughput is an unusual thing to do in production. So let's do one more payload run with tip |
|
@cgwalters: This pull request references Jira Issue OCPBUGS-8113, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| return nil | ||
| } | ||
|
|
||
| // TODO: Drop this code and use https://github.com/coreos/rpm-ostree/issues/2542 instead | ||
| defaultKernel := []string{"kernel", "kernel-core", "kernel-modules", "kernel-modules-extra"} |
There was a problem hiding this comment.
I missed to ask this, since we are not yet adding kernel-modules-core in defaultKernel list packages. Fixing OCPBUGS-8113 will still need that, correct?
There was a problem hiding this comment.
Yes; but we can't land this in 4.14/master (still using rhel8.6) if we specify that package. The change to do so for rhel9 is part of that PR, see 4e9fca2
But at a technical level I think we can say that this is still "the" fix for OCPBUGS-8113 since it has 98% of the required code changes?
There was a problem hiding this comment.
Thanks, this will help QE when they perform testing.
There was a problem hiding this comment.
|
/lgtm Putting hold for qe approval under pre-merge testing |
openshift-ci
bot
added
the
do-not-merge/hold
|
Out of curiosity what would QE be testing that isn't covered by the payload test run and e2e-gcp-op? |
De-duplicate calls to `canonicalizeKernelType` to make the logic easier to read. Also add a few comments.
In prep for usage in MCD.
This is prep for fixing RHEL9 upgrades while maintaining `kernel-rt`. Previously the `switchKernel` logic tried to carefully handle all 4 cases (default -> default, default -> rt, rt -> default, rt -> rt). But, the last one (rt -> rt) was not quite right because the previous `rpm-ostree rebase` command already preserved the previous kernel. In fact it was pretty expensive to do things this way because we'd e.g. regenerate the initramfs *twice*. To say this another way: when doing a RHEL9 update, it's actually the first `rpm-ostree rebase` command which fails before we even get to `switchKernel`. And the reason is due to the introduction of a new `-core` subpackage; xref https://issues.redhat.com/browse/OCPBUGS-8113 So here's the new logic to handle this: - Before we do the `rebase` operation to the new OS, we detect any previous overrides of any packages starting with `kernel-rt` and we remove them. Notably this avoids hardcoding any specific kernel subpackages; we just remove *everything* starting with `kernel-rt` which should be more robust to subpackage changes in the future. - Consequently the `rebase` operation will hence start out by deploying the stock image i.e. with throughput kernel (though note we *are* carefully preserving other local overrides) - The `switchKernel` function now longer needs to take the *previous* machineconfig state into account (except for logging). Instead, we just detect if the target is RT, and if so we then we apply the latest packages. This significantly simplifies the logic in `switchKernel`, and will help fix RHEL9 upgrades.
cgwalters
force-pushed
the
kernel-updates-refactor
branch
from
234c2cb
to
8ac5bee
Compare
|
Rebased 🏄 since another PR bumped the Go deps in the meantime |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, sdodson, sinnykumari The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick release-4.13 |
|
@cgwalters: Jira Issue OCPBUGS-8113: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-8113 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@sdodson: #3580 failed to apply on top of branch "release-4.13": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Set up a manual cherry pick in #3595 |
I understand your point. As per agreement with our QE team, we are following pre-merge testing process since OCP 4.13 to keep things stable for sprintly releases that includes stories with qe_required label and all OCPBUGS related PRs . QE are free to take the call when to test but we manually need qe_approved ack (hopefully someday prow will have automated workflow for this like we have for backport bugs). |
daemon: Clean up
switchKernela bitDe-duplicate calls to
canonicalizeKernelTypeto make thelogic easier to read. Also add a few comments.
vendor: Bump coreos/rpm-ostree-client-go
In prep for usage in MCD.
daemon: Make switchKernel less stateful
This is prep for fixing RHEL9 upgrades while maintaining
kernel-rt.Previously the
switchKernellogic tried to carefully handleall 4 cases (default -> default, default -> rt, rt -> default, rt -> rt).
But, the last one (rt -> rt) was not quite right because
the previous
rpm-ostree rebasecommand already preserved the previouskernel. In fact it was pretty expensive to do things this way
because we'd e.g. regenerate the initramfs twice.
To say this another way: when doing a RHEL9 update, it's actually
the first
rpm-ostree rebasecommand which fails before weeven get to
switchKernel.And the reason is due to the introduction of a new
-coresubpackage;xref https://issues.redhat.com/browse/OCPBUGS-8113
So here's the new logic to handle this:
rebaseoperation to the new OS, we detectany previous overrides of any packages starting with
kernel-rtand we remove them. Notably this avoids hardcoding any specific
kernel subpackages; we just remove everything starting with
kernel-rtwhich should be more robust to subpackage changesin the future.
rebaseoperation will hence start out by deploying thestock image i.e. with throughput kernel (though note we are
carefully preserving other local overrides)
switchKernelfunction now longer needs to take the previousmachineconfig state into account (except for logging).
Instead, we just detect if the target is RT, and if so we then we
apply the latest packages.
This significantly simplifies the logic in
switchKernel, and willhelp fix RHEL9 upgrades.