Merge pull request #1258 from rabi/OSPRH-12709

Don't set BaremetalHosts in the nodeset spec
openstack-k8s-operators · Jan 14, 2025 · f03ace0 · f03ace0
2 parents 3fc524e + 93afb71
commit f03ace0
Show file tree

Hide file tree

Showing 8 changed files with 18 additions and 107 deletions.
diff --git a/apis/bases/dataplane.openstack.org_openstackdataplanenodesets.yaml b/apis/bases/dataplane.openstack.org_openstackdataplanenodesets.yaml
@@ -51,61 +51,22 @@ spec:
                     - metadata
                     - disabled
                     type: string
-                  baremetalHosts:
-                    additionalProperties:
-                      properties:
-                        bmhLabelSelector:
-                          additionalProperties:
-                            type: string
-                          type: object
-                        ctlPlaneIP:
-                          type: string
-                        networkData:
-                          properties:
-                            name:
-                              type: string
-                            namespace:
-                              type: string
-                          type: object
-                          x-kubernetes-map-type: atomic
-                        userData:
-                          properties:
-                            name:
-                              type: string
-                            namespace:
-                              type: string
-                          type: object
-                          x-kubernetes-map-type: atomic
-                      type: object
-                    type: object
                   bmhLabelSelector:
                     additionalProperties:
                       type: string
                     type: object
                   bmhNamespace:
                     default: openshift-machine-api
                     type: string
-                  bootstrapDns:
-                    items:
-                      type: string
-                    type: array
                   cloudUserName:
                     default: cloud-admin
                     type: string
-                  ctlplaneGateway:
-                    type: string
                   ctlplaneInterface:
                     type: string
                   ctlplaneNetmask:
                     type: string
-                  ctlplaneVlan:
-                    type: integer
                   deploymentSSHSecret:
                     type: string
-                  dnsSearchDomains:
-                    items:
-                      type: string
-                    type: array
                   domainName:
                     type: string
                   hardwareReqs:

