-
Notifications
You must be signed in to change notification settings - Fork 461
Import pivot code #859
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
Import pivot code #859
Conversation
|
This is amazing. |
|
|
This introduces into the MCO the same CI issue that kubelet But once we fix that there will be huge benefits because this repository will become an easy place to add functionality to RHCOS that's on Github and CI gated in a useful way. |
0447729 to
dd179f1
Compare
|
Lifting WIP on this, it doesn't actually do anything right now by default, but should be a useful baseline for further iteration. (For example there's various cleanups/dedup that we can do to the imported pivot code, but I want to do that once we have CI testing set up, etc.) |
|
Some really trivial nits which we can postpone but I'd love to get the pkg/daemon/pivot for this. Otherwise, it looks great. /approve |
ashcrow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall 👍. I think once this merges then some clean up can be done as there is some overlap per @runcom's notes.
This is prep for having "early MCD" process kernel arguments from MachineConfig objects at pivot time, and more generally building up the model that the host processes `MachineConfig`, not just Ignition. Right now what I've tested is just manually running `machine-config-daemon pivot registry.svc.ci.openshift.org/rhcos/machine-os-content:latest` A next step here is to have the MCS serve the extra MachineConfig data like `KernelArguments` and then have the MCD process that on boot. After that we should have this take over the early pivot role, then we can drop `/usr/bin/pivot` out of RHCOS. See: openshift#798
dd179f1 to
37d962f
Compare
|
/lgtm |
ashcrow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should select the approve radio button when I approve .... :-)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashcrow, cgwalters, runcom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| inspectArgs = append(inspectArgs, fmt.Sprintf("%s", container)) | ||
| output := utils.RunExt(true, 1, "podman", inspectArgs...) | ||
| var imagedataArray []types.ImageInspection | ||
| json.Unmarshal([]byte(output), &imagedataArray) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a json err check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops i commented too late 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably got imported as is from openshift/pivot and yeah, we should have an error check here - let's follow up all the things here :)
This is prep for having "early MCD" process kernel arguments
from MachineConfig objects at pivot time, and more generally
building up the model that the host processes
MachineConfig,not just Ignition.
Right now what I've tested is just manually running
machine-config-daemon pivot registry.svc.ci.openshift.org/rhcos/machine-os-content:latestBut, next step is to test having this take over the early pivot role,
then we can drop
/usr/bin/pivotout of RHCOS.See: #798