Skip to content

Conversation

@cgwalters
Copy link
Member

This effectively "inverts" a MachineConfig object so that
when it boots, Ignition processes the subset of MachineConfig
that is Ignition, and then we have the other stuff (osImageURL,
kernelArguments and in the future more) as a file on disk
that the MCD can read.

The way to think about this is we're aiming to have RHCOS boot
into MachineConfig, not just Ignition.

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 19, 2019
@cgwalters
Copy link
Member Author

Part of #798

@cgwalters
Copy link
Member Author

This does nothing at the moment until the MCD learns to read it, which is going to happen in another PR after more prep work lands from #866 etc - but we have unit coverage and I think this is looking good.

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Jun 19, 2019

I'm a little confused about the mention of "inverting" it?? Would this eventually change our mcs?

From what I can tell here you are taking a machine config, extracting the IGN config part from spec and then adding the non-spec extras like osimageurl into the ignition config? <- is this correct?

I dont fully understand what the final flow will look like or how it will differ from what we have?

@cgwalters
Copy link
Member Author

Currently MachineConfig contains Ignition. With this, the generated Ignition contains the rest of the MachineConfig. "invert" seems right for that, no?

From what I can tell here you are taking a machine config, extracting the IGN config part from spec and then adding the non-spec extras like osimageurl into the ignition config? <- is this correct?

Yep!

I dont fully understand what the final flow will look like or how it will differ from what we have?

The main rationale is in #798
Today we don't have a good way for pivot which runs on the first boot to get access to the kernelArguments in the MachineConfig. This fixes that by serving the whole MachineConfig, and then later PRs will replace pivot with machine-config-daemon which will soon be built into RHCOS (in addition to being run as a container).

@cgwalters
Copy link
Member Author

With this, the generated Ignition contains the rest of the MachineConfig. "invert" seems right for that, no?

Though to be very clear this is just a transient implementation detail for the first boot, invisible to users and most of the code. This inversion is a lot like what we are doing with the node annotations today, just an extension to provide the full MC and not just its hash.

@kikisdeliveryservice
Copy link
Contributor

With this, the generated Ignition contains the rest of the MachineConfig. "invert" seems right for that, no?

Though to be very clear this is just a transient implementation detail for the first boot, invisible to users and most of the code. This inversion is a lot like what we are doing with the node annotations today, just an extension to provide the full MC and not just its hash.

AHA!!!!!!!! Ok gotcha!

@cgwalters
Copy link
Member Author

Another way to think of this is that in theory we could do something like have machine-config-daemon actually wrap Ignition on the firstboot; the MCS would literally serve MachineConfig YAML or whatever and the MCD would unwrap the Ignition part and pass it to Ignition etc.

That would be...a lot more invasive of a change though.

@cgwalters cgwalters mentioned this pull request Jun 19, 2019
@runcom
Copy link
Member

runcom commented Jun 20, 2019

Awesome, I'll approve and leave it for lgtm because of the comment above /run

/approve

1 similar comment
@runcom
Copy link
Member

runcom commented Jun 20, 2019

Awesome, I'll approve and leave it for lgtm because of the comment above /run

/approve

@runcom
Copy link
Member

runcom commented Jun 20, 2019

Awesome, I'll approve and leave it for lgtm because of the comment above /run

/approve
/retest

@runcom
Copy link
Member

runcom commented Jun 20, 2019

Waiting for #868 (comment), otherwise it looks great

/approve
/retest

@runcom
Copy link
Member

runcom commented Jun 20, 2019

/approve
/retest

@runcom
Copy link
Member

runcom commented Jun 20, 2019

Waiting for #868 (comment) but looks great anyway, there seem to be an issue with e2e also

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2019
@cgwalters
Copy link
Member Author

Rebased 🏄‍♂️ and dropped the comment about /run.

@cgwalters
Copy link
Member Author

Interesting, failure to bootstrap in both cases. I haven't tested that case locally yet, just did a make deploy-server. Looking at the logs gathered by the installer, the bootstrap MCS seems fine...but I'll see if I can reproduce this locally.

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Jun 20, 2019

Not sure if relevant, but in the kublet.log I see:
Jun 20 13:09:29 ip-10-0-11-17 hyperkube[1214]: E0620 13:09:29.110280 1214 container_manager_linux.go:476] failed to find cgroups of kubelet - cpu and memory cgroup hierarchy not unified. cpu: /, memory: /system.slice/kubelet.service

Also in approve-csr.log:

Jun 20 12:46:50 ip-10-0-11-17 approve-csr.sh[1392]: error: the server doesn't have a resource type "csr"

Jun 20 12:47:10 ip-10-0-11-17 approve-csr.sh[1392]: The connection to the server api.ci-op-mfl8xx9x-c4a31.origin-ci-int-aws.dev.rhcloud.com:6443 was refused - did you specify the right host or port?```

Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment

@cgwalters cgwalters force-pushed the mcs-serve-mc branch 2 times, most recently from 7ac7123 to fcf6c47 Compare June 20, 2019 17:37
Missed this as part of openshift#773
(Doesn't matter yet, just prep for any future changes)
This effectively "inverts" a `MachineConfig` object so that
when it boots, Ignition processes the subset of MachineConfig
that is Ignition, and then we have the other stuff (`osImageURL`,
`kernelArguments` and in the future more) as a file on disk
that the MCD can read.

The way to think about this is we're aiming to have RHCOS boot
into `MachineConfig`, not just Ignition.
@runcom
Copy link
Member

runcom commented Jun 21, 2019

This is green now :)

@cgwalters
Copy link
Member Author

This is green now :)

Yeah, I broke the main case when refactoring to make the unit tests happy. Both are good now!

@runcom
Copy link
Member

runcom commented Jun 21, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, runcom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants