Replace Ansible with SQLAlchemy/PyMySQL for database creation. #206
Conversation
intlabs
changed the title
intlabs
changed the title
alanmeadows
left a comment
alanmeadows
left a comment
There was a problem hiding this comment.
Good start Pete. A few comments below.
| @@ -0,0 +1,12 @@ | |||
| {{- define "helm-toolkit.secret_db_root" }} | |||
There was a problem hiding this comment.
This is not named after the file that contains it.
| @@ -0,0 +1,2 @@ | |||
| {{- $envAll := . }} | |||
| {{- include "helm-toolkit.secret_db_root" $envAll }} | |||
There was a problem hiding this comment.
Because this toolkit script leverages only values from .Values.database I would recommend that is what we pass. We have a mix between passing macros $envAll or . and the leafs that contain the data they need. We should standardize on one. I prefer leafs (.Values.database as its clear what input we are passing the macro but I'm flexible.
There was a problem hiding this comment.
That approach works for me.
There was a problem hiding this comment.
This work (and comments) have been made moot by: https://github.com/att-comdev/openstack-helm/pull/262/files#diff-728a997bda2af0665cbf1fde10e4231fR153, which is a better approach. For now I'll adopt this method verbatim, to allow work to continue on this PR.
| ip_address: "0.0.0.0" | ||
|
|
||
| database: | ||
| secret: |
There was a problem hiding this comment.
It's not clear to me how this differs from the root_password below
There was a problem hiding this comment.
This is the name of the secret, rather than the actual password. I'll try to make this clearer in a follow-up commit.
There was a problem hiding this comment.
That makes much more sense. I think just secret_name vs secret would of made me realize this.
There was a problem hiding this comment.
This will be refactored to match the style here: https://github.com/alanmeadows/aic-helm/blob/b40a7bf3be5bcc9960452b0ad72c6db9932f101e/keystone/values.yaml#L303. I'll insert the key k8s_secret_name as appropriate within this to define the name of the secret that will hold the derived SQLAlchemy connection string.
| secretKeyRef: | ||
| name: {{ .Values.database.secret.root }} | ||
| key: DB_CONNECTION | ||
| - name: OPENSTACK_CONFIG_FILE |
There was a problem hiding this comment.
Seems like this will keep growing, maybe our checker-helpers needs its own oslo config file which is just a config map? This may grow to unwieldy proportions. Understandably could be future work.
There was a problem hiding this comment.
The idea here is to consume the same config file as the service will consume, the job itself is what defines the scope here I think. Unless we are across purposes?
| mountPath: /tmp/db-create.py | ||
| subPath: db-create.py | ||
| readOnly: true | ||
| - name: pod-etc-service |
There was a problem hiding this comment.
Just a minor nit, not sure I fully grasp the naming conventions here... pod-etc-service really is an emptyDir, service-config is keystone.conf... I had to look down below two or three times - which either represents my own senility or the need to make this clearer
There was a problem hiding this comment.
I've used the naming conventions from harbor here:
- Prefixes:
- pod: represents a volume that is local to the pod, almost always an emptyDir
- service: a volume that is common the service, almost always a configMap, but may be a shared volume
- Suffixes:
- The mount location of the volume if only used once.
- If a configmap, just configmap (unless several are used, in which case I added a further suffix with some brief indicator of the contents)
Of course this comment contradicts reality, unless there is objection I'll update to make the above true.
|
To resume work on this PR we should merge the following:
As they make changes to the job layout that will cause conflicts with this work. |
|
Superseded by #289 |
3 participants
What is the purpose of this pull request?: To replace ansible with PyMySQL for DB Creation
What issue does this pull request address?: This is part of the effort to become image agnostic.
Notes for reviewers to consider: This PR is intended to supersede #140
Specific reviewers for pull request: @alanmeadows, @srwilkers, @v1k0d3n