feat: replace ansible with PyMySQL for DB Creation #289

intlabs · 2017-03-16T19:13:35Z

What is the purpose of this pull request?: To replace ansible with PyMySQL for DB Creation

What issue does this pull request address?: Superceeds #206, which in turn superseded #140.

Notes for reviewers to consider:
This PR will limit its scope to helm-toolkit and keystone until we have resolved the path ahead, once consensus is reached it will be expanded to the remaining services.

Specific reviewers for pull request:
@alanmeadows @v1k0d3n

v1k0d3n · 2017-03-17T12:26:51Z

you'll have to let me know when you want this reviewed. i was just about to pull in the other work from #206, and found you sprawled into this new PR.

intlabs · 2017-03-17T14:46:24Z

@v1k0d3n np, I'll remove the wip when it's ready :)

The PR's in #206 are still needed for this to be functional:

clean: Update Neutron Keystone Jobs to match common architecture #275
clean: Update Glance Keystone Jobs to match common architecture #281
clean: Update Nova Keystone Jobs to match common architecture #273
As they make changes to the job layout that will cause conflicts with this work.

A new PR was created as after 1 month it's simply not feasible to rebase :/

intlabs · 2017-03-22T14:42:05Z

@alanmeadows, @v1k0d3n if you could have a look at this, its ready for initial review. Once we've got this looking sweet I'll expand the concept out to all the other charts.

wilkers-steve

Unless I'm missing something, this looks good to me @intlabs. I'm definitely looking forward to this change getting merged in and expanding the work to the other charts.

alanmeadows

Looks good @intlabs. A few comments, nothing major.

alanmeadows · 2017-03-31T23:40:00Z

helm-toolkit/templates/scripts/_db-init.py.tpl

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+{{- define "helm-toolkit.db_init" }}


I still recommend we adopt full paths (even though I am sometimes not following my own advice), to locate where these methods are physically located:

define "helm-toolkit.scripts.db_init"

Also, we can probably do better then scripts, perhaps keystone foo, db foo, and so on.

I agree with this sentiment, can we hit this in a separate PR though, so as to minimise disruption to work in flight?

Agree with @alanmeadows here, I think it would be best to add a bit more detail to the path within helm-toolkit.

I agree w/ @intlabs that this needs to be in another PR. and documented in an issue.

alanmeadows · 2017-03-31T23:43:14Z

helm-toolkit/templates/scripts/_db-init.py.tpl

+    print("Created database {0}".format(database))
+except:
+    print("Could not create database {0}".format(database))
+    sys.exit(1)


I suppose you do not want to involve the standard logger for simplicity reasons (which could use LOG.traceback(foo) but this exception will lack a traceback that may include some hint at why if one should ever need it.

There is a balance between overloading the user and providing them curt errors but I'd appreciate a traceback if an action like this fails as sqlalchemy may provide some hint from the engine as to what went wrong.

Alternative to logger could be if your really trying to be print-locked:

import traceback print traceback.format_exc()

no reason to be bound to print. It was just the quickest and simplest way to implement this. I'm fine using the standard logger, or adding tracebacks.

alanmeadows · 2017-03-31T23:43:42Z

helm-toolkit/templates/scripts/_db-init.py.tpl

+            database, user, password))
+    print("Created user {0} for {1}".format(user, database))
+except:
+    print("Could not create user {0} for {1}".format(user, database))


Same, re: traceback.

alanmeadows · 2017-03-31T23:43:54Z

helm-toolkit/templates/scripts/_db-init.py.tpl

+    print 'Database connection for user ok'
+except:
+    print 'Could not connect to database as user'
+    sys.exit(1)


Same, re: traceback.

alanmeadows · 2017-03-31T23:46:10Z

keystone/templates/job-db-init.yaml

+                secretKeyRef:
+                  name: keystone-db-root
+                  key: DB_CONNECTION
+            - name: OPENSTACK_CONFIG_FILE


I like this approach. No need to pass data to this thing thats already in the config.

alanmeadows · 2017-03-31T23:47:11Z

helm-toolkit/templates/scripts/_db-init.py.tpl

+        print("Trying to load db config from {0}:{1}".format(
+            os_conf_section, os_conf_key))
+        user_db_conn = config.get(os_conf_section, os_conf_key)
+        print("Got config from {0}".format(os_conf))


For debugging do we perhaps want to print what we're working with (the final resulting db connect string) or do we feel security would be compromised?

Plus @v1k0d3n for thoughts on stdout containing sensitive usernames and passwords. Helps debugging. Hurts security.

@alanmeadows I'd be very hesitant to do that as a result of the security concerns you highlight. Printing passwords to stdout seems like a recipe for having a bad time to me.

if we're getting down to it, we're already sending passwords/secrets in the clear today (via configmaps, and then sending them on to tiller).

