Merge lp:~foli/landscape-charm/tools-fixes into lp:~landscape/landscape-charm/tools

[Michael Foley]

db-migrator: correct store_name for account, add another exclude, print dump/restore commands

the printing of the dump/restore commands is optional, they can be removed on merge if unwanted, but I have found them useful in debugging and to see progress.

This is for work on cRT#92053.

[Chad Smith (chad.smith)]

+1 on this correction to tools store_name. Thanks Michael.

review: Approve

[lp:~foli/landscape-charm/tools-fixes]

24. By Michael Foley on 2016-08-22

[foli] db-migrator: add generic table exclude argument option, cRT#92053

25. By Michael Foley on 2016-08-22

[foli] db-migrator: add generic table exclude argument option, cRT#92053

[Michael Foley (foli)]

Have additionally added a generic table exclude argument option

[lp:~foli/landscape-charm/tools-fixes]

26. By Michael Foley on 2016-08-22

[foli] db-migrator: add generic table exclude argument option, cRT#92053

[Chad Smith (chad.smith)]

While the exclude tables argument is good for quick dev and migration checking, I'm sort of averse to adding a generic exclude tables argument.

All of these exclude tables arguments in this tool in general feel like a bandaid for something that landscape broke by not properly dropping a table that we no longer use when we removed it from our model. Hard-coding these known shortcomings in the code is the lesser evil than adding the flexibility to specify any exclude-tables on the commandline because it raises our awareness of patches that landscape now needs to clean up these orphaned tables.

If we further extend the db-migrator tool to allow users to override the intended behavior, we risk losing the data (the feedback loop from the pain of changing the tool) which was needed to just get the migration to work in the first place.

to be noted, the issues you have run into currently with ec2_region and change_package_lock_request and metadata_*_seq have each resulted in landscape db patches in trunk for our next release (so this won't hit other customers at some point).

[Chad Smith (chad.smith)]

Do you think we can drop the additional exclude-tables argument, but accept the rest of the patch?

review: Needs Information

[lp:~foli/landscape-charm/tools-fixes]

27. By Michael Foley on 2016-08-24

[foli] db-migrator: revert generic table exclude argument option

[Michael Foley (foli)]

> Do you think we can drop the additional exclude-tables argument, but accept
> the rest of the patch?

That is fine by me. I was not clear on what the original intention of the tool was, so your arguments make sense to me.

As you said the issues I have run into are being patched and handled so I don't need the generic exclude tables ability. I have been given a command to manually rename the metadata_id_seq sequence to annotation_id_seq, and I will just have to remove extraneous backup tables that someone created in one of our databases to make it clean before migration.

I have reverted my most recent changes, only leaving the "--exclude-table=change_package_lock_request*" and s/account/account-1/ fixes, as well as leaving printing the dump/restore commands being run.

[Alberto Donato (ack)]

Looks good, +1.

I have a minor comment inline

review: Approve

[Chad Smith (chad.smith)]

review: Approve

=== modified file 'db-migrator.py'
--- db-migrator.py	2016-08-18 17:45:48 +0000
+++ db-migrator.py	2016-08-24 22:18:21 +0000
@@ -163,9 +163,10 @@
     ]
     if store_name == "main":
         options.append("--exclude-table=ec2_region*")
-    if store_name == "account":
+    if store_name == "account-1":
         options.append("--exclude-table=create_package_lock_request*")
         options.append("--exclude-table=remove_package_lock_request*")
+        options.append("--exclude-table=change_package_lock_request*")
     user = stores_info["admin"]["user"]
     if user:
         options.append("--user={}".format(user))
@@ -175,6 +176,7 @@
     env = os.environ.copy()
     if password:
         env["PGPASSWORD"] = password
+    print "Running: {}".format(" ".join(cmd))
     subprocess.check_call(cmd, env=env)
 
 
@@ -199,6 +201,7 @@
     env = os.environ.copy()
     if password:
         env["PGPASSWORD"] = password
+    print "Running: {}".format(" ".join(cmd))
     subprocess.check_output(cmd, env=env)