daemon: unit-testable rpm-ostree wrapper and karg improvements #2360
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
approved
|
Throwing this up for early review. If the team likes the approach for unit-testable executables such as Anyway, if the team likes this approach I can continue with the work. |
|
/test all |
darkmuggle
changed the title
darkmuggle
removed
the
do-not-merge/work-in-progress
|
Okay, this is ready now that 4.8 is opened. |
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
Related to this we have coreos/rpm-ostree#2389 and I hope to add Go rpm-ostree bindings at some point and take some of the trickier interaction logic there. That might naturally lead to making things easier to mock. That said I think by far the biggest MCO simplification would be if rpm-ostree implemented coreos/rpm-ostree#2326 (this is also related to ye olde #1190 ) - basically the MCO would just translate the MachineConfig bits into e.g. Anyways I think the rpm-ostree Go bindings would be extracted from this repo, and it makes total sense from that PoV to have things like "reconcile kernel args" be part of that. |
darkmuggle
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
I am generally in favour of splitting it up into interfaces, however with this PR I was unable to test the details due to an error (see below comments).
Also in general there are some points that may change behaviour (e.g. not allowing you to update OS now, whereas before it was a silent no-op, see below) that would block rhel nodes from updating. I think we should be very careful not to change that explicit behaviour.
|
@yuqi-zhang I dropped the errors for now to preserve the behavior. If this merges this week (no pressure -- it merges when it merges or not) I'll follow up with PR's that build on it. |
|
/retest |
pkg/daemon/rpm-ostree.go
Outdated
| // commands. These checks are not meant to be exhaustive. | ||
| switch noun { | ||
| // Argless commands | ||
| case "cancel", "rollblack": |
| // known error conditions | ||
| case "": | ||
| return nil, errNotEnoughArgs | ||
| default: |
There was a problem hiding this comment.
"status" is still not supported in this switch case right?
There was a problem hiding this comment.
Yeah with this PR I get Failed to initialize single run daemon: error reading osImageURL from rpm-ostree: unsupported command 'rpm-ostree status'
There was a problem hiding this comment.
I reworked this to include full command validation, but this will need some serious testing.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashcrow, cgwalters, darkmuggle 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 |
|
/hold (again) Putting this back into hold state pending validation on RHEL 7. |
openshift-ci-robot
added
the
do-not-merge/hold
This is a first-step towards unit-testable rpm-ostree commands by creating a mockable rpm-ostree wrapper. To demonstrate the idea, kernel arguments were plumbed into the new facility. The NodeUpdateClient is the top level interface used by the daemon for doing node updates and configuration. However, not all `rpm-ostree` commands were using the CoreOS implementation of `RpmOstreeClient.` To facilate mockability, all `rpm-ostree` commands are now routed through `RpmOstreeClient`. Mocking of `rpm-ostree` command is done through the new struct field of `runRpmOstreeFunc`. Unit tests, can use this faculty to mock output of `rpm-ostree`. The first unit-test that uses this mocking feature is kernel arguments. `rpm-ostree kargs --delete` will only delete kargs that it knows about, and attempting to delete a run-time or missing karg results in an error. By mocking `rpm-ostree` the logic is of adding, removing and updating kernel arguments is provable. Finally, this does introduce a behavior change. Previously if an admin adjusted the kargs outside of the MCD and upgrade would fail due to the kargs being missing. The MCD will now tolerate the missing kargs and apply the expected state. Unit tests have been added deal with expecting, albeit runtime missing kargs. Signed-off-by: Ben Howard <ben.howard@redhat.com>
|
@darkmuggle: 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. |
|
Withdrawling as stale. |
This is a first-step towards unit-testable rpm-ostree commands by
creating a mockable rpm-ostree wrapper. To demonstrate the idea, kernel
arguments were plumbed into the new facility.
The NodeUpdateClient is the top level interface used by the daemon for
doing node updates and configuration. However, not all
rpm-ostreecommands were using the CoreOS implementation of
RpmOstreeClient.Tofacilate mockability, all
rpm-ostreecommands are now routedthrough
RpmOstreeClient. Mocking ofrpm-ostreecommand is donethrough the new struct field of
runRpmOstreeFunc. Unit tests, can usethis faculty to mock output of
rpm-ostree.The first unit-test that uses this mocking feature is kernel arguments.
rpm-ostree kargs --deletewill only delete kargs that it knows about,and attempting to delete a run-time or missing karg results in an
error. By mocking
rpm-ostreethe logic is of adding, removing andupdating kernel arguments is provable.
Finally, this does introduce a behavior change. Previously if an admin
adjusted the kargs outside of the MCD and upgrade would fail due to the
kargs being missing. The MCD will now tolerate the missing kargs and
apply the expected state. Unit tests have been added deal with
expecting, albeit runtime missing kargs.
Signed-off-by: Ben Howard ben.howard@redhat.com
- What I did
- How to verify it
- Description for the changelog