slapproxy: Make rows unique for forwarded requests

See merge request nexedi/slapos.core!423

slapos/proxy/schema.sql

---version:15
+--version:16
 CREATE TABLE IF NOT EXISTS local_software_release_root%(version)s (
   path VARCHAR(255)
 );
@@ -54,5 +54,6 @@ CREATE TABLE IF NOT EXISTS partition_network%(version)s (
 
 CREATE TABLE IF NOT EXISTS forwarded_partition_request%(version)s (
   partition_reference VARCHAR(255), -- a.k.a source_instance_id
-  master_url VARCHAR(255)
+  master_url VARCHAR(255),
+  CONSTRAINT uniq PRIMARY KEY (partition_reference, master_url)
 );

slapos/proxy/views.py

@@ -194,7 +194,7 @@ def _upgradeDatabaseIfNeeded():
     for row in execute_db(table, 'SELECT * from %s', db_version=current_schema_version):
       columns = ', '.join(row.keys())
       placeholders = ':'+', :'.join(row.keys())
-      query = 'INSERT INTO %s (%s) VALUES (%s)' % ('%s', columns, placeholders)
+      query = 'INSERT OR REPLACE INTO %s (%s) VALUES (%s)' % ('%s', columns, placeholders)
       execute_db(table, query, row)
   # then drop old tables
   for previous_table in previous_table_list:

slapos/tests/test_slapproxy.py

@@ -1781,7 +1781,7 @@ database_uri = %(rootdir)s/lib/external_proxy.db
     """
     Test there is no instance on local proxy.
     Test there is instance on external proxy.
-    Test there is instance reference in external table of databse of local proxy.
+    Test there is instance reference in external table of database of local proxy.
     """
     # Adjust name to match forwarding prefix
     name = '%s_%s' % (requester or 'user', name)
@@ -1853,7 +1853,7 @@ database_uri = %(rootdir)s/lib/external_proxy.db
 
   def testForwardToMasterInList(self):
     """
-    Test that explicitely asking a master_url in SLA causes
+    Test that explicitly asking a master_url in SLA causes
     proxy to forward request to this master.
     """
     dummy_parameter_dict = {'foo': 'bar'}
@@ -1871,6 +1871,32 @@ database_uri = %(rootdir)s/lib/external_proxy.db
         self.external_master_url
     )
 
+  def testForwardToMasterInList_NoDuplicates(self):
+    """
+    Test that multiple explicitly forwarded requests for the same instance
+    do not result in duplicate entries in the proxy database.
+    """
+    dummy_parameter_dict = {'foo': 'bar'}
+    instance_reference = 'MyFirstInstance'
+    self.format_for_number_of_partitions(1)
+    self.external_proxy_format_for_number_of_partitions(1)
+
+    filter_kw = {'master_url': self.external_master_url}
+
+    for _ in range(2):
+      partition = self.request(self.software_release_not_in_list, None, instance_reference, 'slappart0',
+                               filter_kw=filter_kw, partition_parameter_kw=dummy_parameter_dict)
+
+    # Explicitly do the relevant test, even if _checkInstanceIsFowarded does it too
+    entries = slapos.proxy.views.execute_db('forwarded_partition_request', 'SELECT * from %s', db=self.db)
+    self.assertEqual(len(entries), 1)
+
+    self._checkInstanceIsFowarded(instance_reference, 'slappart0', dummy_parameter_dict, self.software_release_not_in_list)
+    self.assertEqual(
+        partition._master_url,
+        self.external_master_url
+    )
+
   def testForwardToMasterNotInList(self):
     """
     Test that explicitely asking a master_url in SLA causes
@@ -1904,6 +1930,32 @@ database_uri = %(rootdir)s/lib/external_proxy.db
     self.assertEqual(self.external_software_release, partition.getSoftwareRelease())
     self.assertEqual({}, partition.getConnectionParameterDict())
 
+  def testForwardRequest_SoftwareReleaseList_NoDuplicates(self):
+    """
+    Test that multiple automatically forwarded requests for the same instance
+    do not result in duplicate entries in the proxy database.
+    """
+    dummy_parameter_dict = {'foo': 'bar'}
+    instance_reference = 'MyFirstInstance'
+    self.format_for_number_of_partitions(1)
+    self.external_proxy_format_for_number_of_partitions(1)
+
+    for _ in range(2):
+      partition = self.request(self.external_software_release, None, instance_reference, 'slappart0',
+                               partition_parameter_kw=dummy_parameter_dict)
+
+    # Explicitly do the relevant test, even if _checkInstanceIsFowarded does it too
+    entries = slapos.proxy.views.execute_db('forwarded_partition_request', 'SELECT * from %s', db=self.db)
+    self.assertEqual(len(entries), 1)
+
+    self._checkInstanceIsFowarded(instance_reference, 'slappart0', dummy_parameter_dict, self.external_software_release)
+
+    instance_parameter_dict = partition.getInstanceParameterDict()
+    instance_parameter_dict.pop('timestamp')
+    self.assertEqual(dummy_parameter_dict, instance_parameter_dict)
+    self.assertEqual(self.external_software_release, partition.getSoftwareRelease())
+    self.assertEqual({}, partition.getConnectionParameterDict())
+
   def testForwardRequestFromPartition(self):
     """
     Test that instance request is forwarded and requested from computer partition.