diff --git a/apis/dataplane/v1beta1/openstackdataplanenodeset_types.go b/apis/dataplane/v1beta1/openstackdataplanenodeset_types.go
@@ -37,7 +37,7 @@ import (
 type OpenStackDataPlaneNodeSetSpec struct {
 	// +kubebuilder:validation:Optional
 	// BaremetalSetTemplate Template for BaremetalSet for the NodeSet
-	BaremetalSetTemplate baremetalv1.OpenStackBaremetalSetSpec `json:"baremetalSetTemplate,omitempty"`
+	BaremetalSetTemplate baremetalv1.OpenStackBaremetalSetTemplateSpec `json:"baremetalSetTemplate,omitempty"`
 
 	// +kubebuilder:validation:Required
 	// NodeTemplate - node attributes specific to nodes defined by this resource. These
@@ -188,9 +188,8 @@ func (instance *OpenStackDataPlaneNodeSet) InitConditions() {
 		condition.UnknownCondition(condition.ServiceAccountReadyCondition, condition.InitReason, condition.ServiceAccountReadyInitMessage),
 	)
 
-	// Only set Baremetal related conditions if we have baremetal hosts included in the
-	// baremetalSetTemplate.
-	if len(instance.Spec.BaremetalSetTemplate.BaremetalHosts) > 0 {
+	// Only set Baremetal related conditions if required
+	if !instance.Spec.PreProvisioned && len(instance.Spec.Nodes) > 0 {
 		cl = append(cl, *condition.UnknownCondition(NodeSetBareMetalProvisionReadyCondition, condition.InitReason, condition.InitReason))
 	}
 

diff --git a/apis/dataplane/v1beta1/openstackdataplanenodeset_webhook.go b/apis/dataplane/v1beta1/openstackdataplanenodeset_webhook.go
@@ -24,7 +24,6 @@ import (
 
 	"github.com/go-playground/validator/v10"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/condition"
-	baremetalv1 "github.com/openstack-k8s-operators/openstack-baremetal-operator/api/v1beta1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -79,6 +78,7 @@ func (spec *OpenStackDataPlaneNodeSetSpec) Default() {
 				node.HostName = strings.Join([]string{nodeName, domain}, ".")
 			}
 		}
+
 		spec.Nodes[nodeName] = *node.DeepCopy()
 	}
 
@@ -87,15 +87,6 @@ func (spec *OpenStackDataPlaneNodeSetSpec) Default() {
 		if spec.BaremetalSetTemplate.DeploymentSSHSecret == "" {
 			spec.BaremetalSetTemplate.DeploymentSSHSecret = spec.NodeTemplate.AnsibleSSHPrivateKeySecret
 		}
-		nodeSetHostMap := make(map[string]baremetalv1.InstanceSpec)
-		for _, node := range spec.Nodes {
-			instanceSpec := baremetalv1.InstanceSpec{}
-			instanceSpec.BmhLabelSelector = node.BmhLabelSelector
-			instanceSpec.UserData = node.UserData
-			instanceSpec.NetworkData = node.NetworkData
-			nodeSetHostMap[node.HostName] = instanceSpec
-		}
-		spec.BaremetalSetTemplate.BaremetalHosts = nodeSetHostMap
 	} else if spec.NodeTemplate.Ansible.AnsibleUser == "" {
 		spec.NodeTemplate.Ansible.AnsibleUser = "cloud-admin"
 	}
@@ -220,9 +211,9 @@ func (r *OpenStackDataPlaneNodeSetSpec) ValidateUpdate(oldSpec *OpenStackDataPla
 	// If the BaremetalSetTemplate is changed, we will offload the parsing of these details
 	// to the openstack-baremetal-operator webhook to avoid duplicating logic.
 	if !reflect.DeepEqual(r.BaremetalSetTemplate, oldSpec.BaremetalSetTemplate) {
-
 		// Call openstack-baremetal-operator webhook Validate() to parse changes
-		err := r.BaremetalSetTemplate.Validate(oldSpec.BaremetalSetTemplate)
+		err := r.BaremetalSetTemplate.ValidateTemplate(
+			len(oldSpec.Nodes), oldSpec.BaremetalSetTemplate)
 		if err != nil {
 			errors = append(errors, field.Forbidden(
 				field.NewPath("spec.baremetalSetTemplate"),

diff --git a/config/crd/bases/dataplane.openstack.org_openstackdataplanenodesets.yaml b/config/crd/bases/dataplane.openstack.org_openstackdataplanenodesets.yaml
@@ -51,61 +51,22 @@ spec:
                     - metadata
                     - disabled
                     type: string
-                  baremetalHosts:
-                    additionalProperties:
-                      properties:
-                        bmhLabelSelector:
-                          additionalProperties:
-                            type: string
-                          type: object
-                        ctlPlaneIP:
-                          type: string
-                        networkData:
-                          properties:
-                            name:
-                              type: string
-                            namespace:
-                              type: string
-                          type: object
-                          x-kubernetes-map-type: atomic
-                        userData:
-                          properties:
-                            name:
-                              type: string
-                            namespace:
-                              type: string
-                          type: object
-                          x-kubernetes-map-type: atomic
-                      type: object
-                    type: object
                   bmhLabelSelector:
                     additionalProperties:
                       type: string
                     type: object
                   bmhNamespace:
                     default: openshift-machine-api
                     type: string
-                  bootstrapDns:
-                    items:
-                      type: string
-                    type: array
                   cloudUserName:
                     default: cloud-admin
                     type: string
-                  ctlplaneGateway:
-                    type: string
                   ctlplaneInterface:
                     type: string
                   ctlplaneNetmask:
                     type: string
-                  ctlplaneVlan:
-                    type: integer
                   deploymentSSHSecret:
                     type: string
-                  dnsSearchDomains:
-                    items:
-                      type: string
-                    type: array
                   domainName:
                     type: string
                   hardwareReqs:

diff --git a/hack/crd-schema-checker.sh b/hack/crd-schema-checker.sh
@@ -3,11 +3,15 @@ set -euxo pipefail
 
 CHECKER=$INSTALL_DIR/crd-schema-checker
 
-DISABLED_VALIDATORS=NoMaps # TODO: https://issues.redhat.com/browse/OSPRH-12254
+# (TODO) Remove NoFieldRemoval after this PR merges
+DISABLED_VALIDATORS=NoMaps,NoFieldRemoval # TODO: https://issues.redhat.com/browse/OSPRH-12254
 
 CHECKER_ARGS=""
 if [[ ${DISABLED_VALIDATORS:+x} ]]; then
-    CHECKER_ARGS="$CHECKER_ARGS --disabled-validators $DISABLED_VALIDATORS"
+    CHECKER_ARGS="$CHECKER_ARGS "
+    for check in ${DISABLED_VALIDATORS//,/ }; do
+        CHECKER_ARGS+=" --disabled-validators $check"
+    done
 fi
 
 TMP_DIR=$(mktemp -d)

diff --git a/pkg/dataplane/baremetal.go b/pkg/dataplane/baremetal.go
@@ -49,15 +49,12 @@ func DeployBaremetalSet(
 		},
 	}
 
-	if instance.Spec.BaremetalSetTemplate.BaremetalHosts == nil {
-		return false, fmt.Errorf("no baremetal hosts set in baremetalSetTemplate")
-	}
 	utils.LogForObject(helper, "Reconciling BaremetalSet", instance)
 	_, err := controllerutil.CreateOrPatch(ctx, helper.GetClient(), baremetalSet, func() error {
 		ownerLabels := labels.GetLabels(instance, labels.GetGroupLabel(NodeSetLabel), map[string]string{})
 		baremetalSet.Labels = utils.MergeStringMaps(baremetalSet.GetLabels(), ownerLabels)
-
-		instance.Spec.BaremetalSetTemplate.DeepCopyInto(&baremetalSet.Spec)
+		baremetalSet.Spec.BaremetalHosts = make(map[string]baremetalv1.InstanceSpec)
+		instance.Spec.BaremetalSetTemplate.DeepCopyInto(&baremetalSet.Spec.OpenStackBaremetalSetTemplateSpec)
 		// Set Images
 		if containerImages.OsContainerImage != nil && instance.Spec.BaremetalSetTemplate.OSContainerImageURL == "" {
 			baremetalSet.Spec.OSContainerImageURL = *containerImages.OsContainerImage
@@ -72,11 +69,14 @@ func DeployBaremetalSet(
 		for _, node := range instance.Spec.Nodes {
 			hostName := node.HostName
 			ipSet, ok := ipSets[hostName]
-			instanceSpec := baremetalSet.Spec.BaremetalHosts[hostName]
 			if !ok {
 				err := fmt.Errorf("no IPSet found for host: %s", hostName)
 				return err
 			}
+			instanceSpec := baremetalv1.InstanceSpec{}
+			instanceSpec.BmhLabelSelector = node.BmhLabelSelector
+			instanceSpec.UserData = node.UserData
+			instanceSpec.NetworkData = node.NetworkData
 			for _, res := range ipSet.Status.Reservation {
 				if strings.ToLower(string(res.Network)) == dataplanev1.CtlPlaneNetwork {
 					_, ipNet, err := net.ParseCIDR(res.Cidr)

diff --git a/tests/functional/dataplane/openstackdataplanenodeset_controller_test.go b/tests/functional/dataplane/openstackdataplanenodeset_controller_test.go
@@ -331,7 +331,6 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 					"telemetry"}
 
 				Expect(dataplaneNodeSetInstance.Spec.Services).Should(Equal(services))
-				Expect(dataplaneNodeSetInstance.Spec.BaremetalSetTemplate.BaremetalHosts).Should(BeEmpty())
 			})
 
 			It("should have input not ready and unknown Conditions initialized", func() {
@@ -432,7 +431,6 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 					"global-service"}
 
 				Expect(dataplaneNodeSetInstance.Spec.Services).Should(Equal(services))
-				Expect(dataplaneNodeSetInstance.Spec.BaremetalSetTemplate.BaremetalHosts).Should(BeEmpty())
 			})
 
 			It("should have input not ready and unknown Conditions initialized", func() {
@@ -846,7 +844,6 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 					"telemetry"}
 
 				Expect(dataplaneNodeSetInstance.Spec.Services).Should(Equal(services))
-				Expect(dataplaneNodeSetInstance.Spec.BaremetalSetTemplate.BaremetalHosts).Should(BeEmpty())
 			})
 			It("should have input not ready and unknown Conditions initialized", func() {
 				th.ExpectCondition(

diff --git a/tests/functional/dataplane/openstackdataplanenodeset_webhook_test.go b/tests/functional/dataplane/openstackdataplanenodeset_webhook_test.go
@@ -41,7 +41,6 @@ var _ = Describe("DataplaneNodeSet Webhook", func() {
 					"hostName": "compute-0"},
 			}
 			nodeSetSpec["baremetalSetTemplate"] = map[string]interface{}{
-				"cloudUserName": "test-user",
 				"bmhLabelSelector": map[string]string{
 					"app": "test-openstack",
 				},
@@ -52,7 +51,6 @@ var _ = Describe("DataplaneNodeSet Webhook", func() {
 		It("Should block changes to the BmhLabelSelector object in baremetalSetTemplate spec", func() {
 			Eventually(func(_ Gomega) string {
 				instance := GetDataplaneNodeSet(dataplaneNodeSetName)
-				instance.Spec.BaremetalSetTemplate.CloudUserName = "new-user"
 				instance.Spec.BaremetalSetTemplate.BmhLabelSelector = map[string]string{
 					"app": "openstack1",
 				}