Test karg drift #3115
Test karg drift #3115
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters 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 |
openshift-ci
bot
added
the
approved
I'm debugging https://bugzilla.redhat.com/show_bug.cgi?id=2075126 and while I haven't verified this is the case, as far as I can tell from looking through the code and thinking about things, if we somehow fail to apply the expected kernel arguments (which can occur if `ostree-finalize-staged` fails) then we will (on the next boot) drop in to `validateOnDiskState()` which has for a long time checked that all the expected *files* exist and mark the update as complete. But we didn't check the kernel arguments. That can then cause later problems because in trying to apply further updates we'll ask rpm-ostree to try to remove kernel arguments that aren't actually present. Worse, often these kernel arguments are actually *quite important* and may even have security relevant properties (e.g. `nosmt`). Now...I am actually increasingly convinced that we *really* need to move opinionated kernel argument handling into ostree (and rpm-ostree). There's ye olde ostreedev/ostree#2217 and the solution may look something like that. Particularly now with the layering philosophy that it makes sense to support e.g. customizations dropping content in `/usr/lib` and such. For now though, validating that we didn't get the expected kargs should make things go Degraded, the same as if there was a file conflict. And *that* in turn should make it easier to debug failures. As of right now, it will appear that updates are complete, and then we'll only find out much later that the kargs are actually missing. And in turn, because kubelet spams the journal, any error messages from e.g. `ostree-finalize-staged.service` may be lost.
Putting this a separate commit/PR because I think we also need to bump our e2e times to ship this.
cgwalters
force-pushed
the
test-karg-drift
branch
from
a843a97 to
c72c5c8
Compare
|
@cgwalters: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
OK so I ran into some tech debt working on this around the And...it's the only one to do so, even though the other tests rely on a created The next thing I noticed is that function actually returns a "cleanup function" that should be called...but isn't, and that's how the other tests can use it! I started looking at cleaning this up...I think what we actually want is something like: But then I got distracted looking at layering branch stuff. Not too sure about reworking the tests right now. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Is this still getting worked on or we should close this? |
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. 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. |
Add an e2e for #3105