Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 185 additions & 0 deletions operator/v1/0000_30_metal3provisioning_00_config.crd.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
creationTimestamp: null
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed, drop.

name: metal3provisioning.operator.openshift.io
spec:
group: operator.openshift.io
names:
kind: Metal3Provisioning
listKind: Metal3ProvisioningList
singular: metal3provisioning
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing plural here.

scope: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

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

preserveUnknownFields: false
subresources:
scale:
labelSelectorPath: .status.selector
specReplicasPath: .spec.replicas
statusReplicasPath: .status.availableReplicas
status: {}
validation:
openAPIV3Schema:
description: "Metal3Provisioning contains configuration used by the Provisioning
service (Ironic) to provision baremetal hosts. \n Metal3Provisioning is created
Copy link

Choose a reason for hiding this comment

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

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

by the Openshift installer using admin or user provided information about
the provisioning network and the NIC on the server that can be used to PXE
boot it. \n This CR is a singleton, created by the installer and currently
only consumed by the machine-api-operator to bring up and update containers
in a metal3 cluster."
type: object
required:
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
description: spec is the specification of the desired behavior of Metal3Provisioning.
type: object
properties:
provisioningInterface:
description: "provisioningInterface is the name of the network interface
on a baremetal server to the provisioning network. It can have values
like eth1 or ens3."
type: string
provisioningIP:
description: "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."
type: string
provisioningNetworkCIDR:
description: "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."
type: string
provisioningDHCP:
description: "provisioningDHCP consists of two parameters that represents
whether the DHCP 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."
type: object
properties:
logLevel:
description: logLevel is an intent based logging for an overall
component. It does not give fine grained control, but it is a
simple way to manage coarse grained logging choices that operators
have to interpret for their operands.
type: string
managementState:
description: managementState indicates whether DHCP server is present
within the metal3 cluster or external to it. A managementState
value of Removed indicates an external DHCP server and a value
of Managed indicates a DHCP server managed within the metal3 cluster.
type: string
pattern: ^(Managed|Removed)$
observedConfig:
description: observedConfig holds a sparse config that controller
has observed from the cluster state. It exists in spec because
it is an input to the level for the operator
type: object
nullable: true
x-kubernetes-preserve-unknown-fields: true
operatorLogLevel:
description: operatorLogLevel is an intent based logging for the
operator itself. It does not give fine grained control, but it
is a simple way to manage coarse grained logging choices that
operators have to interpret for themselves.
type: string
unsupportedConfigOverrides:
description: 'unsupportedConfigOverrides holds a sparse config that
will override any previously set options. It only needs to be
the fields to override it will end up overlaying in the following
order: 1. hardcoded defaults 2. observedConfig 3. unsupportedConfigOverrides'
type: object
nullable: true
x-kubernetes-preserve-unknown-fields: true
DHCPRange:
description: Needs to be interpreted along with the managementState.
If the ManagementState within the OperatorStatus is set to "Managed",
then the DHCPRange represents the range of IP addresses that the
DHCP server running within the metal3 cluster can use while provisioning
baremetal servers. If the value of ManagementState is set to "Removed",
then the value of DHCPRange will be ignored. If the ManagementState
is "Managed" and the value of DHCPRange is not set, then the DHCP
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.
Copy link

Choose a reason for hiding this comment

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

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

type: string
nullable: true
status:
description: status holds observed values from the cluster. They may not
be overridden.
type: object
properties:
conditions:
description: conditions is a list of conditions and their status
type: array
items:
description: OperatorCondition is just the standard condition fields.
type: object
properties:
lastTransitionTime:
type: string
format: date-time
message:
type: string
reason:
type: string
status:
type: string
type:
type: string
generations:
description: generations are used to determine when an item needs to
be reconciled or has changed in a way that needs a reaction.
type: array
items:
description: GenerationStatus keeps track of the generation for a
given resource so that decisions about forced updates can be made.
type: object
properties:
group:
description: group is the group of the thing you're tracking
type: string
hash:
description: hash is an optional field set for resources without
generation that are content sensitive like secrets and configmaps
type: string
lastGeneration:
description: lastGeneration is the last generation of the workload
controller involved
type: integer
format: int64
name:
description: name is the name of the thing you're tracking
type: string
namespace:
description: namespace is where the thing you're tracking is
type: string
resource:
description: resource is the resource type of the thing you're
tracking
type: string
observedGeneration:
description: observedGeneration is the last generation change you've
dealt with
type: integer
format: int64
readyReplicas:
description: readyReplicas indicates how many replicas are ready and
at the desired state
type: integer
format: int32
version:
description: version is the level this availability applies to
type: string
2 changes: 2 additions & 0 deletions operator/v1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&ServiceCatalogControllerManagerList{},
&IngressController{},
&IngressControllerList{},
&Metal3Provisioning{},
&Metal3ProvisioningList{},
)

