Support dedupe of overidden services in deployment

Signed-off-by: rabi <[email protected]>

controllers/dataplane/openstackdataplanedeployment_controller.go

@@ -250,17 +250,16 @@ func (r *OpenStackDataPlaneDeploymentReconciler) Reconcile(ctx context.Context,
 		globalSSHKeySecrets[nodeSet.Name] = nodeSet.Spec.NodeTemplate.AnsibleSSHPrivateKeySecret
 	}
 
-	if instance.Spec.ServicesOverride == nil {
-		if nodesetServiceMap, err = deployment.DedupServices(ctx, helper, nodeSets.Items); err != nil {
-			util.LogErrorForObject(helper, err, "OpenStackDeployment error for deployment", instance)
-			instance.Status.Conditions.MarkFalse(
-				condition.DeploymentReadyCondition,
-				condition.ErrorReason,
-				condition.SeverityError,
-				dataplanev1.ServiceErrorMessage,
-				err.Error())
-			return ctrl.Result{}, err
-		}
+	if nodesetServiceMap, err = deployment.DedupeServices(ctx, helper, nodeSets.Items,
+		instance.Spec.ServicesOverride); err != nil {
+		util.LogErrorForObject(helper, err, "OpenStackDeployment error for deployment", instance)
+		instance.Status.Conditions.MarkFalse(
+			condition.DeploymentReadyCondition,
+			condition.ErrorReason,
+			condition.SeverityError,
+			dataplanev1.ServiceErrorMessage,
+			err.Error())
+		return ctrl.Result{}, err
 	}
 
 	version, err := dataplaneutil.GetVersion(ctx, helper, instance.Namespace)
@@ -309,7 +308,7 @@ func (r *OpenStackDataPlaneDeploymentReconciler) Reconcile(ctx context.Context,
 		// deploy with the OpenStackDataPlaneNodeSet's Services.
 		var deployResult *ctrl.Result
 		if len(instance.Spec.ServicesOverride) != 0 {
-			deployResult, err = deployer.Deploy(instance.Spec.ServicesOverride)
+			deployResult, err = deployer.Deploy(nodesetServiceMap[deployment.AllNodeSets])
 		} else {
 			deployResult, err = deployer.Deploy(nodesetServiceMap[nodeSet.Name])
 		}

docs/assemblies/proc_creating-a-custom-service.adoc

@@ -92,7 +92,7 @@ spec:
 +
 A mount is created for each `ConfigMap` and `Secret` in the `OpenStackAnsibleEE` pod with a filename that matches the resource value. The mounts are created under `/var/lib/openstack/configs/<service name>`.
 
-. Optional: It may be necessary to run some services on all nodesets at the same time. These services need to have their `deployOnAllNodeSets` field set to true.
+. Optional: It may be necessary to run some services on all nodesets at the same time. These services need to have their `deployOnAllNodeSets` field set to true. If these services are repated in multiple nodeset specs included in a deployment, they would be ignored from subsequent nodeset services and would be run only once.
 +
 ----
 apiVersion: dataplane.openstack.org/v1beta1
@@ -112,7 +112,7 @@ spec:
   deployOnAllNodeSets: true
 ----
 
-. Optional: Specify the `edpmServiceType` field for the service. Different custom services may use the same ansible content to manage the same EDPM service (such as `ovn` or `nova`). The `DataSources`, TLS certificates, and CA certificates need to be mounted at the same locations so they can be found by the ansible content even when using a custom service. `edpmServiceType` is used to create this association. The value is the name of the default service that uses the same ansible content as the custom service.
+. Optional: Specify the `edpmServiceType` field for the service. Different custom services may use the same ansible content to manage the same EDPM service (such as `ovn` or `nova`). The `DataSources`, TLS certificates, and CA certificates need to be mounted at the same locations so they can be found by the ansible content even when using a custom service. `edpmServiceType` is used to create this association. The value is the name of the default service that uses the same ansible content as the custom service. If there are multiple services with the same `edpmServiceType` listed in a nodeset or deployment spec, latter ones would be ignored.
 +
 For example, a custom service that uses the `edpm_ovn` ansible content from `edpm-ansible` would set `edpmServiceType` to `ovn`, which matches the default `ovn` service name provided by `openstack-operator`.
 +

pkg/dataplane/const.go

@@ -71,4 +71,8 @@ const (
 
 	//HostnameLabel label for marking secrets to be watched for changes
 	HostnameLabel = "hostname"
+
+	//AllNodeSets Key used in nodeset service map for all nodesets when
+	//using serviceOverride.
+	AllNodeSets = "AllNodeSets"
 )

pkg/dataplane/service.go

@@ -25,6 +25,7 @@ import (
 
 	"golang.org/x/exp/slices"
 	yaml "gopkg.in/yaml.v3"
+	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -150,31 +151,60 @@ func EnsureServices(ctx context.Context, helper *helper.Helper, instance *datapl
 	return nil
 }
 
-// Dedup services for deploy
+// Dedupe services to deploy
 // Multiple Services of same ServiceType/ServiceName in a nodeset
 // Global Services in multiple NodeSets for a deployment
-func DedupServices(ctx context.Context, helper *helper.Helper, nodesets []dataplanev1.OpenStackDataPlaneNodeSet) (map[string][]string, error) {
-	var globalServices []string
+func DedupeServices(ctx context.Context, helper *helper.Helper,
+	nodesets []dataplanev1.OpenStackDataPlaneNodeSet,
+	serviceOverride []string) (map[string][]string, error) {
 	var nodeSetServiceMap = make(map[string][]string, 0)
-	for _, nodeset := range nodesets {
-		var services []string
-		var nodeSetServiceTypes []string
-		for _, svc := range nodeset.Spec.Services {
-			service, err := GetService(ctx, helper, svc)
+	var globalServices []string
+	if len(serviceOverride) != 0 {
+		services, err := dedupe(ctx, helper, serviceOverride, globalServices)
+		if err != nil {
+			return nil, err
+		}
+		nodeSetServiceMap[AllNodeSets] = services
+	} else {
+		for _, nodeset := range nodesets {
+			services, err := dedupe(ctx, helper, nodeset.Spec.Services, globalServices)
 			if err != nil {
-				helper.GetLogger().Error(err, fmt.Sprintf("Configured service %s does not exist", svc))
 				return nil, err
 			}
-			if !slices.Contains(nodeSetServiceTypes, service.Spec.EDPMServiceType) && !slices.Contains(services, svc) {
-				nodeSetServiceTypes = append(nodeSetServiceTypes, service.Spec.EDPMServiceType)
-				services = append(services, svc)
+			nodeSetServiceMap[nodeset.Name] = services
+		}
+	}
+	return nodeSetServiceMap, nil
+}
+
+func dedupe(ctx context.Context, helper *helper.Helper,
+	services []string, globalServices []string) ([]string, error) {
+	var dedupedServices []string
+	var nodeSetServiceTypes []string
+	for _, svc := range services {
+		service, err := GetService(ctx, helper, svc)
+		if err != nil {
+			if k8s_errors.IsNotFound(err) && !slices.Contains(dedupedServices, svc) {
+				helper.GetLogger().Error(err, fmt.Sprintf("Configured service %s does not exist", svc))
+				// Add the service to the service list as it would fail later when deploying and
+				// Update the statuses accordingly.
+				dedupedServices = append(dedupedServices, svc)
+				continue
 			}
-			if service.Spec.DeployOnAllNodeSets && !slices.Contains(globalServices, svc) {
-				globalServices = append(globalServices, svc)
-				services = append(services, svc)
+			return dedupedServices, err
+
+		}
+		if !slices.Contains(nodeSetServiceTypes, service.Spec.EDPMServiceType) && !slices.Contains(dedupedServices, svc) {
+			if service.Spec.DeployOnAllNodeSets {
+				if !slices.Contains(globalServices, svc) {
+					globalServices = append(globalServices, svc)
+				} else {
+					continue
+				}
 			}
+			nodeSetServiceTypes = append(nodeSetServiceTypes, service.Spec.EDPMServiceType)
+			dedupedServices = append(dedupedServices, svc)
 		}
-		nodeSetServiceMap[nodeset.Name] = services
 	}
-	return nodeSetServiceMap, nil
+	return dedupedServices, nil
 }

tests/functional/dataplane/base_test.go

@@ -237,7 +237,6 @@ func DefaultDataPlaneDeploymentSpec() map[string]interface{} {
 }
 
 func MinorUpdateDataPlaneDeploymentSpec() map[string]interface{} {
-
 	return map[string]interface{}{
 		"nodeSets": []string{
 			"edpm-compute-nodeset",
@@ -246,6 +245,19 @@ func MinorUpdateDataPlaneDeploymentSpec() map[string]interface{} {
 	}
 }
 
+// Build OpenStackDataPlnaeDeploymentSpec with duplicate services
+func DuplicateServiceDeploymentSpec() map[string]interface{} {
+	return map[string]interface{}{
+		"nodeSets": []string{
+			"edpm-compute-nodeset",
+		},
+		"servicesOverride": []string{
+			"foo-service",
+			"duplicate-service",
+		},
+	}
+}
+
 func DefaultNetConfigSpec() map[string]interface{} {
 	return map[string]interface{}{
 		"networks": []map[string]interface{}{{

tests/functional/dataplane/openstackdataplanedeployment_controller_test.go

@@ -706,7 +706,105 @@ var _ = Describe("Dataplane Deployment Test", func() {
 			)
 		})
 	})
+	When("A dataplaneDeployment is created with duplicate service in deployment service override", func() {
+		BeforeEach(func() {
+			CreateDataplaneServicesWithSameServiceType(dataplaneServiceName)
+			CreateSSHSecret(dataplaneSSHSecretName)
+			DeferCleanup(th.DeleteInstance, th.CreateSecret(neutronOvnMetadataSecretName, map[string][]byte{
+				"fake_keys": []byte("blih"),
+			}))
+			DeferCleanup(th.DeleteInstance, th.CreateSecret(novaNeutronMetadataSecretName, map[string][]byte{
+				"fake_keys": []byte("blih"),
+			}))
+			DeferCleanup(th.DeleteInstance, th.CreateSecret(novaCellComputeConfigSecretName, map[string][]byte{
+				"fake_keys": []byte("blih"),
+			}))
+			DeferCleanup(th.DeleteInstance, th.CreateSecret(novaMigrationSSHKey, map[string][]byte{
+				"ssh-privatekey": []byte("fake-ssh-private-key"),
+				"ssh-publickey":  []byte("fake-ssh-public-key"),
+			}))
+			DeferCleanup(th.DeleteInstance, th.CreateSecret(ceilometerConfigSecretName, map[string][]byte{
+				"fake_keys": []byte("blih"),
+			}))
+			// DefaultDataPlanenodeSetSpec comes with two mock services, one marked for deployment on all nodesets
+			// But we will not create them to test this scenario
+			DeferCleanup(th.DeleteInstance, CreateNetConfig(dataplaneNetConfigName, DefaultNetConfigSpec()))
+			DeferCleanup(th.DeleteInstance, CreateDNSMasq(dnsMasqName, DefaultDNSMasqSpec()))
+			SimulateDNSMasqComplete(dnsMasqName)
+			DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, DefaultDataPlaneNodeSetSpec(dataplaneNodeSetName.Name)))
+			DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(dataplaneDeploymentName, DuplicateServiceDeploymentSpec()))
+			SimulateIPSetComplete(dataplaneNodeName)
+			SimulateDNSDataComplete(dataplaneNodeSetName)
+		})
+		It("Should have Spec fields initialized", func() {
+			dataplaneDeploymentInstance := GetDataplaneDeployment(dataplaneDeploymentName)
+			expectedSpec := dataplanev1.OpenStackDataPlaneDeploymentSpec{
+				NodeSets:              []string{"edpm-compute-nodeset"},
+				AnsibleTags:           "",
+				AnsibleLimit:          "",
+				AnsibleSkipTags:       "",
+				BackoffLimit:          &DefaultBackoffLimit,
+				DeploymentRequeueTime: 15,
+				ServicesOverride:      []string{dataplaneServiceName.Name, "duplicate-service"},
+			}
+			Expect(dataplaneDeploymentInstance.Spec).Should(Equal(expectedSpec))
+		})
 
+		It("should have conditions set to true", func() {
+			baremetal := baremetalv1.OpenStackBaremetalSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      dataplaneNodeSetName.Name,
+					Namespace: dataplaneNodeName.Namespace,
+				},
+			}
+			// Set baremetal provisioning conditions to True
+			// This must succeed, as the "alpha-nodeset" exists
+			Eventually(func(g Gomega) {
+				// OpenStackBaremetalSet has the same name as OpenStackDataPlaneNodeSet
+				g.Expect(th.K8sClient.Get(th.Ctx, dataplaneNodeSetName, &baremetal)).To(Succeed())
+				baremetal.Status.Conditions.MarkTrue(
+					condition.ReadyCondition,
+					condition.ReadyMessage)
+				g.Expect(th.K8sClient.Status().Update(th.Ctx, &baremetal)).To(Succeed())
+
+			}, th.Timeout, th.Interval).Should(Succeed())
+
+			// Create config map for OVN service
+			ovnConfigMapName := types.NamespacedName{
+				Namespace: namespace,
+				Name:      "ovncontroller-config",
+			}
+			mapData := map[string]interface{}{
+				"ovsdb-config": "test-ovn-config",
+			}
+			th.CreateConfigMap(ovnConfigMapName, mapData)
+			service := GetService(dataplaneServiceName)
+			aeeName, _ := dataplaneutil.GetAnsibleExecutionNameAndLabels(service,
+				dataplaneDeploymentName.Name, dataplaneNodeSetName.Name)
+			ansibleeeName := types.NamespacedName{
+				Name:      aeeName,
+				Namespace: dataplaneDeploymentName.Namespace,
+			}
+			Eventually(func(g Gomega) {
+				ansibleEE := GetAnsibleee(ansibleeeName)
+				ansibleEE.Status.JobStatus = ansibleeev1.JobStatusSucceeded
+				g.Expect(th.K8sClient.Status().Update(th.Ctx, ansibleEE)).To(Succeed())
+			}, th.Timeout, th.Interval).Should(Succeed())
+
+			th.ExpectCondition(
+				dataplaneDeploymentName,
+				ConditionGetterFunc(DataplaneDeploymentConditionGetter),
+				condition.DeploymentReadyCondition,
+				corev1.ConditionTrue,
+			)
+			th.ExpectCondition(
+				dataplaneDeploymentName,
+				ConditionGetterFunc(DataplaneDeploymentConditionGetter),
+				condition.InputReadyCondition,
+				corev1.ConditionTrue,
+			)
+		})
+	})
 	When("A dataplaneDeployment is created with non-existent service in nodeset", func() {
 		BeforeEach(func() {
 			CreateSSHSecret(dataplaneSSHSecretName)