Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ spec:
description: OpenStackDataPlaneDeploymentSpec defines the desired state
of OpenStackDataPlaneDeployment
properties:
ansibleEEEnvConfigMapName:
description: |-
AnsibleEEEnvConfigMapName is the name of the ConfigMap containing environment
variables to inject into the Ansible Execution Environment pod.
If set, this takes precedence over the NodeSet's AnsibleEEEnvConfigMapName.
If not specified, the NodeSet's AnsibleEEEnvConfigMapName is used, or the
default "openstack-aee-default-env" if neither is set.
maxLength: 253
type: string
ansibleExtraVars:
description: AnsibleExtraVars for ansible execution
x-kubernetes-preserve-unknown-fields: true
Expand Down
12 changes: 12 additions & 0 deletions api/bases/dataplane.openstack.org_openstackdataplanenodesets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ spec:
description: OpenStackDataPlaneNodeSetSpec defines the desired state of
OpenStackDataPlaneNodeSet
properties:
ansibleEEEnvConfigMapName:
default: openstack-aee-default-env
description: |-
AnsibleEEEnvConfigMapName is the name of the ConfigMap containing environment
variables to inject into the Ansible Execution Environment pod.
The ConfigMap should contain environment variables used by ansible-runner
such as ANSIBLE_VERBOSITY, ANSIBLE_FORCE_COLOR, or other runner settings.
See https://ansible.readthedocs.io/projects/runner/en/stable/intro/#env-settings-settings-for-runner-itself
If not specified, defaults to "openstack-aee-default-env".
This can be overridden at the Deployment level.
maxLength: 253
type: string
baremetalSetTemplate:
description: BaremetalSetTemplate Template for BaremetalSet for the
NodeSet
Expand Down
3 changes: 3 additions & 0 deletions api/dataplane/v1beta1/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,4 +198,7 @@ type AnsibleEESpec struct {
// the ansible execution run with. Without specifying, it will run with
// default serviceaccount
ServiceAccountName string
// AnsibleEEEnvConfigMapName is the name of ConfigMap containing environment
// variables to inject to the Ansible execution environment pod.
AnsibleEEEnvConfigMapName string `json:"ansibleEEEnvConfigMapName,omitempty"`
}
9 changes: 9 additions & 0 deletions api/dataplane/v1beta1/openstackdataplanedeployment_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,15 @@ type OpenStackDataPlaneDeploymentSpec struct {
// +kubebuilder:validation:Optional
// AnsibleJobNodeSelector to target subset of worker nodes running the ansible jobs
AnsibleJobNodeSelector map[string]string `json:"ansibleJobNodeSelector,omitempty"`

// +kubebuilder:validation:Optional
// +kubebuilder:validation:MaxLength:=253
// AnsibleEEEnvConfigMapName is the name of the ConfigMap containing environment
// variables to inject into the Ansible Execution Environment pod.
// If set, this takes precedence over the NodeSet's AnsibleEEEnvConfigMapName.
// If not specified, the NodeSet's AnsibleEEEnvConfigMapName is used, or the
// default "openstack-aee-default-env" if neither is set.
AnsibleEEEnvConfigMapName string `json:"ansibleEEEnvConfigMapName,omitempty"`
}

// OpenStackDataPlaneDeploymentStatus defines the observed state of OpenStackDataPlaneDeployment
Expand Down
21 changes: 17 additions & 4 deletions api/dataplane/v1beta1/openstackdataplanenodeset_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,18 @@ type OpenStackDataPlaneNodeSetSpec struct {
// +kubebuilder:default=true
// +operator-sdk:csv:customresourcedefinitions:type=spec,xDescriptors={"urn:alm:descriptor:com.tectonic.ui:booleanSwitch"}
TLSEnabled bool `json:"tlsEnabled" yaml:"tlsEnabled"`

// +kubebuilder:validation:Optional
// +kubebuilder:default="openstack-aee-default-env"
// +kubebuilder:validation:MaxLength:=253
// AnsibleEEEnvConfigMapName is the name of the ConfigMap containing environment
// variables to inject into the Ansible Execution Environment pod.
// The ConfigMap should contain environment variables used by ansible-runner
// such as ANSIBLE_VERBOSITY, ANSIBLE_FORCE_COLOR, or other runner settings.
// See https://ansible.readthedocs.io/projects/runner/en/stable/intro/#env-settings-settings-for-runner-itself
// If not specified, defaults to "openstack-aee-default-env".
// This can be overridden at the Deployment level.
AnsibleEEEnvConfigMapName string `json:"ansibleEEEnvConfigMapName,omitempty"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can drop this from the NodeSet actually. I don't see where it's used honestly. And given that the env vars apply to the entire Deployment, there isn't a way to set different env vars per NodeSet anyway, so it should only be allowed on the Deployment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done considering that, any env configuration applied to/at NodeSet level, will propagate to each deployment(if not set at deployment level) which is part of that NodeSet.

This might be useful in cases where we need different configuration for different deployment and we can use NodeSet as umbrella for a default behavior. Examples

  • Different compute or machine types needing different ansible runner settings
  • Reusable NodeSet definitions with default values.
  • Multiple Deployments reusing the same NodeSet with its predefined env configuration, until a different configuration is required at deployment level or for any specific use cases

This might be useful for end user who is configuring the environment and don't want to write same env configurations for each deployment(example 100 OSDPD) yaml config file, rather the user can provide this at NodeSet and that will be used by all 100 OSDPDs. This would be less error prone and easily managable.

With this approach we can manage NodeSet level(Default) as well Deployment(specific) both.

What do you say?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that the env vars that will be set for Ansible from the ConfigMap apply globally to the ansible execution. Given a Deployment can be for multiple NodeSets, there is no way to handle different env vars per NodeSet in the same Deployment.

Even in this PR, I don't see how the ConfigMap is used at the NodeSet level. It's never set anywhere. In deployment_controller.go, the value is gotten from the Deployment, and then in ansible_execution.go, that's the value that is used. The value on the NodeSet is never used, afaict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that the env vars that will be set for Ansible from the ConfigMap apply globally to the ansible execution. Given a Deployment can be for multiple NodeSets, there is no way to handle different env vars per NodeSet in the same Deployment.

No, there is a loop for handling different env vars per NodeSet

for _, nodeSet := range nodeSets.Items {
...........
............
          // override the ansibleEEEnvConfigMapName on nodeset if there is any provided for a specific deployment.
		if instance.Spec.AnsibleEEEnvConfigMapName != "" {
			ansibleEESpec.AnsibleEEEnvConfigMapName = instance.Spec.AnsibleEEEnvConfigMapName
		}
}

Even in this PR, I don't see how the ConfigMap is used at the NodeSet level. It's never set anywhere.


func (instance OpenStackDataPlaneNodeSet) GetAnsibleEESpec() AnsibleEESpec {
	return AnsibleEESpec{
		NetworkAttachments:        instance.Spec.NetworkAttachments,
		ExtraMounts:               instance.Spec.NodeTemplate.ExtraMounts,
		Env:                       instance.Spec.Env,
		ServiceAccountName:        instance.Name,
		AnsibleEEEnvConfigMapName: instance.Spec.AnsibleEEEnvConfigMapName, // HERE, the instance is NodeSet type, this will only be overridden if ConfigMap for environment is set in deployment
	}
}

In deployment_controller.go, the value is gotten from the Deployment, and then in ansible_execution.go, that's the value that is used. The value on the NodeSet is never used, afaict.

Only if env is explicitly set in deployment otherwise it is used from what is in Nodeset.


// +kubebuilder:object:root=true
Expand Down Expand Up @@ -205,10 +217,11 @@ func (instance *OpenStackDataPlaneNodeSet) InitConditions() {
// GetAnsibleEESpec - get the fields that will be passed to AEE Job
func (instance OpenStackDataPlaneNodeSet) GetAnsibleEESpec() AnsibleEESpec {
return AnsibleEESpec{
NetworkAttachments: instance.Spec.NetworkAttachments,
ExtraMounts: instance.Spec.NodeTemplate.ExtraMounts,
Env: instance.Spec.Env,
ServiceAccountName: instance.Name,
NetworkAttachments: instance.Spec.NetworkAttachments,
ExtraMounts: instance.Spec.NodeTemplate.ExtraMounts,
Env: instance.Spec.Env,
ServiceAccountName: instance.Name,
AnsibleEEEnvConfigMapName: instance.Spec.AnsibleEEEnvConfigMapName,
}
}

Expand Down
21 changes: 21 additions & 0 deletions bindata/crds/crds.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17144,6 +17144,15 @@ spec:
description: OpenStackDataPlaneDeploymentSpec defines the desired state
of OpenStackDataPlaneDeployment
properties:
ansibleEEEnvConfigMapName:
description: |-
AnsibleEEEnvConfigMapName is the name of the ConfigMap containing environment
variables to inject into the Ansible Execution Environment pod.
If set, this takes precedence over the NodeSet's AnsibleEEEnvConfigMapName.
If not specified, the NodeSet's AnsibleEEEnvConfigMapName is used, or the
default "openstack-aee-default-env" if neither is set.
maxLength: 253
type: string
ansibleExtraVars:
description: AnsibleExtraVars for ansible execution
x-kubernetes-preserve-unknown-fields: true
Expand Down Expand Up @@ -17399,6 +17408,18 @@ spec:
description: OpenStackDataPlaneNodeSetSpec defines the desired state of
OpenStackDataPlaneNodeSet
properties:
ansibleEEEnvConfigMapName:
default: openstack-aee-default-env
description: |-
AnsibleEEEnvConfigMapName is the name of the ConfigMap containing environment
variables to inject into the Ansible Execution Environment pod.
The ConfigMap should contain environment variables used by ansible-runner
such as ANSIBLE_VERBOSITY, ANSIBLE_FORCE_COLOR, or other runner settings.
See https://ansible.readthedocs.io/projects/runner/en/stable/intro/#env-settings-settings-for-runner-itself
If not specified, defaults to "openstack-aee-default-env".
This can be overridden at the Deployment level.
maxLength: 253
type: string
baremetalSetTemplate:
description: BaremetalSetTemplate Template for BaremetalSet for the
NodeSet
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ spec:
description: OpenStackDataPlaneDeploymentSpec defines the desired state
of OpenStackDataPlaneDeployment
properties:
ansibleEEEnvConfigMapName:
description: |-
AnsibleEEEnvConfigMapName is the name of the ConfigMap containing environment
variables to inject into the Ansible Execution Environment pod.
If set, this takes precedence over the NodeSet's AnsibleEEEnvConfigMapName.
If not specified, the NodeSet's AnsibleEEEnvConfigMapName is used, or the
default "openstack-aee-default-env" if neither is set.
maxLength: 253
type: string
ansibleExtraVars:
description: AnsibleExtraVars for ansible execution
x-kubernetes-preserve-unknown-fields: true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ spec:
description: OpenStackDataPlaneNodeSetSpec defines the desired state of
OpenStackDataPlaneNodeSet
properties:
ansibleEEEnvConfigMapName:
default: openstack-aee-default-env
description: |-
AnsibleEEEnvConfigMapName is the name of the ConfigMap containing environment
variables to inject into the Ansible Execution Environment pod.
The ConfigMap should contain environment variables used by ansible-runner
such as ANSIBLE_VERBOSITY, ANSIBLE_FORCE_COLOR, or other runner settings.
See https://ansible.readthedocs.io/projects/runner/en/stable/intro/#env-settings-settings-for-runner-itself
If not specified, defaults to "openstack-aee-default-env".
This can be overridden at the Deployment level.
maxLength: 253
type: string
baremetalSetTemplate:
description: BaremetalSetTemplate Template for BaremetalSet for the
NodeSet
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,10 @@ func (r *OpenStackDataPlaneDeploymentReconciler) Reconcile(ctx context.Context,
ansibleEESpec.AnsibleSkipTags = instance.Spec.AnsibleSkipTags
ansibleEESpec.AnsibleLimit = instance.Spec.AnsibleLimit
ansibleEESpec.ExtraVars = instance.Spec.AnsibleExtraVars
// override the ansibleEEEnvConfigMapName on nodeset if there is any provided for a specific deployment.
if instance.Spec.AnsibleEEEnvConfigMapName != "" {
ansibleEESpec.AnsibleEEEnvConfigMapName = instance.Spec.AnsibleEEEnvConfigMapName
}

if nodeSet.Status.DNSClusterAddresses != nil && nodeSet.Status.CtlplaneSearchDomain != "" {
ansibleEESpec.DNSConfig = &corev1.PodDNSConfig{
Expand Down
9 changes: 8 additions & 1 deletion internal/dataplane/util/ansible_execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,18 @@ func AnsibleExecution(
return nil
}

// Fallback for backwards compatibility with existing NodeSets created
// before ansibleEEEnvConfigMapName field was added
envConfigMapName := aeeSpec.AnsibleEEEnvConfigMapName
if envConfigMapName == "" {
envConfigMapName = "openstack-aee-default-env"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we verify if this block is okay for backward compatibility? Or is there a better way to approach this?

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

ansibleEE := EEJob{
Name: executionName,
Namespace: deployment.GetNamespace(),
Labels: labels,
EnvConfigMapName: "openstack-aee-default-env",
EnvConfigMapName: envConfigMapName,
}

ansibleEE.NetworkAttachments = aeeSpec.NetworkAttachments
Expand Down
143 changes: 143 additions & 0 deletions test/functional/dataplane/openstackdataplanenodeset_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1811,4 +1811,147 @@ var _ = Describe("Dataplane NodeSet Test", func() {
})
})
})

When("A NodeSet is created without specifying ansibleEEEnvConfigMapName", func() {
BeforeEach(func() {
nodeSetSpec := DefaultDataPlaneNodeSetSpec("edpm-compute")
nodeSetSpec["preProvisioned"] = true
nodeSetSpec["services"] = []string{"bootstrap"}
// NOT setting ansibleEEEnvConfigMapName - should use default

DeferCleanup(th.DeleteInstance, CreateNetConfig(dataplaneNetConfigName, DefaultNetConfigSpec()))
DeferCleanup(th.DeleteInstance, CreateDNSMasq(dnsMasqName, DefaultDNSMasqSpec()))
DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, nodeSetSpec))
DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(dataplaneDeploymentName, DefaultDataPlaneDeploymentSpec()))
CreateSSHSecret(dataplaneSSHSecretName)
CreateCABundleSecret(caBundleSecretName)
SimulateDNSMasqComplete(dnsMasqName)
SimulateIPSetComplete(dataplaneNodeName)
SimulateDNSDataComplete(dataplaneNodeSetName)
})

It("Should use default openstack-aee-default-env ConfigMap in the Job", func() {
Eventually(func(g Gomega) {
ansibleeeName := types.NamespacedName{
Name: fmt.Sprintf("bootstrap-%s-%s", dataplaneDeploymentName.Name, dataplaneNodeSetName.Name),
Namespace: namespace,
}
ansibleEE := GetAnsibleee(ansibleeeName)

// Verify EnvFrom references the default ConfigMap
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom).To(HaveLen(1))
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom[0].ConfigMapRef).NotTo(BeNil())
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom[0].ConfigMapRef.LocalObjectReference.Name).To(Equal("openstack-aee-default-env"))
g.Expect(*ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom[0].ConfigMapRef.Optional).To(BeTrue())
}, th.Timeout, th.Interval).Should(Succeed())
})
})

When("A NodeSet is created with custom ansibleEEEnvConfigMapName", func() {
BeforeEach(func() {
nodeSetSpec := DefaultDataPlaneNodeSetSpec("edpm-compute")
nodeSetSpec["preProvisioned"] = true
nodeSetSpec["services"] = []string{"bootstrap"}
nodeSetSpec["ansibleEEEnvConfigMapName"] = "custom-ansible-env"

DeferCleanup(th.DeleteInstance, CreateNetConfig(dataplaneNetConfigName, DefaultNetConfigSpec()))
DeferCleanup(th.DeleteInstance, CreateDNSMasq(dnsMasqName, DefaultDNSMasqSpec()))
DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, nodeSetSpec))
DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(dataplaneDeploymentName, DefaultDataPlaneDeploymentSpec()))
CreateSSHSecret(dataplaneSSHSecretName)
CreateCABundleSecret(caBundleSecretName)
SimulateDNSMasqComplete(dnsMasqName)
SimulateIPSetComplete(dataplaneNodeName)
SimulateDNSDataComplete(dataplaneNodeSetName)
})

It("Should use custom ConfigMap name from NodeSet in the Job", func() {
Eventually(func(g Gomega) {
ansibleeeName := types.NamespacedName{
Name: fmt.Sprintf("bootstrap-%s-%s", dataplaneDeploymentName.Name, dataplaneNodeSetName.Name),
Namespace: namespace,
}
ansibleEE := GetAnsibleee(ansibleeeName)

// Verify EnvFrom references the custom ConfigMap from NodeSet
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom).To(HaveLen(1))
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom[0].ConfigMapRef).NotTo(BeNil())
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom[0].ConfigMapRef.LocalObjectReference.Name).To(Equal("custom-ansible-env"))
}, th.Timeout, th.Interval).Should(Succeed())
})
})

When("A Deployment overrides NodeSet's ansibleEEEnvConfigMapName", func() {
BeforeEach(func() {
nodeSetSpec := DefaultDataPlaneNodeSetSpec("edpm-compute")
nodeSetSpec["preProvisioned"] = true
nodeSetSpec["services"] = []string{"bootstrap"}
nodeSetSpec["ansibleEEEnvConfigMapName"] = "nodeset-ansible-env"

deploymentSpec := DefaultDataPlaneDeploymentSpec()
deploymentSpec["ansibleEEEnvConfigMapName"] = "deployment-override-env"

DeferCleanup(th.DeleteInstance, CreateNetConfig(dataplaneNetConfigName, DefaultNetConfigSpec()))
DeferCleanup(th.DeleteInstance, CreateDNSMasq(dnsMasqName, DefaultDNSMasqSpec()))
DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, nodeSetSpec))
DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(dataplaneDeploymentName, deploymentSpec))
CreateSSHSecret(dataplaneSSHSecretName)
CreateCABundleSecret(caBundleSecretName)
SimulateDNSMasqComplete(dnsMasqName)
SimulateIPSetComplete(dataplaneNodeName)
SimulateDNSDataComplete(dataplaneNodeSetName)
})

It("Should use Deployment's ConfigMap name instead of NodeSet's", func() {
Eventually(func(g Gomega) {
ansibleeeName := types.NamespacedName{
Name: fmt.Sprintf("bootstrap-%s-%s", dataplaneDeploymentName.Name, dataplaneNodeSetName.Name),
Namespace: namespace,
}
ansibleEE := GetAnsibleee(ansibleeeName)

// Verify EnvFrom references the Deployment's ConfigMap (override)
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom).To(HaveLen(1))
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom[0].ConfigMapRef).NotTo(BeNil())
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom[0].ConfigMapRef.LocalObjectReference.Name).To(Equal("deployment-override-env"))
}, th.Timeout, th.Interval).Should(Succeed())
})
})

When("A Deployment without ansibleEEEnvConfigMapName uses NodeSet's value", func() {
BeforeEach(func() {
nodeSetSpec := DefaultDataPlaneNodeSetSpec("edpm-compute")
nodeSetSpec["preProvisioned"] = true
nodeSetSpec["services"] = []string{"bootstrap"}
nodeSetSpec["ansibleEEEnvConfigMapName"] = "nodeset-specific-env"

// Deployment does NOT set ansibleEEEnvConfigMapName
deploymentSpec := DefaultDataPlaneDeploymentSpec()

DeferCleanup(th.DeleteInstance, CreateNetConfig(dataplaneNetConfigName, DefaultNetConfigSpec()))
DeferCleanup(th.DeleteInstance, CreateDNSMasq(dnsMasqName, DefaultDNSMasqSpec()))
DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, nodeSetSpec))
DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(dataplaneDeploymentName, deploymentSpec))
CreateSSHSecret(dataplaneSSHSecretName)
CreateCABundleSecret(caBundleSecretName)
SimulateDNSMasqComplete(dnsMasqName)
SimulateIPSetComplete(dataplaneNodeName)
SimulateDNSDataComplete(dataplaneNodeSetName)
})

It("Should use NodeSet's ConfigMap name when Deployment doesn't specify one", func() {
Eventually(func(g Gomega) {
ansibleeeName := types.NamespacedName{
Name: fmt.Sprintf("bootstrap-%s-%s", dataplaneDeploymentName.Name, dataplaneNodeSetName.Name),
Namespace: namespace,
}
ansibleEE := GetAnsibleee(ansibleeeName)

// Verify EnvFrom references NodeSet's ConfigMap (no override)
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom).To(HaveLen(1))
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom[0].ConfigMapRef).NotTo(BeNil())
g.Expect(ansibleEE.Spec.Template.Spec.Containers[0].EnvFrom[0].ConfigMapRef.LocalObjectReference.Name).To(Equal("nodeset-specific-env"))
}, th.Timeout, th.Interval).Should(Succeed())
})
})
})