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

baremetal: enhancement for ironic config passthrough #401

Closed
wants to merge 1 commit into from

Conversation

derekhiggins
Copy link

This enhancement describes a mechanism to pass arbitrary
config option through to ironic in baremetal ipi.

This enhancement describes a mechanism to pass arbitrary
config option through to ironic in baremetal ipi.
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: derekhiggins
To complete the pull request process, please assign stevekuznetsov
You can assign the PR to them by writing /assign @stevekuznetsov in a comment when ready.

The full list of commands accepted by this bot can be found 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

The motivation for this enhancement is to allow people to deal with their individual corner
cases without the need to insert special cases into the ironic containers. The ironic
container can stay opinionated and targeted for a specific set of environments while also
being usable where uniq envirnments require adjustments.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have some examples of configuration changes that users have needed to change?

Copy link
Author

Choose a reason for hiding this comment

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

Yup, there are 3 examples ongoing that I can think of

Operators trying to deploy kni on openstack(for demos) have been looking for a way to customise IPMI timouts and retries, see here
metal3-io/ironic-image#167

For CI, we've been trying to find a way to add certain kernel args for CI only, one of this merged which I believe means IPA is no longer being logged to the console, making debugging very difficult
metal3-io/ironic-image#172
metal3-io/ironic-image#160

For debugging IPA users need to add a SSH key to kernel args (this one could probbaly be a top level config option)
metal3-io/ironic-image#150

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, there are 3 examples ongoing that I can think of

Operators trying to deploy kni on openstack(for demos) have been looking for a way to customise IPMI timouts and retries, see here
metal3-io/ironic-image#167

That seems like a reasonable thing to put into an API. Is it configurable on a per-host basis, or do we have to put a value into the ironic config file?

For CI, we've been trying to find a way to add certain kernel args for CI only, one of this merged which I believe means IPA is no longer being logged to the console, making debugging very difficult
metal3-io/ironic-image#172
metal3-io/ironic-image#160

Is there any reason we wouldn't want serial console logging all the time? Can we re-enable it and write a test to ensure it can't be disabled accidentally?

If it needs to be configurable, is it a per-host option or global?

For debugging IPA users need to add a SSH key to kernel args (this one could probbaly be a top level config option)
metal3-io/ironic-image#150

Could we use the same ssh key that is given in the install-config for access to the control plane nodes?

```yaml
platform:
baremetal:
ironicExtraConf: {"OS_PXE__PXE_APPEND_PARAMS":'nofb nomodeset vga=normal sshkey="ssh-rsa AAAA..."', "OS_API__API_WORKERS":"8"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ironic (and IPA) was always intended to be an implementation detail for IPI, so I'm pretty uncomfortable with adding a top-level ironicFoo option tbh.

In particular if you check the discussion on coreos/ignition#979 you'll see there are a bunch of unsolved issues wrt the OpenShift coreos-install based ramdisk vs IPA, and the options in either case are completely different.

I wonder if we could perhaps make this more generic and have some pxeExtra interface or something, or even better some interface which describes the required behavior e.g which console to use?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, upstream we want to use the oslo.config environment variable interface instead of customizing individual settings in the ironic config to cut down on the number of changes to the image. The whole point of adding environment variable handling to oslo.config was to make it easier to pass settings to OpenStack services running in containers.

Downstream in OpenShift, however, I would rather that we add explicit APIs for the things we need to allow users to change. That allows us to take advantage of the API server for some basic validation, and makes it harder for users to put the system into an unsupportable state or one where metal3 is crash-looping due to invalid settings.

```yaml
platform:
baremetal:
ironicExtraConf: {"OS_PXE__PXE_APPEND_PARAMS":'nofb nomodeset vga=normal sshkey="ssh-rsa AAAA..."', "OS_API__API_WORKERS":"8"}
Copy link
Contributor

Choose a reason for hiding this comment

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

For readability, can you stick to canonical YAML:

platform: 
  baremetal: 
    ironicExtraConf: 
      OS_API__API_WORKERS: "8"
      OS_PXE__PXE_APPEND_PARAMS: "nofb nomodeset vga=normal sshkey=\"ssh-rsa AAAA...\""

@derekhiggins
Copy link
Author

OK will close this, looks like we'd rather add things when needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants