-
Notifications
You must be signed in to change notification settings - Fork 102
Fix: Added customizable ansibleEE env ConfigMap name #1772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
oliashish
wants to merge
1
commit into
main
Choose a base branch
from
fix/configurable-envConfigMapName
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+247
−5
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
| } | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there is a loop for handling different env vars per NodeSet
Only if env is explicitly set in deployment otherwise it is used from what is in Nodeset.