return nil
Expand Down
109 changes: 109 additions & 0 deletions operator/v1/types_metal3provisioning.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
package v1

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// +genclient
// +genclient:nonNamespaced
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status

// Metal3Provisioning contains configuration used by the Provisioning
// service (Ironic) to provision baremetal hosts.
//
// Metal3Provisioning is created by the Openshift installer using admin
// or user provided information about the provisioning network and the NIC
// on the server that can be used to PXE boot it.
//
// This CR is a singleton, created by the installer and currently only
// consumed by the machine-api-operator to bring up and update containers
// in a metal3 cluster.
//
type Metal3Provisioning struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`

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

Choose a reason for hiding this comment

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

why is Spec omitempty?


// status is the most recently observed status of the
// Metal3Provisioning.
Status Metal3ProvisioningStatus `json:"status,omitempty"`
}

// ProvisioningDHCP represents just the configuration required to fully
// identify the way DHCP would be handled during baremetal host bringup.
//
// DHCP services could be provided external to the metal3 cluster, in
// which case, IP address assignment for the baremetal servers should
// happen via this external DHCP server and not via a DHCP server started
// within the metal3 cluster.
// If IP address assignment needs to happen via the DHCP server within the
// metal3 cluster, then the CR needs to contain the DHCP address range that
// this internal DHCP server needs to use.
//
type ProvisioningDHCP struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't understand the doc on this field. Is this controlling what will be (desired state) or reflecting what is (actual state).

I'm going to try to find @sadasu for a bluejeans chat.

Copy link

Choose a reason for hiding this comment

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

I replied above already, this describes the desired state of the DHCP services run on the cluster.

This is configuration data which we use to configure a DHCP server that's hosted on the cluster, for PXE boot provisioning of baremetal servers.

Copy link

Choose a reason for hiding this comment

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

In order to configure a DHCP server, you have to specify a pool or IP range for the leases, that's what this provides - @deads2k how can we adjust the comment to explain this more clearly (to me it already seems clear FWIW), any suggestions?

// ManagementState within the OperatorSpec needs to be set to
// indicate if the DHCP server is internal or external to the
// metal3 cluster. ManagementState set to "Removed" indicates
// that the DHCP server is outside the metal3 cluster. And a
// value of "Managed" indicates that the DHCP services are
// managed within the metal3 cluster.
// The other fields of OperatorSpec retain their existing
// semantics.
OperatorSpec `json:",inline"`
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


// If the ManagementState within the OperatorStatus is set to
// "Managed", then the DHCPRange represents the range of IP addresses
// that the DHCP server running within the metal3 cluster can use
// while provisioning baremetal servers. If the value of ManagementState
// is set to "Removed", then the value of DHCPRange will be ignored.
// If the ManagementState is "Managed" and the value of DHCPRange is
// not set, then the DHCP 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.
DHCPRange string `json:"DHCPRange,omitempty"`
Copy link

Choose a reason for hiding this comment

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

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

}

// Metal3ProvisioningSpec is the specification of the desired behavior of the
// Metal3Provisioning.
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"`
Copy link
Contributor

Choose a reason for hiding this comment

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

provisioningNetworkInterface please

Copy link
Contributor

Choose a reason for hiding this comment

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

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


// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

explain why it's temporary, and reused.

Copy link

Choose a reason for hiding this comment

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

It's not temporary, as mentioned above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

ProvisioningIP string `json:"provisioningIP"`
Copy link
Contributor

Choose a reason for hiding this comment

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

provisioningTemporaryBootstrapIP please

Copy link

Choose a reason for hiding this comment

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

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.


// 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"`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.


// provisioningDHCP consists of two parameters that represents whether the DHCP
// 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"`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@stbenjam stbenjam Jan 7, 2020

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

this isn't needed because this is config.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// +kubebuilder:object:root=true

// Metal3ProvisioningList contains a list of Metal3Provisioning.
type Metal3ProvisioningList struct {
metav1.TypeMeta `json:",inline"`
metav1.ListMeta `json:"metadata,omitempty"`
Items []Metal3Provisioning `json:"items"`
}
Loading