WIP: Configure Metal3 from the installer #2449

imain · 2019-10-02T22:15:41Z

Extend the baremetal platform fields to include provisioning information
for the pod started by the Machine API Operator. This is a work in progress
and I would appreciate feedback on this approach.

openshift-ci-robot · 2019-10-02T22:16:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

imain · 2019-10-02T22:17:59Z

To test with dev-scripts: openshift-metal3/dev-scripts#818

hardys · 2019-10-03T16:22:23Z

pkg/asset/manifests/infrastructure.go

The RHCOS image URL shouldn't be provided via the installConfig, it's generated from the data/data/rhcos.json - e.g see ac89074

To add more detail, I think you'll need to use a similar approach to https://github.com/openshift/installer/blob/master/pkg/asset/cluster/tfvars.go#L77 where "github.com/openshift/installer/pkg/asset/rhcos" gets imported, then you add new(rhcos.Image), to the Dependencies of this asset, and to the parents.Get in the Generate function

hardys · 2019-10-03T16:23:03Z

pkg/types/baremetal/platform.go

as above I don't think we should add the rhcos url here, instead we'll reference the platform.Image

hardys · 2019-10-03T16:27:01Z

pkg/types/baremetal/validation/platform.go

If this approach is acceptable we'll need to add actual validation for all of these similar to what I did in #2320

Yes just a POC. The tricky part is that a lot of these values don't work with the validations.. eg the provisioning IP is in the form of a CIDR, the interface doesn't exist so it errors out etc.

hardys

I think adding the install-config values is OK except for the RHCOS image, and perhaps we can default some of the *IP values based on the provisioningCIDR (but that can be a follow-up).

What I'd really like to see is how we then wire these values into the CRD or configmap for the MAO?

hardys · 2019-10-03T16:29:14Z

/label platform/baremetal

hardys · 2019-10-03T16:56:08Z

Ok so overall this looks good, we'll need feedback from the core installer team about the approach using the infrastructure manifest to provide this data to the MAO but it seems reasonable to me.

As a next step I'd suggest we fix the RHCOS image issue mentioned, defer adding the install-config validation, and create a corresponding MAO PR which demonstrates how this data will be used, then we can ask for more detailed feedback from the core reviewers.

hardys · 2019-10-03T17:02:16Z

Also we need defaults for all the new install-config variables in pkg/types/baremetal/defaults/platform.go

metal3ci · 2019-10-03T17:46:56Z

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1192/

metal3ci · 2019-10-03T19:09:15Z

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1193/

stbenjam · 2019-10-04T13:37:15Z

pkg/types/baremetal/validation/platform.go

What's the format of this? Something like "172.22.0.1-172.22.0.10"? For validation, it might be easier if it's two separate fields with a start, and end.

stbenjam · 2019-10-04T13:40:20Z

In general, the approach looks fine to me, but I think the fields need validation. The installer team is definitely going to ask for that. CIDR fields can be validated with https://golang.org/pkg/net/#ParseCIDR

imain · 2019-10-04T17:26:02Z

In general, the approach looks fine to me, but I think the fields need validation. The installer team is definitely going to ask for that. CIDR fields can be validated with https://golang.org/pkg/net/#ParseCIDR

Awesome thanks for that. It was the approach I wanted to check before diving in any further. Yeah we could do a start/end for the dhcp range too and test that it's in the network CIDR.

metal3ci · 2019-10-04T20:20:31Z

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1194/

hardys · 2019-10-24T10:28:01Z

pkg/types/baremetal/defaults/platform.go

I think this replaces the hard-coded default for ClusterProvisioningIP, and we should probably also calculate BootstrapProvisioningIP?

hardys

Looks good, a few changes needed I think @imain @sadasu if you'd like I can take care of them, would you prefer to give me push access to the fork or shall I create a new PR from my own repo?

hardys · 2019-10-24T12:43:47Z

We should also tag this as related to #2091 - we also need to either fix #2251 or update the templating of the startironic.sh script to consume these new values instead of hard-coding

hardys · 2019-10-25T09:30:58Z

pkg/types/baremetal/platform.go

This duplicates the existing ClusterProvisioningIP interface so I'll remove it

hardys · 2019-10-25T13:36:02Z

pkg/asset/manifests/infrastructure.go

cc @sadasu let me know if you'd prefer to move this to the MAO

I have modified https://github.com/openshift/api/pull/480/files to accept provisioningIP and provisioningNetworkCIDR as 2 seperate config items so that we do not add the CIDR to provisioningIP. As you had mentioned earlier, this is the more intuitive way of getting config.

