Add Metal3 specific config items to BareMetalPlatformStatus #480

sadasu · 2019-10-17T14:57:10Z

Adding config items required for a Baremetal IPI deployment to
the BareMetalPlatformStatus CR. Based on enhancement request:
openshift/enhancements#90

openshift-ci-robot · 2019-10-17T14:57:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

OWNERS

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

dhellmann

I've suggested some wording updates to add/clarify some details. Let me know what you think.

dhellmann · 2019-10-23T15:41:44Z

config/v1/types_infrastructure.go

 	// to the nodes in the cluster.
 	NodeDNSIP string `json:"nodeDNSIP,omitempty"`
+
+	// Interface on a Baremetal server where the provisioning network is connected.


How about:

Suggested change

// Interface on a Baremetal server where the provisioning network is connected.

// The name of the network interface on a Baremetal server connected to the provisioning network.

dhellmann · 2019-10-23T15:43:49Z

config/v1/types_infrastructure.go

+	// Interface on a Baremetal server where the provisioning network is connected.
+	ProvisioningInterface string `json:"provisioningInterface"`
+
+	// IP address of the Provisioning interface.


This isn 't the IP of the interface. It's a VIP that moves with the provisioning service.

Suggested change

// IP address of the Provisioning interface.

// The VIP to be used by the provisioning service. This must be on the provisioning subnet, and outside of the DHCP range.

That's not strictly true, it's an IP which we use to configure the IP on whatever node runs the provisioning service, to me VIP implies loadbalancing/keepalived which is not the case here?

That's my misunderstanding, then. I thought that it was the IP that moved with the service if it was rescheduled. Maybe that's not the same thing as a VIP.

Yeah it's not actually a VIP, we the IP moves only because https://github.com/openshift/ironic-static-ip-manager gets moved as start of the pod reschedule, so we bring up the same IP on the new node, and the short lifetime means the previous node no longer uses it, assuming it's still running.

dhellmann · 2019-10-23T15:45:10Z

config/v1/types_infrastructure.go

+	// IP address of the Provisioning interface.
+	ProvisioningIp string `json:"provisioningIp"`
+
+	// IP address range from which Baremetal hosts can be assigned an IP.


Suggested change

// IP address range from which Baremetal hosts can be assigned an IP.

// IP address range on the provisioning subnet from which Baremetal hosts can be assigned an IP.

dhellmann · 2019-10-23T15:45:57Z

config/v1/types_infrastructure.go

+	// for faster downloads.
+	CacheUrl string `json:"cacheUrl"`
+
+	// Metal3 deployment specific config.


Suggested change

// Metal3 deployment specific config.

// URL for the RHCOS image for deploying new nodes in the cluster.

deads2k · 2019-10-23T18:55:31Z

config/v1/types_infrastructure.go

+
+	// provisioningIp is the VIP to be used by the provisioning service. This must be on the provisioning
+	// subnet, and outside of the DHCP range.
+	ProvisioningIp string `json:"provisioningIp"`


In the related Installer PR we have two install-config entries, ProvisioniningIP and ProvisioningNetworkCIDR

openshift/installer#2449

Is there a preference in terms of API conventions with whether we pass both ProvisioniningIP and ProvisioningNetworkCIDR here, or just ProvisioningIP and make that CIDR format, e.g append the subnet?

deads2k · 2019-10-23T18:55:44Z

config/v1/types_infrastructure.go

+	ProvisioningInterface string `json:"provisioningInterface"`
+
+	// provisioningIp is the VIP to be used by the provisioning service. This must be on the provisioning
+	// subnet, and outside of the DHCP range.


required or optional?

This should say just "IP" not VIP, it's used to bring up a NIC for provisioning.

The provisioning interface is probably required in this API I think, as although there are defaults specified in the underlying containers, it's likely the MAO will require these values to set up the ConfigMap @sadasu ?

OK, I thought this was the IP that floated around with the metal3 pod if it was rescheduled. Is that not the case?

@deads2k All the config that is being added here with the exception of CacheURL are required for a Baremetal IPI deployment.

