Support dedupe of overidden services in deployment

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

controllers/dataplane/openstackdataplanedeployment_controller.go

@@ -251,17 +251,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)
@@ -310,7 +309,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])
 		}

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"
@@ -161,31 +162,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

@@ -213,6 +213,20 @@ func DefaultDataPlaneDeploymentSpec() 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)