nexedi / slapos.core

added 2 commits

• 70cfde38 - slapproxy: remove old tabless on migration
• 0820c816 - slapproxy: update database version to 13

Compare with previous version

mentioned in merge request slapos!532

test on python 2, test on python3

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

Can you add a .sql extension 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 of str(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

Thanks @jerome for pushing this MR so fast ! I've added 2 minor ideas. Otherwise this MR seems very good to me.

added 3 commits

• 3483561d - test: rename test_slapproxy so that pytest can find it
• b66f6714 - slapproxy: remove old tables on migration
• 3ef2c7d0 - slapproxy: update database version to 13

Compare with previous version

tests are OK after latest amendments: python2 / python3

@tomo @rafael @alain.takoudjou let's merge this, make a slapos.core release, update slapprunner profiles to use the new slapos.core, tag a new slapos release and update webrunners ?

table is not used here...

True, I just ignored this, but it's wrong ( since automatic migration was introduced - 6171fe46 )

I think this code was trying to assert that new tables are not already there at the beginning of the test. I'll try to fix this part of the test too.

added 3 commits

• ed6c77d5 - test: fix a strange assertion on test_slapproxy
• 51d3b297 - slapproxy: remove old tables on migration
• 251590d0 - slapproxy: update database version to 13

Compare with previous version

@tomo please take another look, I applied the changes you suggested. Tests is still ok: py3 / py2

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 table variable). And now you are doing "check that only version 10 of tables exist" which is even better

would it be possible to have 'computer{}'.format(DB_VERSION) ? I'm not sure this is feasible or desirable but I'm just suggesting.

Yes it seems good idea. I refactored the test for upgrade to define the version in only one place and get table names dynamically like this.

thanks for implementing this !

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 TestMigrateVersion10To12 and 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, TestMigrateVersion11To13 and TestMigrateVersion12To13 (and I believe they should not be named ...To13 but ...ToLatest, so that we don't have to update). I will produce dumps and add these tests.

added 3 commits

• 2f83e1e7 - slapproxy: remove old tables on migration
• 18c814a1 - test_slapproxy: Review test for migrations
• 96ef944b - slapproxy: update database version to 13

Compare with previous version

added 1 commit

• 1b83da94 - slapproxy: update database version to 13

Compare with previous version

added 1 commit

• d0ce721b - slapproxy: update database version to 13

Compare with previous version

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.

tests are OK, py2 / py3

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.

mentioned in merge request slapos.package!77

Ok to merge from my side.

merged

mentioned in commit c0912336

mentioned in commit slapos.package@af5a60eb

@jerome I can see a webrunner version 102 (with slapos.core containing this MR, see https://lab.nexedi.com/nexedi/slapos/blob/1.0.102/stack/slapos.cfg#L138) whose database wasn't migrated to version 13. All tables are still version 11.

Can you describe exactly when the migration should happen ? It seems that we use global variable is_schema_already_executed https://lab.nexedi.com/nexedi/slapos.core/blob/d0ce721bfcefd3094266241861224f7315d13a1d/slapos/proxy/views.py#L223 to see whether we should execute _upgradeDatabaseIfNeeded. Webrunner was restarted and slapproxy of webrunner was also restarted but there is still no migration.

/cc @rafael @luke as I think some tickets are related to this problem.

Can you describe exactly when the migration should happen ?

This is a flask "before request" hook that should be called before each request made to slap proxy. Maybe no request was made to the slap proxy yet ? Can you try to run something like slapos proxy show or maybe just curl the url of proxy ?

Webrunner was restarted and slapproxy of webrunner was also restarted

Yes, I'm not sure there's already anything to restart automatically this slapproxy service when webrunner is restarted.