i think we should fix the security issues we have soon, and start planning for proper secrets management (it's on the roadmap), but i also think we should include a helpful debug mechanism like @alanmeadows is suggesting. otherwise it just gets too magical for the average user starting to work with openstack-helm.

hopefully with a proper combination of the two, we'll give users/developers confidence that when they use debugging in OSH, passwords/secrets are only being passed at their command, which would make @intlabs and me much more happy.

I am removing my copywrite and transfering it to the OpenStack-Helm Authors for the DB Managment Script. As although this was primarily written while an independant OSS Developer, it is not currently present in any other codebase and will only be merged post commencing work on the project in an official capacity. This does not affect the copywrite of any other code contibuted to the project by myself or any other party.

larryrensing

This looks great @intlabs, nice work.

larryrensing · 2017-04-05T14:01:13Z

helm-toolkit/templates/scripts/_db-init.py.tpl

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+{{- define "helm-toolkit.db_init" }}


Agree with @alanmeadows here, I think it would be best to add a bit more detail to the path within helm-toolkit.

wilkers-steve

@intlabs This looks good to me. I'm leaning towards @alanmeadows and @larryrensing suggestions that we work with the paths a bit, but that should be handled separately. I don't see a reason for that to block getting this in finally.

v1k0d3n added the in progress label Mar 16, 2017

intlabs changed the title ~~wip: Update toolkit functions to include DB Endpoint lookups~~ wip: replace ansible with PyMySQL for DB Creation Mar 16, 2017

intlabs mentioned this pull request Mar 16, 2017

Replace Ansible with SQLAlchemy/PyMySQL for database creation. #206

Closed

intlabs mentioned this pull request Mar 18, 2017

clean: Update Nova Keystone Jobs to match common architecture #273

Merged

This was referenced Mar 18, 2017

clean: Update Glance Keystone Jobs to match common architecture #281

Merged

clean: Update Neutron Keystone Jobs to match common architecture #275

Merged

v1k0d3n changed the title ~~wip: replace ansible with PyMySQL for DB Creation~~ feat: replace ansible with PyMySQL for DB Creation Mar 18, 2017

v1k0d3n changed the title ~~feat: replace ansible with PyMySQL for DB Creation~~ wip: replace ansible with PyMySQL for DB Creation Mar 18, 2017

intlabs changed the title ~~wip: replace ansible with PyMySQL for DB Creation~~ feat: replace ansible with PyMySQL for DB Creation (do not merge) Mar 22, 2017

v1k0d3n added this to the 0.3.0 milestone Mar 22, 2017

v1k0d3n assigned intlabs Mar 22, 2017

v1k0d3n requested review from alanmeadows and v1k0d3n March 22, 2017 19:02

intlabs changed the title ~~feat: replace ansible with PyMySQL for DB Creation (do not merge)~~ feat: replace ansible with PyMySQL for DB Creation (do not merge - ready for review) Mar 29, 2017

v1k0d3n added the needs core review label Mar 29, 2017

wilkers-steve approved these changes Mar 31, 2017

View reviewed changes

alanmeadows reviewed Mar 31, 2017

View reviewed changes

intlabs added 9 commits April 4, 2017 11:33

Add Python DB Init Script

f88c401

PyMySQL Keystone Jobs

2a2ae0d

Update DB Init script to include tracebacks and improved output

549bfea

Update Glance DB Jobs

64bbf67

Update Cinder DB Jobs

105349a

Update Neutron DB Jobs

a0092c7

Update Cinder DB Jobs

90c4496

Update Heat DB Jobs

c0141e1

intlabs changed the title ~~feat: replace ansible with PyMySQL for DB Creation (do not merge - ready for review)~~ feat: replace ansible with PyMySQL for DB Creation Apr 4, 2017

intlabs requested a review from larryrensing April 4, 2017 20:23

larryrensing approved these changes Apr 5, 2017

View reviewed changes

wilkers-steve approved these changes Apr 6, 2017

View reviewed changes

wilkers-steve merged commit d0a9bd2 into att-comdev:master Apr 6, 2017

v1k0d3n removed the in progress label Apr 6, 2017

intlabs mentioned this pull request Apr 6, 2017

clean: refactor helm-toolkit paths #329

Open

feat: replace ansible with PyMySQL for DB Creation #289

feat: replace ansible with PyMySQL for DB Creation #289

Uh oh!

Conversation

intlabs commented Mar 16, 2017

Uh oh!

v1k0d3n commented Mar 17, 2017

Uh oh!

intlabs commented Mar 17, 2017

Uh oh!

intlabs commented Mar 22, 2017

Uh oh!

wilkers-steve left a comment

Choose a reason for hiding this comment

Uh oh!

alanmeadows left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intlabs Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanmeadows Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larryrensing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilkers-steve left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

intlabs Apr 3, 2017 •

edited

Loading

alanmeadows Mar 31, 2017 •

edited

Loading