Remove old slapproxy tables when migrating to new version
Some tools are using directly slapproxy sqlite database and they stopped working when table number was increased in !76 .
We made a quick improvement to the tools so that they can autodetect the current table names instead of hardcoding ( slapos@b8c895cd ) but this failed, because slapproxy remove did not remove the old tables after migration. This branch is about removing the table, to quickly repair these tools.
The long term plan is (hopefully) to improve these tools so that they don't use the private tables like this.
Merged
by 
The changes were merged into master. The source branch has been removed.
-
-
slapos/tests/test_slapproxy/__init__.py → slapos/tests/test_slapproxy.py1420 1438 1421 slave_list = self.db.execute("select * from slave12").fetchall() 1439 slave_list = self.db.execute("select * from slave13").fetchall() 1422 1440 self.assertEqual( 1423 1441 slave_list, 1424 1442 [] 1425 1443 ) 1426 1444 1427 partition_network_list = self.db.execute("select * from partition_network12").fetchall() 1445 partition_network_list = self.db.execute("select * from partition_network13").fetchall() 1428 1446 self.assertEqual( 1429 1447 partition_network_list, 1430 1448 [(u'slappart0', u'computer', u'slappart0', u'127.0.0.1', u'255.255.255.255'), (u'slappart0', u'computer', u'slappart0', u'fc00::1', u'ffff:ffff:ffff::'), (u'slappart1', u'computer', u'slappart1', u'127.0.0.1', u'255.255.255.255'), (u'slappart1', u'computer', u'slappart1', u'fc00::1', u'ffff:ffff:ffff::'), (u'slappart2', u'computer', u'slappart2', u'127.0.0.1', u'255.255.255.255'), (u'slappart2', u'computer', u'slappart2', u'fc00::1', u'ffff:ffff:ffff::'), (u'slappart3', u'computer', u'slappart3', u'127.0.0.1', u'255.255.255.255'), (u'slappart3', u'computer', u'slappart3', u'fc00::1', u'ffff:ffff:ffff::'), (u'slappart4', u'computer', u'slappart4', u'127.0.0.1', u'255.255.255.255'), (u'slappart4', u'computer', u'slappart4', u'fc00::1', u'ffff:ffff:ffff::'), (u'slappart5', u'computer', u'slappart5', u'127.0.0.1', u'255.255.255.255'), (u'slappart5', u'computer', u'slappart5', u'fc00::1', u'ffff:ffff:ffff::'), (u'slappart6', u'computer', u'slappart6', u'127.0.0.1', u'255.255.255.255'), (u'slappart6', u'computer', u'slappart6', u'fc00::1', u'ffff:ffff:ffff::'), (u'slappart7', u'computer', u'slappart7', u'127.0.0.1', u'255.255.255.255'), (u'slappart7', u'computer', u'slappart7', u'fc00::1', u'ffff:ffff:ffff::'), (u'slappart8', u'computer', u'slappart8', u'127.0.0.1', u'255.255.255.255'), (u'slappart8', u'computer', u'slappart8', u'fc00::1', u'ffff:ffff:ffff::'), (u'slappart9', u'computer', u'slappart9', u'127.0.0.1', u'255.255.255.255'), (u'slappart9', u'computer', u'slappart9', u'fc00::1', u'ffff:ffff:ffff::')] 1431 1449 ) 1432 1450 1451 old_table_list = self.db.execute("SELECT name FROM sqlite_master WHERE type='table' and (name = 'software9' or name = 'software10')").fetchall() -
Can you just add a comment here ?
# Check that old tables have been removed -
I think the test would be more robust if we do :
software_table_list = self.db.execute("SELECT name FROM sqlite_master WHERE type='table' and name='software%'").fetchall()and then
assertEqual(software_table_list, ['software13']) -
Ah good point, it's always better to assert what should be here instead of asserting what should be not here, there's always so many possible reasons for things to be not here.
I think we can assert the exact tables like this:
software_table_list = self.db.execute("SELECT name FROM sqlite_master WHERE type='table' ").fetchall() self.assertEqual(sorted(software_table_list), sorted(['software13', ... ]))I'm amending the patch
-
-
slapos/proxy/views.py
176 177 if current_schema_version == DB_VERSION: 177 178 return 178 179 180 previous_table_list = _getTableList() 181 # first, make a backup of current database 182 if current_schema_version != '-1': 183 backup_file_name = "{}-backup-{}to{}-{}".format( -
Can you add a
.sqlextension to this file ? Then it will be obvious to user that it's a dump, so it will give information on how to be used. -
Yes, that's good idea.
I also want to change to use
datetime.now().isoformat()instead ofstr(datetime.now())because the former has a space character. Now output filename will look like:/tmp/tmp_s3v581x/lib/proxy.db-backup-10to13-2019-03-19T04:23:35.652447.sql
-
-
slapos/tests/test_slapproxy/__init__.py → slapos/tests/test_slapproxy.py
1383 'database_dump_version_10.sql' 1383 'slapos.tests', 1384 os.path.join('test_slapproxy', 'database_dump_version_10.sql') 1384 1385 )) % dict(version='12') 1385 1386 self.db = sqlite_connect(self.proxy_db) 1386 1387 self.db.cursor().executescript(schema) 1387 1388 self.db.commit() 1388 1389 1389 1390 def test_automatic_migration(self): 1391 # create an old table, to assert it is properly removed. 1392 self.db.execute("create table software9 (int a)") 1393 self.db.commit() 1394 1390 1395 table_list = ('software12', 'computer12', 'partition12', 'slave12', 'partition_network12') 1391 1396 for table in table_list: 1392 1397 self.assertRaises(sqlite3.OperationalError, self.db.execute, "SELECT name FROM computer12") -
tableis not used here...
-
-
-
yes, I think you're correct. The test was doing "check that no version12 of tables exist" (and it was badly implemented because it was not using
tablevariable). And now you are doing "check that only version 10 of tables exist" which is even better
-
-
-
I did one more suggestion but I'm totally OK to merge as is.
-
(no objection to merge from me)
-
I noticed that I did not update the name of the test class
TestMigrateVersion10To12and I think I misunderstood this comment added in 3f988f40 :# XXX: when testing new schema version, # rename to "TestMigrateVersion10ToLatest" and test accordingly. # Of course, also test version 11 to latest (should be 12). class TestMigrateVersion10To11(TestInformation, TestRequest, TestSlaveRequest, TestMultiNodeSupport):I feel the intention was that each time we version up the tables, we create a new test based on this, so we should have by now
TestMigrateVersion10To13,TestMigrateVersion11To13andTestMigrateVersion12To13(and I believe they should not be named...To13but...ToLatest, so that we don't have to update). I will produce dumps and add these tests. -
Toggle commit list
-
Toggle commit list
-
Edited
I implemented what I suggested for migration tests in 18c814a1 this changes only the test, but it's not so small, so it's better if you can take another look. @rafael you commited 3f988f40 with Cédric. Maybe you remember the intention for migration tests ? Or at least can confirm that what I implemented here is not in wrong direction.
-
OK to merge for me.
note that Rafael is off today.
-
He already agreed on the general idea, the remaining part is only about test, so if we want to make a new webrunner release to fix the problem with resilience on updated runners, I feel we can merge this and continue with making a release.
This reminds me that I wanted to add a changelog entries, something like:
* ``slapproxy``: remove old tables when running migration. A backup is made as a separate sql file. * ``slapproxy``: update database version to 13, to force removal of old tables. -
Ok to merge from my side.
-
merged
Toggle commit list
-
Please register or sign in to post a comment