metal3ci · 2019-10-25T14:01:17Z

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1271/

hardys · 2019-10-25T15:20:42Z

/retitle WIP: Configure Metal3 from the installer

sadasu · 2019-10-28T06:01:31Z

pkg/asset/manifests/infrastructure.go

openshift/api#480 currently assumes that the DHCP range is provided as comma separated IP address pair where the 1st IP address represents the start of the range and 2nd IP address represents the last address in the range. The enhancement request (openshift/enhancements#90) has been raised with the same format in mind.

Ack we can join that here - there was some open discussion on the API PR regarding the comma-separated format vs using a CIDR to express the DHCP range, I'll assume we're going with the comma-separated approach for now.

@sadasu another approach to resolve the API abiguity would be to also adopt explicit start/end parameters I guess, but I'll update this to align with the API PR so we can proceed with testing

Extend the baremetal platform fields to include provisioning information for the pod started by the Machine API Operator. Now includes defaults and the ability to set defaults based on just the network CIDR. Note this depends on openshift/api#480 Co-Authored-By: Steven Hardy <[email protected]>

openshift-ci-robot · 2019-10-28T13:57:37Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-disruptive	e62edfe8db377ee374c84e8260ed4ba43f424b3f	link	`/test e2e-aws-disruptive`
ci/prow/gofmt	`211cfd1`	link	`/test gofmt`
ci/prow/verify-vendor	`211cfd1`	link	`/test verify-vendor`
ci/prow/govet	`211cfd1`	link	`/test govet`
ci/prow/unit	`211cfd1`	link	`/test unit`
ci/prow/e2e-aws-scaleup-rhel7	`211cfd1`	link	`/test e2e-aws-scaleup-rhel7`
ci/prow/e2e-aws-upgrade	`211cfd1`	link	`/test e2e-aws-upgrade`
ci/prow/e2e-libvirt	`211cfd1`	link	`/test e2e-libvirt`
ci/prow/e2e-aws	`211cfd1`	link	`/test e2e-aws`
ci/prow/images	`211cfd1`	link	`/test images`
ci/prow/e2e-openstack	`211cfd1`	link	`/test e2e-openstack`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

metal3ci · 2019-10-28T16:37:25Z

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1277/

celebdor · 2019-11-12T13:44:50Z

pkg/asset/manifests/infrastructure.go

 		}
 	case baremetal.Name:
 		config.Status.PlatformStatus.Type = configv1.BareMetalPlatformType
+		// The MAO expects the ProvisioiningDHCPRange in comma-separated format


Suggested change

// The MAO expects the ProvisioiningDHCPRange in comma-separated format

// The MAO expects the ProvisioningDHCPRange in comma-separated format

celebdor · 2019-11-12T13:45:38Z

pkg/types/baremetal/defaults/platform.go

 	HardwareProfile         = "default"
 	APIVIP                  = ""
 	IngressVIP              = ""
+	ProvisioningInterface   = "ens3"


Why should there be an interface name default?

hardys · 2019-12-02T08:59:55Z

Note this is blocked on openshift/enhancements#119 since openshift/enhancements#90 and related API change this was based on got rejected

openshift-ci-robot · 2019-12-02T09:00:02Z

@imain: PR needs rebase.

Details

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.

hardys · 2019-12-16T10:06:26Z

@imain do you have a new version of this you can share?

As mentioned in #2758 I wonder if it'd be easier to approach this as either two PRs, or at least two commits - one which adds the install-config interfaces and templating for the boostrap hosted ironic, and another which writes out the CR based on openshift/api#540

I think that would simplify reviews/testing, and also enable us to fix #2091 which would be useful for some ongoing testing.

imain · 2019-12-17T18:18:13Z

Should have shortly. Sorry it's taking so long.

fabianofranz · 2020-01-13T15:02:45Z

Is there anything blocking this other than openshift/machine-api-operator#460?

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 2, 2019

openshift-ci-robot requested review from hardys and patrickdillon October 2, 2019 22:15

imain mentioned this pull request Oct 2, 2019

WIP: Add new items for configuring the BMO to the baremetal platform. openshift-metal3/dev-scripts#818

Closed

hardys reviewed Oct 3, 2019

View reviewed changes

pkg/types/baremetal/platform.go Outdated

Copy link

hardys Oct 3, 2019

Choose a reason for hiding this comment

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

