Fix: Added customizable ansibleEE env ConfigMap name #1772
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oliashish The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/d91bb4d5006d46cb8f53bf9db1e962da ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 12m 34s |
Currently there is only one hardcoded string `openstack-aee-default-env` for ansibleEE environment variable, which is applied throughout the cluser. This PR tries to let users ability to customize the ansibleEE env configMap for deployments and nodeset levels individually. The precedence order for ansibleEEEnvConfigMap is deployment > nodeset > default.
oliashish
force-pushed
the
fix/configurable-envConfigMapName
branch
from
b1bf4a9 to
0822da4
Compare
oliashish
changed the title
| if envConfigMapName == "" { | ||
| envConfigMapName = "openstack-aee-default-env" | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we verify if this block is okay for backward compatibility? Or is there a better way to approach this?
| // If not specified, defaults to "openstack-aee-default-env". | ||
| // This can be overridden at the Deployment level. | ||
| AnsibleEEEnvConfigMapName string `json:"ansibleEEEnvConfigMapName,omitempty"` | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if envConfigMapName == "" { | ||
| envConfigMapName = "openstack-aee-default-env" | ||
| } | ||
|
|
|
/retest |
|
@oliashish: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Currently there is only one hardcoded string
openstack-aee-default-envfor ansibleEE environment variable, which is applied throughout the cluser.This PR tries to let users ability to customize the ansibleEE env configMap for deployments and nodeset levels individually. The precedence order for ansibleEEEnvConfigMap is deployment > nodeset > default.
Test Cases
Jira: OSPRH-18955