@hardys ProvisioningInterface is a required parameter. We cannot makes any good assumptions about which interface on the physical server would be connected to the provisioning interface.

deads2k · 2019-10-23T18:56:25Z

config/v1/types_infrastructure.go

+	// subnet, and outside of the DHCP range.
+	ProvisioningIp string `json:"provisioningIp"`
+
+	// dhcpRange is the IP address range on the provisioning subnet from which Baremetal hosts


indicate format. I'm assuming CIDR, but I'm not sure.

Should this be "ProvisioningDHCPRange" given the Provisioning prefix for the Interface/IP values above?

Re the format, the underlying container expects a comma-separated pair of IPs, not a CIDR:

https://github.com/openshift/ironic-image/blob/master/rundnsmasq.sh#L6

We could however convert from a CIDR, either in the container or the MAO if needed?

deads2k · 2019-10-23T18:56:52Z

config/v1/types_infrastructure.go

+
+	// dhcpRange is the IP address range on the provisioning subnet from which Baremetal hosts
+	// can be assigned an IP address.
+	DhcpRange string `json:"dhcpRange"`


@sttts I can't remember, did we default optional or required?

Here optional https://github.com/openshift/api/blob/master/config/v1/doc.go#L5

deads2k · 2019-10-23T18:57:28Z

config/v1/types_infrastructure.go

+
+	// cacheUrl is the location where images have been previously downloaded and available
+	// for faster image downloads within the cluster.
+	CacheUrl string `json:"cacheUrl,omitempty"`


URL be consistent with the API in the same file

deads2k · 2019-10-23T18:57:55Z

config/v1/types_infrastructure.go

+
+	// dhcpRange is the IP address range on the provisioning subnet from which Baremetal hosts
+	// can be assigned an IP address.
+	DhcpRange string `json:"dhcpRange"`


if this is a CIDR, we use CIDR in the network API to describe this.

@deads2k Currently, it is comma separated pair of IP addresses where the 1st IP is the start of the range and the 2nd IP represents the end of the range. I can update the description to make this clear.

deads2k · 2019-10-23T18:58:09Z

config/v1/types_infrastructure.go

+
+	// cacheUrl is the location where images have been previously downloaded and available
+	// for faster image downloads within the cluster.
+	CacheUrl string `json:"cacheUrl,omitempty"`


describe what happens if this is missing.

If this configuration is missing, the image will be downloaded directly from the RHCOSImageURL.

deads2k · 2019-10-23T18:59:54Z

/hold

This needs an enhancement (https://github.com/openshift/enhancements) that includes descriptions of what you're replacing and how you plan to handle the migration and upgrade of existing data.

dhellmann · 2019-10-24T12:23:18Z

config/v1/types_infrastructure.go

+	CacheUrl string `json:"cacheUrl,omitempty"`
+
+	// rhcosImageUrl is the URL for RHCOS Image for deploying new nodes in the cluster.
+	RhcosImageUrl string `json:"rhcosImageUrl"`


Based on other comments, I assume this should be RHCOSImageURL? Do we need the "RHCOS" prefix in the name?

I have updated the name to be RHCOSImageURL. Having RHCOS as a prefix would make it clear what that URL points to but I am open to other naming options.

hardys · 2019-10-24T14:28:58Z

that includes descriptions of what you're replacing and how you plan to handle the migration and upgrade of existing data.

Can you expand on that requirement please @deads2k - this is a net-new integration for the currently experimental/unsupported baremetal IPI platform

Adding config items required for a Baremetal IPI deployment to the BareMetalPlatformStatus CR. Based on enhancement request: openshift/enhancements#90

hardys · 2019-10-28T10:07:25Z

config/v1/types_infrastructure.go

+
+	// dhcpRange is the IP address range on the provisioning subnet from which baremetal hosts
+	// can be assigned an IP address.
+	DHCPRange string `json:"dhcpRange"`


To make it clear what this is for should we call this ProvisioningDHCPRange?

Also I think a previous reviewer asked for clarification on the format here - currently we expect a comma-separated list of two IPs, so we should mention that in the comment.

Alternatively we could instead have two parameters (ProvisioningDHCPStart and ProvisioningDHCPEnd), or a CIDR to express the range, I don't really have a strong opinion on which although it would be nice to decide soon so we can align the related installer PR with the same interface.

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]>

sadasu · 2019-12-09T21:31:03Z

/close
These config items are being added to a new CR. Pull request for that effort : #540.

soltysh · 2019-12-12T16:23:38Z

Based on previous comment, looks like the bot didn't catch the command.

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 17, 2019

openshift-ci-robot requested review from abhinavdahiya and adambkaplan October 17, 2019 14:57

sadasu force-pushed the metal3-config branch from 93d9752 to 25b069a Compare October 17, 2019 15:10

sadasu changed the title ~~Add Metal3 specific config items to Baremetal Infrastructure status~~ WIP: Add Metal3 specific config items to Baremetal Infrastructure status Oct 17, 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 17, 2019

sadasu force-pushed the metal3-config branch from 25b069a to 816a419 Compare October 17, 2019 18:42

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 17, 2019

sadasu force-pushed the metal3-config branch 3 times, most recently from 03594a1 to 3e220c2 Compare October 23, 2019 15:34

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2019

dhellmann requested changes Oct 23, 2019

View reviewed changes

sadasu force-pushed the metal3-config branch 3 times, most recently from 10ddfd8 to dc13360 Compare October 23, 2019 18:28

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 23, 2019

sadasu changed the title ~~WIP: Add Metal3 specific config items to Baremetal Infrastructure status~~ Add Metal3 specific config items to Baremetal Infrastructure status Oct 23, 2019

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

deads2k reviewed Oct 23, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2019

dhellmann reviewed Oct 24, 2019

View reviewed changes

sadasu changed the title ~~Add Metal3 specific config items to Baremetal Infrastructure status~~ Add Metal3 specific config items to BareMetalPlatformStatus Oct 24, 2019

sadasu force-pushed the metal3-config branch from dc13360 to 3edd427 Compare October 24, 2019 19:21

Add Metal3 specific config items to BareMetalPlatformStatus

3601882

Adding config items required for a Baremetal IPI deployment to the BareMetalPlatformStatus CR. Based on enhancement request: openshift/enhancements#90

sadasu force-pushed the metal3-config branch from 3edd427 to 3601882 Compare October 28, 2019 05:49

sadasu mentioned this pull request Oct 28, 2019

WIP: Configure Metal3 from the installer openshift/installer#2449

Closed

hardys reviewed Oct 28, 2019

View reviewed changes

hardys mentioned this pull request Oct 30, 2019

Proposing enhancement to BareMetalPlatformStatus openshift/enhancements#90

Closed

soltysh closed this Dec 12, 2019

hardys mentioned this pull request Jan 9, 2020

Add baremetal provisioning configuration in a new CR #540

Closed

	// Interface on a Baremetal server where the provisioning network is connected.
	// The name of the network interface on a Baremetal server connected to the provisioning network.

	// IP address of the Provisioning interface.
	// The VIP to be used by the provisioning service. This must be on the provisioning subnet, and outside of the DHCP range.

	// IP address range from which Baremetal hosts can be assigned an IP.
	// IP address range on the provisioning subnet from which Baremetal hosts can be assigned an IP.

	// Metal3 deployment specific config.
	// URL for the RHCOS image for deploying new nodes in the cluster.

Add Metal3 specific config items to BareMetalPlatformStatus #480

Add Metal3 specific config items to BareMetalPlatformStatus #480

Uh oh!

Conversation

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

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 17, 2019

Uh oh!

dhellmann left a comment

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!

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!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

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

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!

deads2k commented Oct 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

sadasu commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sadasu commented Oct 17, 2019 •

edited

Loading

sadasu Oct 24, 2019 •

edited

Loading

sadasu Oct 24, 2019 •

edited

Loading

sadasu Oct 28, 2019 •

edited

Loading

sadasu commented Dec 9, 2019 •

edited

Loading