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.

Sign up for GitHub

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

Jump to bottom

[WIP] baremetal: Pass through of config variables to ironic #3887

Closed
wants to merge 2 commits into from
Closed

[WIP] baremetal: Pass through of config variables to ironic #3887

wants to merge 2 commits into from

Conversation

derekhiggins
Copy link
Contributor

Makes use of oslo.config's ability to read config from env variables
see metal3-io/ironic-image#165

@derekhiggins
Copy link
Contributor Author

/hold
this only works for ironic on the bootstrap node, holding while I look into how to also do it on master nodes.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2020
stbenjam
stbenjam suggested changes Jul 14, 2020
View reviewed changes
Copy link
Member

@stbenjam stbenjam left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I like the idea of a top-level platform configuration for setting Ironic's internal configuration. It's exposing an implementation detail we prefer to keep hidden, if at all possible. If we have to make it available, it might belong tucked away in an Advanced struct member in the baremetal platform, with appropriate warnings.

This also doesn't handle how we pass this through to MAO/CBO for the in-cluster Ironic images.

TBH, I think this change needs an enhancement request so we can get all the details worked out.

pkg/types/baremetal/platform.go
@@ -108,4 +108,7 @@ type Platform struct {
//
// +optional
ClusterOSImage string `json:"clusterOSImage,omitempty" validate:"omitempty,osimageuri,urlexist"`


IronicExtraConf map[string]interface{} `json:"ironicExtraConf,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Public struct members need godoc comment

@stbenjam
Copy link
Member

this only works for ironic on the bootstrap node, holding while I look into how to also do it on master nodes.

We'll need to update the Provisioning CRD to support a mechanism to pass-through Ironic configuration. It'll be an API change. openshift/machine-api-operator#632 alters the API and makes use of it, not quite the same idea but it should give you the gist of how the change would look.

abhinavdahiya
abhinavdahiya reviewed Jul 14, 2020
View reviewed changes
pkg/types/baremetal/platform.go
@@ -108,4 +108,7 @@ type Platform struct {
//
// +optional
ClusterOSImage string `json:"clusterOSImage,omitempty" validate:"omitempty,osimageuri,urlexist"`


IronicExtraConf map[string]interface{} `json:"ironicExtraConf,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

secondly a map of string to anything is not a great API.

Let's make sure there is an enhancement for this new field before we move ahead on this.

/hold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

secondly a map of string to anything is not a great API.

Let's make sure there is an enhancement for this new field before we move ahead on this.

/hold

Agreed, mapping to anything is not ideal, the problem is that ironic has ~700 config options and ironic-inspector has another ~400, in order to allow people to deal with special cases we either need to

  1. have some sort of pass through mechanism like this or
  2. add individual options for each one as it comes up with the delay thats involved to merge/release

as is we're seeing patches merged into the ironic image with options specific to CI, specific hardware and for debugging
metal3-io/ironic-image#172
metal3-io/ironic-image#160
metal3-io/ironic-image#167
metal3-io/ironic-image#150

some of these could be top-level platform configuration options but for corner cases I don't think we'd want to start creating options for every single one.

I can create an enhancement for this, I didn't intend on removing the hold on this PR until we came to a consensuses anyways.

@derekhiggins
Copy link
Contributor Author

derekhiggins commented Jul 15, 2020 •
edited
Loading

I'm not sure I like the idea of a top-level platform configuration for setting Ironic's internal configuration. It's exposing an implementation detail we prefer to keep hidden, if at all possible. If we have to make it available, it might belong tucked away in an Advanced struct member in the baremetal platform, with appropriate warnings.

Ack, will look into it

This also doesn't handle how we pass this through to MAO/CBO for the in-cluster Ironic images.

yup, I was going to look into that next

TBH, I think this change needs an enhancement request so we can get all the details worked out.

ack, will also look into creating one

@derekhiggins
Copy link
Contributor Author

this only works for ironic on the bootstrap node, holding while I look into how to also do it on master nodes.

We'll need to update the Provisioning CRD to support a mechanism to pass-through Ironic configuration. It'll be an API change. openshift/machine-api-operator#632 alters the API and makes use of it, not quite the same idea but it should give you the gist of how the change would look.

thanks, I was trying something similar, this should help me get it right.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign sdodson
You can assign the PR to them by writing /assign @sdodson 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

@derekhiggins derekhiggins mentioned this pull request Jul 16, 2020
Closed
@derekhiggins derekhiggins changed the title baremetal: Pass through of config variables to ironic [WIP] baremetal: Pass through of config variables to ironic Jul 16, 2020
@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 Jul 16, 2020
@openshift-ci-robot
Copy link
Contributor

@derekhiggins: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/verify-codegen db3a3c7 link /test verify-codegen
ci/prow/e2e-ovirt db3a3c7 link /test e2e-ovirt
ci/prow/e2e-libvirt db3a3c7 link /test e2e-libvirt
ci/prow/e2e-metal-ipi db3a3c7 link /test e2e-metal-ipi
ci/prow/e2e-aws-scaleup-rhel7 db3a3c7 link /test e2e-aws-scaleup-rhel7

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.

@derekhiggins
Copy link
Contributor Author

@stbenjam @abhinavdahiya I've started an enhancement here if ye have any thought I can update it,
openshift/enhancements#401

@derekhiggins
Copy link
Contributor 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
Reviewers

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

@stbenjam stbenjam stbenjam requested changes

@russellb russellb Awaiting requested review from russellb

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@derekhiggins @stbenjam @openshift-ci-robot @abhinavdahiya