Skip to content
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

Add MCO Flattened Ignition proposal #467

Closed
wants to merge 1 commit into from

Conversation

hardys
Copy link
Contributor

@hardys hardys commented Sep 2, 2020

Draft proposal to add MCO management of a "flattened" ignition configuration - this could be used to break the chicken/egg problem evident in some network configurations (particularly in the case of baremetal), e.g https://bugzilla.redhat.com/show_bug.cgi?id=1824331

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 2, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hardys

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2020
@hardys
Copy link
Contributor Author

hardys commented Sep 2, 2020

/cc @celebdor @stbenjam @kirankt

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Let's close openshift/machine-config-operator#1690 in favor of this please.

Overall...I'm OK with this. The text notes that long term we want to move baremetal IPI to the Live ISO which solves a whole lot of problems in this area.

enhancements/machine-config/mco-flattened-ignition.md Outdated Show resolved Hide resolved
enhancements/machine-config/mco-flattened-ignition.md Outdated Show resolved Hide resolved
@hardys
Copy link
Contributor Author

hardys commented Sep 11, 2020

Let's close openshift/machine-config-operator#1690 in favor of this please.

Now closed.

Overall...I'm OK with this. The text notes that long term we want to move baremetal IPI to the Live ISO which solves a whole lot of problems in this area.

Thanks for the review! Yes I added clarification that long term we want to address that overlap.

Sounds like we're OK to proceed with some implementation work on this pending further reviews, I'll take a look and get something up as a WIP PR.

There are two remaining open questions that will influence the implementation:

What kind of resource should we use to store the flattented config - I'm thinking a MachineConfig like the rendered config?

How do we access the flattened config in the machine-api actuator, given that the MCS isn't accessible from services running on the cluster? I'm thinking we ensure it's possible to derive which config to use (either by role or some additional metadata) for any given machine, then access the resource directly via the kube API (will the machine controller API creds allow this, given that the namespaces are different?)

@hardys
Copy link
Contributor Author

hardys commented Sep 14, 2020

/retitle Add MCO Flattened Ignition proposal

@openshift-ci-robot openshift-ci-robot changed the title [WIP] Add MCO Flattened Ignition proposal Add MCO Flattened Ignition proposal Sep 14, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2020
@craychee
Copy link

Co-authored-by: Antoni Segura Puimedon <celebdor@gmail.com>
@hardys
Copy link
Contributor Author

hardys commented Oct 7, 2020

Updated to account for feedback so far (thanks!) - also added more design details based on initial investigations, and some details of CI coverage we can use to test this.


### Version Skew Strategy

## Alternatives
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another alternative could be to just merge the installer generated ignition config into the rendered config (I guess we'd need to create a MachineConfig object derived from the installer generated user-data secret, then rely on the existing MergeMachineConfigs logic in MCO.

@runcom that would also solve the issue with the management of the pointer ignition, since we'd convert any data from the installer-created ignition to a MachineConfig, thus the pointer config can just be statically tempated as in your recent implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify this idea, something like:

We could potentially flag some warning when this happens, and use it as a migration path to eventually deprecate the direct ignition-configs customization in favor of MachineConfig manifests?

This would also solve the issue for IPI baremetal without any MCO API changes, since we could consume the existing rendered config directly (we did this previously ref openshift/installer#3276 but that was also reverted for the same reason as above)

@cgwalters @crawford any thoughts?

Copy link
Contributor Author

@hardys hardys Oct 8, 2020

Choose a reason for hiding this comment

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

I guess there are some RBAC considerations:

  • The current user-data secret ends up in the openshift-machine-api namespace - Manage the ignition stub config machine-config-operator#1792 worked around that by generating the pointer ignition manifest in the right namespace
  • We could adjust the installer to write future user-data to the openshift-machine-config-operator namespace (so it can be read and converted into a MachineConfig resource by the MCO, or perhaps the installer just writes the data in that format?)
  • On upgrade the plan from Manage the ignition stub config machine-config-operator#1792 remains, e.g just reference the existing non-managed user-data secret?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at the code, and it's probably simplest to have the installer generate a MachineConfig manifest that contains any config provided via create ignition-configs

That avoids any changes to the MCO (other than restoring the reverted pointer-ignition change from @runcom) and potentially allows us to warn the user when config is provided via that interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm generally in favour of the MCO managing the stub configs coming from installer in one way or another, and this approach will probably work. I think we should keep the following points in mind:

  1. Since the stub configs were never changed before we never considered any dependency between the MAO and the MCO. This method would likely (as I understand it) introduce an ordering where the MCO has to process the stub config first before machines can be booted.
  2. We implicitly supported "per-node" configuration with the stub config, for example after you generated the stub config, you could configure each node's networking separately before boot with a different static IP via the ignition stub, and the MCO would be fine with it since it had no understanding of what existed in the stub. I guess for this example, the generated configs with openshift-installer create ignition-configs would have been supplied to the nodes manually. Is this a case we care about?

Copy link
Member

Choose a reason for hiding this comment

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

good points, I care about solving point number 2 as it seems we really never wanted to advertise the use of that (last we checked with @crawford )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm generally in favour of the MCO managing the stub configs coming from installer in one way or another, and this approach will probably work. I think we should keep the following points in mind:

1. Since the stub configs were never changed before we never considered any dependency between the MAO and the MCO. This method would likely (as I understand it) introduce an ordering where the MCO has to process the stub config first before machines can be booted.

I think we can avoid this if instead the installer creates a MachineConfig manifest with any user customizations it finds on the create cluster phase, and we could either skip generating this or leave it empty in the case where the stub config output via the create ignition-configs phase is unmodified?

We can also warn the user if the decision is made to deprecate the modification of the stub config with this approach (which may be harder if we have the MCO process the stub config, in addition to the ordering concern you raise above).

2. We implicitly supported "per-node" configuration with the stub config, for example after you generated the stub config, you could configure each node's networking separately before boot with a different static IP via the ignition stub, and the MCO would be fine with it since it had no understanding of what existed in the stub. I guess for this example, the generated configs with `openshift-installer create ignition-configs` would have been supplied to the nodes manually. Is this a case we care about?

Good point - IIUC this has only ever worked for the UPI case, where you download the stub config from the installer create ignition-configs phase, then host some per-host modified configs somewhere outside of the cluster?

In the IPI case, there's only a single secret per role, so per-node config is not currently supported via that workflow.

So, I think to ensure both workflows continue to work the same as today, we just need to ensure that the new MachineConfig object either isn't generated or is empty at the create ignition-configs stage, and that we can detect and re-generate that asset in the case where some user-customization happened between create ignition-configs and create cluster (I guess we can compare the assets loaded from file with that generated inside the installer).

hardys pushed a commit to hardys/enhancements that referenced this pull request Nov 16, 2020
hardys pushed a commit to hardys/enhancements that referenced this pull request Nov 16, 2020
hardys pushed a commit to hardys/enhancements that referenced this pull request Nov 16, 2020
@hardys
Copy link
Contributor Author

hardys commented Nov 30, 2020

Superseded by #540

@hardys hardys closed this Nov 30, 2020
hardys pushed a commit to hardys/enhancements that referenced this pull request Dec 3, 2020
hardys pushed a commit to hardys/enhancements that referenced this pull request Jan 29, 2021
VaishnaviHire pushed a commit to VaishnaviHire/enhancements that referenced this pull request Feb 11, 2021
mansikulkarni96 pushed a commit to mansikulkarni96/enhancements that referenced this pull request Mar 26, 2021
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants