Add baremetal provisioning configuration in a new CR #540

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

Add a new CR within operator scope or all the provisioning config items
needed by metal3 to provision baremetal servers.
Implements : https://github.com/openshift/enhancements/blob/master/enhancements/baremetal/baremetal-provisioning-config.md

openshift-ci-robot · 2019-12-09T21:17:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

hardys · 2019-12-16T10:01:31Z

operator/v1/types_baremetal.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,omitempty"`


This needs an update to match the ProvisioningDHCP interface we finally settled on in openshift/enhancements#119

I'd expect this type to also have OperatorSpec since it has several crucial elements that every operator's spec should have.

hardys · 2019-12-16T10:02:02Z

@sadasu can this get an update to unblock the related MAO and installer work?

sadasu · 2019-12-16T15:09:22Z

@sadasu can this get an update to unblock the related MAO and installer work?

Yes, I have been working on it since the enahncement merged. I working through some issues in my code. Will update with latest code shortly.

soltysh · 2019-12-16T15:39:45Z

operator/v1/types_baremetal.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,omitempty"`


I'd expect this type to also have OperatorSpec since it has several crucial elements that every operator's spec should have.

sadasu · 2019-12-16T18:04:53Z

@abhinavdahiya @enxebre @soltysh here is my WIP implementation for https://github.com/openshift/enhancements/blob/master/enhancements/baremetal/baremetal-provisioning-config.md.

I still have some TODOs which I am hoping to get some help with:

I have not added "// +genclient:nonNamespaced", so the CRD would be namespaced. But, I am not sure where to specify the namespace.
Since this is a singleton CR, should I be adding the *List struct at the end of types_baremetal.go? Automated verification does not pass if I don't add the *List struct.
At what point will the YAML file for the CRD be generated?

Thanks!

soltysh · 2019-12-17T09:13:38Z

I have not added "// +genclient:nonNamespaced", so the CRD would be namespaced. But, I am not sure where to specify the namespace.

Have a look at one of the pre-existing CRDs definitions if in doubt where to put stuff, https://github.com/openshift/api/blob/master/operator/v1/types_scheduler.go, for example. Namespace is coming from metav1.ObjectMeta which you already have.

Since this is a singleton CR, should I be adding the *List struct at the end of types_baremetal.go? Automated verification does not pass if I don't add the *List struct.

List struct is to enable passing the resource and retrieving it through regular GET operation when you don't specify name. It should be there, look at others again, most if not all our operators' CRs exists as a single instance.

At what point will the YAML file for the CRD be generated?

No, you need to provide the initial YAML for the CRD, the generation will update its contents with the validation parts, only.

stbenjam · 2019-12-17T14:36:13Z

operator/v1/types_baremetal.go

+type ProvisioningDHCP struct {
+	// managementState indicates whether the DHCP service would be started within
+	// the metal3 cluster or would use an external DHCP service
+	ManagementState ManagementState `json:"managementState"`


I think instead of adding the ManagementState fields, we could embed the OperatorSpec struct instead

Suggested change

ManagementState ManagementState `json:"managementState"`

OperatorSpec `json:",inline"`

See: https://golang.org/doc/effective_go.html#embedding

if you need an operatorspec, that sounds good, but please place it directly in spec.

I am trying to follow what was agreed upon in https://github.com/openshift/enhancements/blob/master/enhancements/baremetal/baremetal-provisioning-config.md.

I'll add OperatorSpec instead of just adding ManagementState since we could use the additional fields in OperatorSpec.

I am not clear about adding OperatorSpec directly into the Spec struct for the new CR. According to the proposal that just merged, we were going to add another struct just for the DHCP fields : ManagementState (which is now OperatorSpec) and DHCP Range. If we move the OperatorSpec directly to the Spec, then we do not need a separate ProvisioningDHCP struct and add the DHCP range also directly to the Spec.

if you need an operatorspec, that sounds good, but please place it directly in spec.

What do you mean by "directly in the spec"? Why not embed it in ProvisioningDHCP? This is an OperatorSpec for the DHCP service and originates from the discussion we had in the enhancements PR, as it fits that model.

This was also suggested by @soltysh #540 (comment)

@deads2k I can see why OperatorSpec could be added directly in the spec. In our case, we are using the ManagementState within the OperatorSpec to mean something specific about the state of the DHCP service. This value combined with the DHCPRange represents the complete DHCP configuration. So, for that reason we really cannot embed it in the Spec and have to leave it within the ProvisioningDHCP struct.

enxebre · 2019-12-17T14:48:54Z

operator/v1/register.go

 		&ServiceCatalogControllerManagerList{},
 		&IngressController{},
 		&IngressControllerList{},
+		&Metal3ProvisioningController{},


Why is this called controller?
IngressController is the per object config which instantiates a new specific ingress controller pod which is not the case here.

I think this should be called just Metal3Provisioning, Metal3ProvisioningConfig or similar.

It's config for the Metal3 provisioning controller, and related containers - Metal3ProvisioningConfig sounds fine to me, but we already went over some of this in the enhancement PR - @sadasu were there specific reasons for the current naming?

@enxebre @shardy the naming was left partially unresolved in the enhancement proposal. I think Metal3Provisioning is a better name for us. Making the necessary changes.

deads2k · 2019-12-17T15:50:34Z

operator/v1/types_baremetal.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// +genclient


The enhancment talks about creating these only in one namespace. Why have a namespaced object that can only be created in a single namespace by a privileged user?

Agreed. This can be made Non-namespaced.

Yes. At the time of writing the enhancement proposal, it appeared that having the CR Namespaced was not required but harmless.

update to make non-namespaced

operator/v1/types_baremetal.go

deads2k · 2019-12-17T16:06:20Z

operator/v1/types_baremetal.go

+type Metal3ProvisioningControllerSpec struct {
+	// provisioningInterface is the name of the network interface on a Baremetal server connected
+	// to the provisioning network.
+	ProvisioningInterface string `json:"provisioningInterface"`


@damemi we need to explicitly say required in our repo, right?

@deads2k yup, because we set operator/v1 to default optional at the package level https://github.com/openshift/api/blob/master/operator/v1/doc.go#L5

@damemi we need to explicitly say required in our repo, right?

@damemi Would adding this "// +kubebuilder:object:root=true" take care of that?

operator/v1/types_baremetal.go

deads2k · 2019-12-17T16:07:18Z

operator/v1/types_baremetal.go

+	ProvisioningInterface string `json:"provisioningInterface"`
+
+	// provisioningIP is the IP address assigned to the provisioningInterface for provisioning
+	// the baremetal node. This IP address should be within the provisioning subnet, and


where is this subnet defined, is there validation to ensure this?

It will be defined in the installer, and there is WIP (which depends on this PR) to ensure validation of the IP specified:

openshift/installer#2449

Also related is openshift/installer#2091 and openshift/installer#2320

deads2k · 2019-12-17T16:08:54Z

operator/v1/types_baremetal.go

+
+	// provisioningNetworkCIDR is the network on which the baremetal nodes are provisioned.
+	// The provisioningIP and the IPs in the dhcpRange all come from within this network.
+	ProvisioningNetworkCIDR string `json:"provisioningNetworkCIDR"`


Is this informative? What happens if not all my baremetal servers agree?

This may go back to what is the scoping of this resource: is it one per machine or one per type of machine or something else? The IP defined above suggests its one per machine,.

Currently it is one per cluster, we start only one metal3 deployment via the MAO, and this config data is used as input to that deployment.

That deployment is used to deploy all machines on a common provisioning subnet.

The ProvisioningIP is the IP on this subnet where the provisioning services run (those hosted by the metal3 deployment configured by this resource). It's not one per machine, that is a cluster-wide IP.

Can this IP change? Is this used as input to force that IP? It doesn't sound like it. It sounds like this is an IP that is used to tell something else where to connect to an already existent system. In which case this is status, not spec.

Yes, the provisioning network already exists. But, the IP address to use during the provisioning process is not known ahead of time. So, it should be provided by the admin.

My understanding is that fields in the Metal3ProvisioningStatus should be updated by someone/something other than the user/admin based on current state of the metal3 cluster. There is no way for metal3 to "discover" the provisioning network details. So, how can it update the Status?

This is data provided at install time via openshift-install, which is then used by the metal3 provisioning components to configure networking appropriately.

This will always be environment specific (so we need input from the human admin at install time), and at this time we've not considered changing any of the values as a day-2 operation, but in theory we could have that requirement in future (particularly for the DHCP Range which you might need to e.g expand after initial install).

This is not an IP used to tell something where to connect, it's configuration data we use to start the cluster hosted provisioning services, e.g we're starting the service using this data, which is later connected to (the baremetal controller and the provisioning services run in the same pod, started by the MAO).

deads2k · 2020-01-07T19:04:45Z

operator/v1/types_metal3provisioning.go

+	// provisioningIP is the IP address assigned to the provisioningInterface of
+	// the baremetal server. This IP address should be within the provisioning
+	// subnet, and outside of the DHCP range.
+	ProvisioningIP string `json:"provisioningIP"`


provisioningTemporaryBootstrapIP please

That will make it more confusing - there is already a provisioning bootstrap IP (that is temporary) configured on the bootstrap VM, that isn't what this is - it's the permanent provisioning IP of the cluster-hosted provisioning services.

deads2k · 2020-01-07T19:05:44Z

operator/v1/types_metal3provisioning.go

+
+	// provisioningIP is the IP address assigned to the provisioningInterface of
+	// the baremetal server. This IP address should be within the provisioning
+	// subnet, and outside of the DHCP range.


explain why it's temporary, and reused.

It's not temporary, as mentioned above

My understanding is that the godoc needs to have enough guidance so that the user can provide correct values for these fields. Why do we need to explain the underlying design here within the godoc?

deads2k · 2020-01-07T19:47:36Z

operator/v1/types_metal3provisioning.go

+	// provisioningNetworkCIDR is the network on which the baremetal nodes are
+	// provisioned. The provisioningIP and the IPs in the dhcpRange all come from
+	// within this network.
+	ProvisioningNetworkCIDR string `json:"provisioningNetworkCIDR"`


This isn't required for a first cut. Let's drop this for now.

Sorry what do you mean? This is absolutely critical to have

Agreed, we've already pared this down to the absolute minimum in the $many review iterations of openshift/enhancements#119 - at this point we need to implement what was agreed, and IMHO it's not reasonable to start redefining the interface arbitrarily at this point.

deads2k · 2020-01-07T19:47:41Z

operator/v1/types_metal3provisioning.go

+	// server is internal or external to the metal3 cluster. If it is internal,
+	// the second parameter represents the DHCP address range to be provided
+	// to the baremetal hosts.
+	ProvisioningDHCP ProvisioningDHCP `json:"provisioningDHCP"`


This isn't required for a first cut. Let's drop this for now.

Likewise - we really must have configurable network and configurable DHCP in 4.4.

Agreed, lets stop trying to randomly remove things we already discussed and agreed in the enhancement please @deads2k

deads2k · 2020-01-07T19:49:22Z

operator/v1/types_metal3provisioning.go

+
+// Metal3ProvisioningStatus defines the observed status of Metal3Provisioning.
+type Metal3ProvisioningStatus struct {
+	OperatorStatus `json:",inline"`


this isn't needed because this is config.

@deads2k if this is config then it should reside under config.openshift.io not next to other operators.

deads2k · 2020-01-07T20:00:11Z

I'd like to see the DHCP aspects deferred so we can get this unstuck. After that, this is entirely config related (fields indicate infrastructure managed out of band), so this belongs in config.openshift.io. The only other "obvious" home for this is in the machine.openshift.io. As I understand it, this is only consumed by the the machine-api-operator.

@enxebre why doesn't this live in machine.openshift.io?

Before it is reintroduced, the DHCP configuration needs to consider:

is it required for every Metal3Provisioning? I can imagine cases where different classes of machines have different provisioningNetworkInterfaces and different provisioningTemporaryBootstrapIPs so you can scale multiple at a time, but I wouldn't expect DHCP ranges to vary.
when every machine in a cluster stops, how does a container based dhcp server allow for rebootstrapping master nodes and kubelets which rely upon the internal load balancer for contact to a kube-apiserver to get the pod listing for the dhcp pod.

smarterclayton · 2020-01-07T20:12:55Z

operator/v1/types_metal3provisioning.go

+// consumed by the machine-api-operator to bring up and update containers
+// in a metal3 cluster.
+//
+type Metal3Provisioning struct {


I don't think this is an appropriate name for this object. Metal3 is not a human facing thing - it's a project name. This either belongs in an object named Provisioning in the metal3.io group, or should be human focused here BareMetalProvisioning

When we run "oc get pods" we do see the metal3 -xxx-xx pod running. So, I assumed it would be OK to use/re-use metal3 here.

smarterclayton · 2020-01-07T20:14:17Z

This feels either like a cluster infrastructure spec / status field, or something in a separate API group like BareMetalProvisioning in metal3.io.

deads2k · 2020-01-07T21:18:41Z

cluster infrastructure spec

Speaking to the developer, it doesn't fit there because there is a thought that there could be multiple of these in the future and our infrastructure is very much "one of". This is tightly associated to machines.

soltysh · 2020-01-07T21:11:52Z

operator/v1/0000_30_metal3provisioning_00_config.crd.yaml

+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  creationTimestamp: null


Not needed, drop.

soltysh · 2020-01-07T21:13:04Z

operator/v1/0000_30_metal3provisioning_00_config.crd.yaml

+  names:
+    kind: Metal3Provisioning
+    listKind: Metal3ProvisioningList
+    singular: metal3provisioning


I'm missing plural here.

soltysh · 2020-01-07T21:19:25Z

operator/v1/0000_30_metal3provisioning_00_config.crd.yaml

+    kind: Metal3Provisioning
+    listKind: Metal3ProvisioningList
+    singular: metal3provisioning
+  scope: ""


Cluster, especially that you're setting nonNamespaced in the go code.

soltysh · 2020-01-07T21:23:11Z

operator/v1/types_metal3provisioning.go

+	// managed within the metal3 cluster.
+	// The other fields of OperatorSpec retain their existing
+	// semantics.
+	OperatorSpec `json:",inline"`


Based on a later comment from @deads2k this should not be needed, my bad from an earlier review, where I understood this is an operator. On the other hand if this is config, then it should land under config.openshift.io not here.

soltysh · 2020-01-07T21:23:57Z

operator/v1/types_metal3provisioning.go

+type Metal3ProvisioningSpec struct {
+	// provisioningInterface is the name of the network interface on a Baremetal
+	// server to the provisioning network. It can have values like "eth1" or "ens3".
+	ProvisioningInterface string `json:"provisioningInterface"`


Why not just networkInterface if this is already part of Metal3ProvisioningSpec.

soltysh · 2020-01-07T21:24:59Z

operator/v1/types_metal3provisioning.go

+
+// Metal3ProvisioningStatus defines the observed status of Metal3Provisioning.
+type Metal3ProvisioningStatus struct {
+	OperatorStatus `json:",inline"`


@deads2k if this is config then it should reside under config.openshift.io not next to other operators.

stbenjam · 2020-01-07T21:35:42Z

I'd like to see the DHCP aspects deferred so we can get this unstuck. After that, this is entirely config related (fields indicate infrastructure managed out of band), so this belongs in config.openshift.io. The only other "obvious" home for this is in the machine.openshift.io. As I understand it, this is only consumed by the the machine-api-operator.

@enxebre why doesn't this live in machine.openshift.io?

Before it is reintroduced, the DHCP configuration needs to consider:

I don't think it should be removed, in order to create a fully functioning Metal3 deployment, we need to wire in values from the installer into the first Metal3 pod, and that must include the DHCP configuration (whether it's turned on or not, and what network ranges to use).

Landing part of this change doesn't make a lot of sense, TBH, as we can't make use of the CR until it's complete. This PR is already the minimum set of changes we required to implement https://github.com/openshift/enhancements/blob/master/enhancements/baremetal/baremetal-provisioning-config.md

1. is it required for every `Metal3Provisioning`?  I can imagine cases where different classes of machines have different provisioningNetworkInterfaces and different provisioningTemporaryBootstrapIPs so you can scale multiple at a time, but I wouldn't expect DHCP ranges to vary.

There's only envisioned to be one Metal3Provisioning today, but yes it's possible there could be multiple in the future. The DHCP ranges would always (I think) vary since it's inherently tied to the entire Metal3 pod. If you have a separate Metal3 environment, it needs it's own L2 network for provisioning.

2. when every machine in a cluster stops, how does a container based dhcp server allow for rebootstrapping master nodes and kubelets which rely upon the internal load balancer for contact to a kube-apiserver to get the pod listing for the dhcp pod.

To this point, I think there's a misunderstanding here - there are two networks used for baremetal IPI. It's described in https://github.com/openshift/installer/blob/master/docs/user/metal/install_ipi.md.

The provisioning network is only provisioning, it's how new hosts PXE boot and become nodes. The control plane isn't reliant on that DHCP server being available after it itself is provisioned during bootstrapping.

enxebre · 2020-01-08T11:00:17Z

@enxebre why doesn't this live in machine.openshift.io?

Context invariants (please any bare metal team folk prove me wrong here):

At high level this is the information that enables bare metal as an underlying "cloud platform" in the cluster for any API driven consumer. This is to fill the gap in bare metal that a public cloud provider gives you out of the box.
This it is a singleton CR.
This purely configuration.
This needs to be populated with user input.

Based on this I initially suggested for this to live under config.openshift.io, either as BareMetalPlatformStatus so under infrastructure.config.openshift.io or in its own specific subdomain e.g provisioning.baremetal.config.openshift.io
@abhinavdahiya legitimately disagreed as "this information is only used for one service i.e the provisioning network for PXEing machines. And therefore i think it best belongs in a specific operator config."
openshift/enhancements#90 (comment)

Based on all the above, if we think .config.openshift.io is still not a good fit because this is too specific and .operator.openshift.io is not a good fit because this is purely config, I think this should belong to metal3.io e.g provisioning.metal3.io consitently with other bare metal resources e.g
https://github.com/openshift/machine-api-operator/blob/master/install/0000_30_machine-api-operator_08_baremetalhost.crd.yaml#L4 and it would live in this repo under metal3/v1 or may be baremetal/v1, I think https://github.com/openshift/api/tree/master/network is a good analogy.

cc @smarterclayton @deads2k @sadasu

hardys

@deads2k /cc @smarterclayton @russellb

I'd like to see the DHCP aspects deferred so we can get this unstuck.

This absolutely cannot happen - we need to configure this network e.g for ipv6 provisioning, or to configure a provisioning subnet that is always going to be customer environment specific please can we stop trying to arbitrarily redefine this interface without any understanding of the functional impact.

We've spent the time to get openshift/enhancements#119 approved, so it seems unreasonable to start redefining that here - we already stripped back these configuration items to the bare minimum so can we just resolve the naming nits and move this forward?

How can we work together to reach a better shared understanding of this work, including the priority as this is a critical blocker for baremetal IPI end users?

mfojtik · 2020-01-08T14:50:27Z

operator/v1/types_metal3provisioning.go

+
+	// spec is the specification of the desired behavior of the
+	// Metal3Provisioning.
+	Spec Metal3ProvisioningSpec `json:"spec,omitempty"`


why is Spec omitempty?

sadasu · 2020-01-08T19:49:31Z

@enxebre Regarding your comment #540 (comment), we have no issue adding this CR to (metal3 or baremetal)/v1 in the openshift/api repo. We have never discussed adding this new CRD to MAO just like https://github.com/openshift/machine-api-operator/blob/master/install/0000_30_machine-api-operator_08_baremetalhost.crd.yaml. Do you forsee any issues?

deads2k · 2020-01-08T21:09:59Z

@enxebre Regarding your comment #540 (comment), we have no issue adding this CR to (metal3 or baremetal)/v1 in the openshift/api repo. We have never discussed adding this new CRD to MAO just like https://github.com/openshift/machine-api-operator/blob/master/install/0000_30_machine-api-operator_08_baremetalhost.crd.yaml. Do you forsee any issues?

I've spoken with the install team and I think this is tractable. I'd like to confirm that you (@sadasu and team) own the metal3.io apigroup and I would suggest making a subgroup like provisioning.metal3.io. My thoughts on the path forward run like this:

close this PR
open PR to MAO near https://github.com/openshift/machine-api-operator/blob/master/install/0000_30_machine-api-operator_08_baremetalhost.crd.yaml adding a metal3config (or other name of your choice) resource in provisioning.metal3.io. Be sure that you correctly handle upgrade/downgrade scenarios (@enxebre )
the dynamic client (https://github.com/kubernetes/client-go/tree/master/dynamic) can be used to read this new resource and there are getters in https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/unstructured/helpers.go to get values out
the installer will provide advice about how to wire to this resource. This may or may not be embedded code, but the advice won't be based on what API group this resource is in. (@sdodson @crawford @fabianofranz ).

This path

co-locates the API with usage and keeps it in an API group tightly associated with the binary operand driving its shape. This helps discoverability, reactivity, and ownership. Questions will find the right owners faster.
self-determination of what information needs to be present. It places the point of control closer to where it needs to be.
self-determination of API evolution and versioning. I suspect that your API will evolve. Rather than force choices now, you can move forward now. You will have to be aware that changes you make to your API will impact your code in the installer and may have impacts on upgrade/downgrade.
the possibility of future clean layering between MAO, CVO, installer, and metal3 provisioning apiserver. The idea of CVO profiles lends itself to a future where separate layers can be optionally enabled based on installer criteria. I think it's likely this ends up benefiting from that capability.

I reserve the right to edit this comment with more detail because I want to be sure I get something out before the EOD, but my rough sketch leading to this was....

the metal3 provisioning api belongs in its own group because:

it is providing cloud provider infrastructure that is tightly coupled to the implementation of a single variant of a cloud provider
it is logically direct configuration values for an upstream project which already has its own API group
it is tightly coupled to its upstream binary, so it makes sense to keep them closely located to each other

discounted arguments for moving it openshift/api

the machine-api-operator is creating the resource - this is a quirk of not having a second-level-operator because we lack CVO profiles at the moment
desire for an openshift/client-go - a dynamic client can be used or a different client generated
there could be an adapter from install config to resource manifest - that adapter can be done agnostic of the API group

Before anyone gets really bent of shape, find me on slack. I think I'm in channels with most of you.

hardys · 2020-01-09T09:09:48Z

Thanks for the detailed response @deads2k - it's unfortunate that we didn't identify this approach in any of the previous PRs (openshift/installer#2149 #480 openshift/enhancements#90 and openshift/enhancements#119 - several of which you, @enxebre @wking @abhinavdahiya and others participated in) - this is the fourth(!) redirection on approach we've encountered so it's been an inefficient process to say the least.

Perhaps when this is all done we can have some kind of retrospective to figure out how to avoid such wasted effort and miscommunication in future. Anyway some replies/questions below:

open PR to MAO near https://github.com/openshift/machine-api-operator/blob/master/install/0000_30_machine-api-operator_08_baremetalhost.crd.yaml adding a metal3config (or other name of your choice) resource in provisioning.metal3.io. Be sure that you correctly handle upgrade/downgrade scenarios (@enxebre )

Co-locating the CRD and the component that consumes it makes sense to me, provided that is acceptable to the MAO team - it can publish an interface which is basically a more strongly defined alternative to the current ConfigMap approach.

the dynamic client (https://github.com/kubernetes/client-go/tree/master/dynamic) can be used to read this new resource and there are getters in https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/unstructured/helpers.go to get values out

I'm still not clear why we need the dynamic client - can't the regular openshift client in the MAO just look for a resource with the expected type while reconciling? Then the installer can instantiate CRs of this type, the same way it already does for the BareMetalHost resources mentioned above?

the installer will provide advice about how to wire to this resource. This may or may not be embedded code, but the advice won't be based on what API group this resource is in. (@sdodson @crawford @fabianofranz ).

To be clear the intention is absolutely for this to be embedded in code in the installer, we want the user of the installer to provide these details via the install-config, we'll then use them to configure the metal3 components on the masters (e.g by writing this new CR), so we can deploy workers without additional user interaction (mirroring the installer experience on all other platforms). openshift/installer#2449 is the latest attempt to do this, but it's been blocked for $months now on agreement around this interface.

imain · 2020-01-09T23:19:31Z

This patch openshift/installer#2849 is using this API change to create a CRD/CR, or refer to the link hardys posted for the previous version that used the baremetal platform status.

imain

Looks good IMO Sandhya :) Just a few nits/questions.

imain · 2020-01-09T23:21:20Z

operator/v1/0000_30_metal3provisioning_00_config.crd.yaml

+  validation:
+    openAPIV3Schema:
+      description: "Metal3Provisioning contains configuration used by the Provisioning
+        service (Ironic) to provision baremetal hosts. \n Metal3Provisioning is created


Just curious, do we actually need the \n's here? None of the other longer texts have them..

imain · 2020-01-09T23:22:09Z

operator/v1/0000_30_metal3provisioning_00_config.crd.yaml

+                    range is taken to be the default range which goes from .10 to
+                    .100 of the ProvisioningNetworkCIDR. This is the only value in
+                    all of the provisioning configuration that can be changed after
+                    the installer has created the CR.


I would mention that the dhcp range is two IPs separated by a comma.

imain · 2020-01-09T23:25:37Z

operator/v1/types_metal3provisioning.go

+	// goes from .10 to .100 of the ProvisioningNetworkCIDR. This is the only
+	// value in all of the provisioning configuration that can be changed
+	// after the installer has created the CR.
+	DHCPRange string `json:"DHCPRange,omitempty"`


We can get this updated after deployment? :) Do we actually have the machinery there to do this? Again I'd mention that it's 2 comma separated IPs

stbenjam · 2020-01-10T19:16:17Z

Could we get a short summary here of the agreement to move this to MAO and close this PR so it's clear what the path forward on this is?

sadasu · 2020-01-10T19:20:43Z

Based on comment #540 (comment) , we have decided to add a CRD to the MAO. The PR for this work is here : openshift/machine-api-operator#460.

Work listed in 3 & 4 in the path forward, will be in future PRs to MAO and installer respectively.

This is so baremetal can fetch the provisioning.metal3.io object and parametarise their deployment. Related openshift/api#540 and openshift#460

Implementation of the Baremetal provisioning CR changed based on comment openshift/api#540 (comment) after this original enhancement proposal merged. Updating the proposal to reflect the current implementation.

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 Dec 9, 2019

openshift-ci-robot requested review from abhinavdahiya and adambkaplan December 9, 2019 21:17

This was referenced Dec 9, 2019

Add UseMDNS member to baremetal platform status #522

Closed

Add Metal3 specific config items to BareMetalPlatformStatus #480

Closed

sadasu force-pushed the metal3-new-CR branch 2 times, most recently from 448c442 to aa9604c Compare December 9, 2019 21:44

hardys reviewed Dec 16, 2019

View reviewed changes

This was referenced Dec 16, 2019

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

Closed

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

Closed

soltysh suggested changes Dec 16, 2019

View reviewed changes

sadasu force-pushed the metal3-new-CR branch from aa9604c to 4ba76a3 Compare December 16, 2019 17:50

sadasu force-pushed the metal3-new-CR branch from 4ba76a3 to d4c2393 Compare December 16, 2019 18:14

sadasu changed the title ~~WIP: Add baremetal provisioning configuration in a new CR~~ Add baremetal provisioning configuration in a new CR Dec 16, 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 Dec 16, 2019

stbenjam reviewed Dec 17, 2019

View reviewed changes

enxebre reviewed Dec 17, 2019

View reviewed changes

deads2k reviewed Dec 17, 2019

View reviewed changes

operator/v1/types_baremetal.go Outdated Show resolved Hide resolved

deads2k reviewed Dec 17, 2019

View reviewed changes

operator/v1/types_baremetal.go Outdated Show resolved Hide resolved

deads2k reviewed Dec 17, 2019

View reviewed changes

operator/v1/types_baremetal.go Outdated Show resolved Hide resolved

deads2k reviewed Dec 17, 2019

View reviewed changes

deads2k reviewed Jan 7, 2020

View reviewed changes

smarterclayton reviewed Jan 7, 2020

View reviewed changes

soltysh suggested changes Jan 7, 2020

View reviewed changes

hardys reviewed Jan 8, 2020

View reviewed changes

mfojtik reviewed Jan 8, 2020

View reviewed changes

stbenjam mentioned this pull request Jan 9, 2020

Bug 1796929: baremetal: make provisioning networks more configurable openshift/installer#2895

Merged

imain reviewed Jan 9, 2020

View reviewed changes

sadasu closed this Jan 10, 2020

enxebre mentioned this pull request Jan 16, 2020

Expose a dynamic client in the Operator struct openshift/machine-api-operator#469

Merged

sadasu mentioned this pull request Feb 19, 2020

Update the Baremetal Provisioning CRD enhancement proposal to match implementation openshift/enhancements#219

Merged

hardys mentioned this pull request Jun 24, 2020

baremetal: enhancement for optional provisioning network openshift/enhancements#386

Merged

	ManagementState ManagementState `json:"managementState"`
	OperatorSpec `json:",inline"`

Add baremetal provisioning configuration in a new CR #540

Add baremetal provisioning configuration in a new CR #540

Uh oh!

Conversation

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

Uh oh!

Uh oh!

openshift-ci-robot commented Dec 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardys commented Dec 16, 2019

Uh oh!

sadasu commented Dec 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

soltysh commented Dec 17, 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!

enxebre Dec 17, 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!

sadasu Dec 17, 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!

Uh oh!

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!

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

sadasu commented Dec 9, 2019 •

edited

Loading

sadasu commented Dec 16, 2019 •

edited

Loading

enxebre Dec 17, 2019 •

edited

Loading

sadasu Dec 17, 2019 •

edited

Loading

stbenjam Jan 7, 2020 •

edited

Loading