@@ -2236,7 +2288,7 @@ class _MigrationTestCase(TestInformation, TestRequest, TestSlaveRequest, TestMul
   """
   dump_filename = NotImplemented
   initial_table_list = NotImplemented
-  current_version = '15'
+  current_version = '16'
 
   def setUp(self):
     TestInformation.setUp(self)
@@ -2317,6 +2369,14 @@ class _MigrationTestCase(TestInformation, TestRequest, TestSlaveRequest, TestMul
         [(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::')]
     )
 
+    # Check that duplicate forwarded requests in the initial table result in a single entry in the new table
+    if any(t.startswith('forwarded_partition_request') for t in self.initial_table_list):
+      forwarded_request_list = self.db.execute("select * from forwarded_partition_request{}".format(self.current_version)).fetchall()
+      self.assertEqual(
+          forwarded_request_list,
+          [('forwarded_instance', 'https://bogus/master/url')]
+      )
+
     # Check that we only have new tables
     table_list = self.db.execute("SELECT name FROM sqlite_master WHERE type='table' ORDER BY name").fetchall()
     self.assertEqual([x[0] for x in table_list],
@@ -2376,4 +2436,10 @@ class TestMigrateVersion14ToLatest(_MigrationTestCase):
   initial_table_list = ['computer14', 'forwarded_partition_request14', 'partition14', 'partition_network14', 'slave14', 'software14', ]
 
 
+class TestMigrateVersion15ToLatest(_MigrationTestCase):
+  dump_filename = 'database_dump_version_15.sql'
+  initial_table_list = ['computer15', 'forwarded_partition_request15', 'local_software_release_root15', 'partition15',
+                        'partition_network15', 'slave15', 'software15', ]
+
+
 del _MigrationTestCase

slapos/tests/test_slapproxy/README.md

+# How to update tests after database version upgrade ?
+
+You've just updated the version of the database in `slapos/proxy/schema.sql`,
+and you need to update the tests. You're in the right place!
+
+Let's call `XX` the current version number and `YY` the new version.
+Let's call `UU` the version number before `XX`.
+
+
+## Prerequisites
+
+A SlapOS node in which command `slapos` uses version `XX` of the database.
+- e.g. a Theia in which you're locally developping `slapos.core`
+
+A Python virtual environment in which the `slapos.core` egg is installed
+from the local repository with your changes, including the version upgrade
+to `YY`.
+
+For example, beside the `slapos.core` repository
+
+```
+$ python3 -m venv pyenv
+$ source pyenv/bin/activate
+(pyenv) $ cd slapos.core
+(pyenv) $ pip install -e .
+(pyenv) $ deactivate
+$
+```
+
+The command `pip install -e <path>` installs the egg from sources at `<path>`
+in editable mode, so that any change the source code is immediately taken
+into account.
+
+This provides a `slapos` command that uses version `YY` of the database
+(assuming `slapos/proxy/schema.sql` has been updated in the source code)
+whenever the virtual environment is activated.
+
+
+## In `test_slapproxy.py`
+
+- In `_MigrationTestCase`, update `current_version` from `XX` to `YY`.
+
+- Add a `TestMigrateVersionXXToLatest` test case (see the existing ones).
+  - If tables were added in version `XX`, adjust the `initial_table_list`.
+
+
+## In `test_slapproxy/`
+
+Go to `test_slapproxy/` (assuming you are at the root of `slapos.core`):
+
+```
+$ cd slapos/tests/test_slapproxy
+```
+
+With the Python virtual environment deactivated, run:
+
+```
+$ ./generate_dump.sh computer database_dump_version_UU.sql database_dump_version_XX.sql
+```
+
+This will call `slapos proxy start` with the `slapos` that uses version `XX`,
+load the `database_dump_version_UU.sql` and migrate it to version `XX`, and
+then dump it into a new file `database_dump_version_XX.sql`.
+
+Now activate the virtual environment and run:
+```
+(pyenv) $ ./generate_dump.sh slaprunner database_dump_version_current.sql database_dump_version_current.sql
+```
+
+This will call `slapos proxy start` with `slapos` from the virtual environment
+which has version `YY`, load `database_dump_version_current.sql` which is in
+version `XX`, migrate it to `YY`, and then dump it back overwriting the file.

slapos/tests/test_slapproxy/database_dump_version_11.sql

@@ -120,4 +120,6 @@ CREATE TABLE forwarded_partition_request11 (
   partition_reference VARCHAR(255), -- a.k.a source_instance_id
   master_url VARCHAR(255)
 );
+INSERT INTO "forwarded_partition_request11" VALUES('forwarded_instance','https://bogus/master/url');
+INSERT INTO "forwarded_partition_request11" VALUES('forwarded_instance','https://bogus/master/url');
 COMMIT;

slapos/tests/test_slapproxy/database_dump_version_12.sql

@@ -121,4 +121,6 @@ CREATE TABLE forwarded_partition_request12 (
   partition_reference VARCHAR(255), -- a.k.a source_instance_id
   master_url VARCHAR(255)
 );
+INSERT INTO "forwarded_partition_request12" VALUES('forwarded_instance','https://bogus/master/url');
+INSERT INTO "forwarded_partition_request12" VALUES('forwarded_instance','https://bogus/master/url');
 COMMIT;

slapos/tests/test_slapproxy/database_dump_version_13.sql

@@ -121,4 +121,6 @@ CREATE TABLE forwarded_partition_request13 (
   partition_reference VARCHAR(255), -- a.k.a source_instance_id
   master_url VARCHAR(255)
 );
+INSERT INTO "forwarded_partition_request13" VALUES('forwarded_instance','https://bogus/master/url');
+INSERT INTO "forwarded_partition_request13" VALUES('forwarded_instance','https://bogus/master/url');
 COMMIT;

slapos/tests/test_slapproxy/database_dump_version_14.sql

@@ -122,4 +122,6 @@ CREATE TABLE forwarded_partition_request14 (
   partition_reference VARCHAR(255), -- a.k.a source_instance_id
   master_url VARCHAR(255)
 );
+INSERT INTO "forwarded_partition_request14" VALUES('forwarded_instance','https://bogus/master/url');
+INSERT INTO "forwarded_partition_request14" VALUES('forwarded_instance','https://bogus/master/url');
 COMMIT;

slapos/tests/test_slapproxy/database_dump_version_15.sql

+PRAGMA foreign_keys=OFF;
+BEGIN TRANSACTION;
+CREATE TABLE local_software_release_root15 (
+  path VARCHAR(255)
+);
+INSERT INTO "local_software_release_root15" VALUES('/');
+CREATE TABLE software15 (
+  url VARCHAR(255),
+  computer_reference VARCHAR(255) DEFAULT 'computer',
+  requested_state VARCHAR(255) DEFAULT 'available',
+  CONSTRAINT uniq PRIMARY KEY (url, computer_reference)
+);
+INSERT INTO "software15" VALUES('/srv/slapgrid//srv//runner/project//slapos/software.cfg','computer','available');
+CREATE TABLE computer15 (
+  reference VARCHAR(255) DEFAULT 'computer',
+  address VARCHAR(255),
+  netmask VARCHAR(255),
+  CONSTRAINT uniq PRIMARY KEY (reference)
+);
+INSERT INTO "computer15" VALUES('computer','127.0.0.1','255.255.255.255');
+CREATE TABLE partition15 (
+  reference VARCHAR(255),
+  computer_reference VARCHAR(255) DEFAULT 'computer',
+  slap_state VARCHAR(255) DEFAULT 'free',
+  software_release VARCHAR(255),
+  xml TEXT,
+  connection_xml TEXT,
+  slave_instance_list TEXT,
+  software_type VARCHAR(255),
+  partition_reference VARCHAR(255), -- name of the instance
+  requested_by VARCHAR(255), -- only used for debugging,
+                             -- slapproxy does not support proper scope
+  requested_state VARCHAR(255) NOT NULL DEFAULT 'started',
+  timestamp REAL,
+  CONSTRAINT uniq PRIMARY KEY (reference, computer_reference)
+);
+INSERT INTO "partition15" VALUES('slappart0','computer','busy','/srv/slapgrid//srv//runner/project//slapos/software.cfg','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance>
+  <parameter id="json">{
+  "site-id": "erp5"
+  }
+}</parameter>
+</instance>
+',NULL,NULL,'production','slapos',NULL,'started',NULL);
+INSERT INTO "partition15" VALUES('slappart1','computer','busy','/srv/slapgrid//srv//runner/project//slapos/software.cfg','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance/>
+','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance>
+  <parameter id="url">mysql://127.0.0.1:45678/erp5</parameter>
+</instance>
+',NULL,'mariadb','MariaDB DataBase','slappart0','started',NULL);
+INSERT INTO "partition15" VALUES('slappart2','computer','busy','/srv/slapgrid//srv//runner/project//slapos/software.cfg','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance>
+  <parameter id="cloudooo-json"></parameter>
+</instance>
+','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance>
+  <parameter id="url">cloudooo://127.0.0.1:23000/</parameter>
+</instance>
+',NULL,'cloudooo','Cloudooo','slappart0','started',NULL);
+INSERT INTO "partition15" VALUES('slappart3','computer','busy','/srv/slapgrid//srv//runner/project//slapos/software.cfg','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance/>
+','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance>
+  <parameter id="url">memcached://127.0.0.1:11000/</parameter>
+</instance>
+',NULL,'memcached','Memcached','slappart0','started',NULL);
+INSERT INTO "partition15" VALUES('slappart4','computer','busy','/srv/slapgrid//srv//runner/project//slapos/software.cfg','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance/>
+','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance>
+  <parameter id="url">memcached://127.0.0.1:13301/</parameter>
+</instance>
+',NULL,'kumofs','KumoFS','slappart0','started',NULL);
+INSERT INTO "partition15" VALUES('slappart5','computer','busy','/srv/slapgrid//srv//runner/project//slapos/software.cfg','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance>
+  <parameter id="kumofs-url">memcached://127.0.0.1:13301/</parameter>
+  <parameter id="memcached-url">memcached://127.0.0.1:11000/</parameter>
+  <parameter id="cloudooo-url">cloudooo://127.0.0.1:23000/</parameter>
+</instance>
+','<?xml version=''1.0'' encoding=''utf-8''?>
+<instance>
+  <parameter id="url">https://[fc00::1]:10001</parameter>
+</instance>
+',NULL,'tidstorage','TidStorage','slappart0','started',NULL);
+INSERT INTO "partition15" VALUES('slappart6','computer','free',NULL,NULL,NULL,NULL,NULL,NULL,NULL,'started',NULL);
+INSERT INTO "partition15" VALUES('slappart7','computer','free',NULL,NULL,NULL,NULL,NULL,NULL,NULL,'started',NULL);
+INSERT INTO "partition15" VALUES('slappart8','computer','free',NULL,NULL,NULL,NULL,NULL,NULL,NULL,'started',NULL);
+INSERT INTO "partition15" VALUES('slappart9','computer','free',NULL,NULL,NULL,NULL,NULL,NULL,NULL,'started',NULL);
+CREATE TABLE slave15 (
+  reference VARCHAR(255), -- unique slave reference
+  computer_reference VARCHAR(255) DEFAULT 'computer',
+  connection_xml TEXT,
+  hosted_by VARCHAR(255),
+  asked_by VARCHAR(255) -- only used for debugging,
+                        -- slapproxy does not support proper scope
+);
+CREATE TABLE partition_network15 (
+  partition_reference VARCHAR(255),
+  computer_reference VARCHAR(255) DEFAULT 'computer',
+  reference VARCHAR(255),
+  address VARCHAR(255),
+  netmask VARCHAR(255)
+);
+INSERT INTO "partition_network15" VALUES('slappart0','computer','slappart0','127.0.0.1','255.255.255.255');
+INSERT INTO "partition_network15" VALUES('slappart0','computer','slappart0','fc00::1','ffff:ffff:ffff::');
+INSERT INTO "partition_network15" VALUES('slappart1','computer','slappart1','127.0.0.1','255.255.255.255');
+INSERT INTO "partition_network15" VALUES('slappart1','computer','slappart1','fc00::1','ffff:ffff:ffff::');
+INSERT INTO "partition_network15" VALUES('slappart2','computer','slappart2','127.0.0.1','255.255.255.255');
+INSERT INTO "partition_network15" VALUES('slappart2','computer','slappart2','fc00::1','ffff:ffff:ffff::');
+INSERT INTO "partition_network15" VALUES('slappart3','computer','slappart3','127.0.0.1','255.255.255.255');
+INSERT INTO "partition_network15" VALUES('slappart3','computer','slappart3','fc00::1','ffff:ffff:ffff::');
+INSERT INTO "partition_network15" VALUES('slappart4','computer','slappart4','127.0.0.1','255.255.255.255');
+INSERT INTO "partition_network15" VALUES('slappart4','computer','slappart4','fc00::1','ffff:ffff:ffff::');
+INSERT INTO "partition_network15" VALUES('slappart5','computer','slappart5','127.0.0.1','255.255.255.255');
+INSERT INTO "partition_network15" VALUES('slappart5','computer','slappart5','fc00::1','ffff:ffff:ffff::');
+INSERT INTO "partition_network15" VALUES('slappart6','computer','slappart6','127.0.0.1','255.255.255.255');
+INSERT INTO "partition_network15" VALUES('slappart6','computer','slappart6','fc00::1','ffff:ffff:ffff::');
+INSERT INTO "partition_network15" VALUES('slappart7','computer','slappart7','127.0.0.1','255.255.255.255');
+INSERT INTO "partition_network15" VALUES('slappart7','computer','slappart7','fc00::1','ffff:ffff:ffff::');
+INSERT INTO "partition_network15" VALUES('slappart8','computer','slappart8','127.0.0.1','255.255.255.255');
+INSERT INTO "partition_network15" VALUES('slappart8','computer','slappart8','fc00::1','ffff:ffff:ffff::');
+INSERT INTO "partition_network15" VALUES('slappart9','computer','slappart9','127.0.0.1','255.255.255.255');
+INSERT INTO "partition_network15" VALUES('slappart9','computer','slappart9','fc00::1','ffff:ffff:ffff::');
+CREATE TABLE forwarded_partition_request15 (
+  partition_reference VARCHAR(255), -- a.k.a source_instance_id
+  master_url VARCHAR(255)
+);
+INSERT INTO "forwarded_partition_request15" VALUES('forwarded_instance','https://bogus/master/url');
+INSERT INTO "forwarded_partition_request15" VALUES('forwarded_instance','https://bogus/master/url');
+COMMIT;

slapos/tests/test_slapproxy/generate_dump.sh

@@ -42,12 +42,6 @@ sqlite3 ${TMPD}/proxy.db < $DUMP_BEFORE
 
 slapos proxy start --cfg ${TMPD}/slapos.cfg &
 
-# If you are running tests locally and you want to refer to the slapos being tested,
-# you can use the test python executable in this way:
-# cd ../../..
-# python -m slapos.cli.entry proxy start --cfg ${TMPD}/slapos.cfg &
-# cd -
-
 SLAPOS_PROXY_PID=$!
 
 curl --silent --retry-connrefused --retry 3 http://127.0.0.1:${PORT}/getComputerInformation?computer_id=$COMPUTER_ID