as above I don't think we should add the rhcos url here, instead we'll reference the platform.Image

hardys reviewed Oct 3, 2019

View reviewed changes

hardys suggested changes Oct 3, 2019

View reviewed changes

openshift-ci-robot added the platform/baremetal IPI bare metal hosts platform label Oct 3, 2019

imain force-pushed the metal3-config branch from 1f28c47 to e62edfe Compare October 3, 2019 17:14

imain mentioned this pull request Oct 3, 2019

WIP: Get Baremetal config from the Infrastructure CR openshift/machine-api-operator#402

Closed

imain mentioned this pull request Oct 3, 2019

BareMetal deployment needs more configuration items. #2456

Closed

stbenjam reviewed Oct 4, 2019

View reviewed changes

imain force-pushed the metal3-config branch from e62edfe to b1ea822 Compare October 4, 2019 18:55

imain force-pushed the metal3-config branch from b1ea822 to 742c7c4 Compare October 11, 2019 03:45

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 11, 2019

imain force-pushed the metal3-config branch 2 times, most recently from a4b403f to 0d1903c Compare October 11, 2019 04:48

hardys reviewed Oct 24, 2019

View reviewed changes

hardys suggested changes Oct 24, 2019

View reviewed changes

hardys mentioned this pull request Oct 24, 2019

Add Metal3 specific config items to BareMetalPlatformStatus openshift/api#480

Closed

hardys reviewed Oct 25, 2019

View reviewed changes

pkg/types/baremetal/platform.go Outdated

Copy link

hardys Oct 25, 2019

Choose a reason for hiding this comment

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

This duplicates the existing ClusterProvisioningIP interface so I'll remove it

hardys force-pushed the metal3-config branch from 1440890 to 64cfc38 Compare October 25, 2019 13:27

hardys reviewed Oct 25, 2019

View reviewed changes

openshift-ci-robot changed the title ~~Configure Metal3 from the installer.~~ WIP: Configure Metal3 from the installer Oct 25, 2019

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 Oct 25, 2019

sadasu reviewed Oct 28, 2019

View reviewed changes

hardys force-pushed the metal3-config branch from 64cfc38 to 211cfd1 Compare October 28, 2019 13:51

hardys mentioned this pull request Oct 30, 2019

Proposing enhancement to BareMetalPlatformStatus openshift/enhancements#90

Closed

celebdor suggested changes Nov 12, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2019

stbenjam mentioned this pull request Dec 5, 2019

[WIP] baremetal: make dhcp range configurable for bootstrap node #2758

Closed

hardys mentioned this pull request Dec 9, 2019

Bug 1784624: baremetal: Enable the baremetal platform to override the default rhcos images #2757

Merged

hardys mentioned this pull request Dec 17, 2019

Add baremetal provisioning configuration in a new CR openshift/api#540

Closed

hardys mentioned this pull request Jan 14, 2020

can we have make master and make worker openshift-metal3/dev-scripts#889

Closed

imain closed this Jan 14, 2020

stbenjam mentioned this pull request Feb 10, 2020

Adding IPv6 Troubleshooting guide #3088

Closed

	// The MAO expects the ProvisioiningDHCPRange in comma-separated format
	// The MAO expects the ProvisioningDHCPRange in comma-separated format

WIP: Configure Metal3 from the installer #2449

WIP: Configure Metal3 from the installer #2449

Uh oh!

Conversation

imain commented Oct 2, 2019

Uh oh!

openshift-ci-robot commented Oct 2, 2019

Uh oh!

imain commented Oct 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardys left a comment

Choose a reason for hiding this comment

Uh oh!

hardys commented Oct 3, 2019

Uh oh!

hardys commented Oct 3, 2019

Uh oh!

hardys commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metal3ci commented Oct 3, 2019

Uh oh!

metal3ci commented Oct 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stbenjam commented Oct 4, 2019

Uh oh!

imain commented Oct 4, 2019

Uh oh!

metal3ci commented Oct 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardys left a comment

Choose a reason for hiding this comment

Uh oh!

hardys commented Oct 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metal3ci commented Oct 25, 2019

Uh oh!

hardys commented Oct 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Oct 28, 2019

Uh oh!

metal3ci commented Oct 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardys commented Dec 2, 2019

Uh oh!

openshift-ci-robot commented Dec 2, 2019

Uh oh!

hardys commented Dec 16, 2019

Uh oh!

imain commented Dec 17, 2019

hardys commented Oct 3, 2019 •

edited

Loading