Support high-availability setups for the interactive tools proxy

[kysrpex]

This PR closes #16138. At the moment, information exchanges between the interactive tools proxy and Galaxy are done via a SQLite database. This makes it impossible to have the high-availability (redundant) setup for the proxy that we would like to have at useGalaxy.eu.

To solve the problem, I opted for a relatively simple solution. Since Galaxy already uses SQLAlchemy, I refactored the existing class InteractiveToolSqlite from lib/galaxy/managers/interactivetool.py to use SQLAlchemy instead of Python's built-in sqlite3 library. The changes are backwards compatible. Setting interactivetools_map to a path makes Galaxy use a SQLite database, just like it did before. Setting it to a SQLAlchemy database url allows to use any database supported by SQLAlchemy (e.g. PostgreSQL). Mappings are written to the table "gxitproxy", just as they did before.

Of course, this also needs support on the interactive tools proxy's side, which will be contributed separately.

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

This is a refactor of the "propagator" InteractiveToolSqlite in lib/galaxy/managers/interactivetool.py. Tests are already available and can be run using
./run_tests.sh -integration test/integration/test_interactivetools_api.py::TestInteractiveToolsIntegration. I used standard SQL so that the code is compatible with the widest possible variety of databases.

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[kysrpex]

I feel like I may be missing something regarding the FileLock. What role was it actually playing? Isn't SQLite already taking care of locks?

The pager module effectively controls access for separate threads, or separate processes, or both. Throughout this document whenever the word "process" is written you may substitute the word "thread" without changing the truth of the statement.

[jmchilton]

Galaxy has a long history of concurrency issues with respect to SQLite. I cannot promise this FileLock was needed but experience taught me it was better to be safe than sorry here.

[kysrpex]

Some information that may help us understand whether the lock is needed in this case.

• SQLALchemy documentation

However, no matter what locking modes are used, SQLite will still always lock the database file once a transaction is started and DML (e.g. INSERT, UPDATE, DELETE) has at least been emitted, and this will block other transactions at least at the point that they also attempt to emit DML. By default, the length of time on this block is very short before it times out with an error.

This behavior becomes more critical when used in conjunction with the SQLAlchemy ORM. SQLAlchemy’s Session object by default runs within a transaction, and with its autoflush model, may emit DML preceding any SELECT statement. This may lead to a SQLite database that locks more quickly than is expected. The locking mode of SQLite and the pysqlite driver can be manipulated to some degree, however it should be noted that achieving a high degree of write-concurrency with SQLite is a losing battle.

• SQLite documentation

2. Situations Where A Client/Server RDBMS May Work Better

• [...]

• High-volume Websites

  SQLite will normally work fine as the database backend to a website. But if the website is write-intensive or is so busy that it requires multiple servers, then consider using an enterprise-class client/server database engine instead of SQLite.

• [...]

• High Concurrency
  SQLite supports an unlimited number of simultaneous readers, but it will only allow one writer at any instant in time. For many situations, this is not a problem. Writers queue up. Each application does its database work quickly and moves on, and no lock lasts for more than a few dozen milliseconds. But there are some applications that require more concurrency, and those applications may need to seek a different solution.

From both excerpts I understand that it is likely that the long history of concurrency issues that Galaxy has with SQLite probably has to do with using the SQLAlchemy ORM under high load (maybe you can either support/refute this?).

As far as I know, the interactive tools mapping file is written only by InteractiveToolSqlite (or InteractiveToolPropagatorSQLAlchemy in this refactor) and read only by Galaxy (the same class) and the proxy. I understand that issues would appear if interactive tools are in extremely high demand (Galaxy constantly writes keys and tokens to the file). However, in that case, wouldn't it just make better sense, as the SQLite docs claim, that the Galaxy admin uses PostgreSQL rather than SQLite? Another solution would be increasing the SQLite lock timeout.

Maybe if @jdavcs agrees to go forward with a new config property for mappings based on arbitrary SQLAlchemy database URLs, it may even make sense to discourage the use of the original property interactivetools_map in the Galaxy docs to favour the use of Postgres? After all, SQLite use is already discouraged.

In that case though, I think we should also have a look together at the proxy-side implementation, because it also has it's drawbacks regarding database setup (maybe it should just be done automatically by Galaxy).

[jdavcs]

From both excerpts I understand that it is likely that the long history of concurrency issues that Galaxy has with SQLite probably has to do with using the SQLAlchemy ORM under high load (maybe you can either support/refute this?).

I think the core of the issue here is just SQLite: "SQLite will still always lock the database file once a transaction is started" - it's just suboptimal for write-intensive scenarios. Using the ORM makes it worse: the autoflush model mentioned in the docs ensures that the state of objects loaded from the db is kept in sync with the data in the db by issuing DML statements before retrieving new data (when needed) - so the db file gets locked much more frequently if we rely on the Session. However, we don't use the ORM here.

If we go with this proposed approached, we can still keep SQLite as an option (it may be still a good enough option for smaller instances or dev purposes, etc.; after all we support SQLite as an option for the main database too). We may consider adding a note to Galaxy's documentation and updating the IT tutorial accordingly.

[kysrpex]

I added a note (dddf5ba) to the documentation and opened galaxyproject/training-material#5178 and galaxyproject/training-material#5179

[bgruening]

Great stuff @kysrpex!

@sveinugu that might also be interesting for you.

[sveinugu]

Sounds good!

Just make sure that the info field keeps supporting json, as I opted for storing extra attributes to the proxy that are used for path-based interactive tools as json in the "info" field instead of adding new fields to the schema (the info field is not used for SQL queries, so I don't think there is a need to explicitly type this as JSON, plain text should be fine).

Currently the path-based ITs were reverted in the EU instance due to an error. I made a PR to revert this for the 24.1 release, now merged: usegalaxy-eu#242

So just make sure that the path-based ITs still run (first commit of that PR)!

[kysrpex]

@sveinugu Regarding the JSON, it's stored as plain text. Seems to work fine (see the psql output below from a Galaxy instance).

galaxy=> select * from gxitproxy;
      key      |         key_type          |     token     |      host      | port  |                                  info                                  
---------------+---------------------------+---------------+----------------+-------+------------------------------------------------------------------------
 326unmiv5sanl | interactivetoolentrypoint | 2jlcp7mzyjsbm | 192.168.122.64 | 32780 | {"requires_path_in_url": false, "requires_path_in_header_named": null}
(1 row)

I'll test the path-based ITs and let you know if they're working.

[kysrpex]

@sveinugu path-based ITs cause a redirection loop (I am using gx-it-proxy from galaxyproject/gx-it-proxy#21).

galaxy=> select * from gxitproxy;
      key      |         key_type          |     token     |      host      | port  |                                 info                                  
---------------+---------------------------+---------------+----------------+-------+-----------------------------------------------------------------------
 1540u2qksgef3 | interactivetoolentrypoint | 2ypyg6q4fsp20 | 192.168.122.64 | 32782 | {"requires_path_in_url": true, "requires_path_in_header_named": null}
(1 row)

Should they be working? Have you encountered this before? This may be a misconfiguration of my Galaxy instance, as I did not change what the proxy does with the sessions, just where it stores them.

[sveinugu]

Sorry for the late reply, have started my vacation.

Have you configured the nginx proxy correctly for path-based ITs (second config here in addition to the first config, which is the wildcard one)?

https://docs.galaxyproject.org/en/master/admin/special_topics/interactivetools.html#nginx-proxy-server-configuration-in-production

(Note that second config should be part of the config for the main Galaxy domain and not appended to the first one, which is only activated for wildcard subdomains.

[sveinugu]

Note that I don't think usegalaxy-eu has been configured to support path-based ITs yet, which is more likely the cause than your change.

[kysrpex]

Note that I don't think usegalaxy-eu has been configured to support path-based ITs yet, which is more likely the cause than your change.

Although I am not testing this in production, that is correct. I had to launch the proxy with the argument --proxyPathPrefix /interactivetool/ep to make path-based interactive tools work. After having a look at the code that handles this argument, it is clear that this is required in addition to the nginx configuration. This argument would also need to be used in production to enable path-based interactive tools on useGalaxy.eu.

In any case, this confirms path-based interactive tools are not affected by this PR, nor galaxyproject/gx-it-proxy#21. Below you have a screenshot of both types of interactive tools (path-based and domain-based) running side-by-side.

[davelopez]

Thanks @kysrpex!
LGTM!

[jdavcs]

Please don't merge yet - I may have some comments. Reviewing now..

[sveinugu]

I had to launch the proxy with the argument --proxyPathPrefix /interactivetool/ep to make path-based interactive tools work.

That is true. Gravity does this by default based on the galaxy.yml configs when it is used to launch the proxy, but you might be starting the proxy differently? If this is not documented anywhere, perhaps you can add this in a natural place?

[kysrpex]

I had to launch the proxy with the argument --proxyPathPrefix /interactivetool/ep to make path-based interactive tools work.

That is true. Gravity does this by default based on the galaxy.yml configs when it is used to launch the proxy, but you might be starting the proxy differently? If this is not documented anywhere, perhaps you can add this in a natural place?

I missed it because I am using our Ansible role usegalaxy-eu/ansible-gie-proxy, which does not provide the option to configure --proxyPathPrefix. I think it makes sense to update the role and the example playbook on the README to include this detail, that should be enough.

[jdavcs]

A few critical notes:

1. I assume the gxitproxy table is meant to be part of the main galaxy's database (as per Store interactive tool sessions in PostgreSQL database "gxitproxy" usegalaxy-eu/infrastructure-playbook#1251)? If so, that may break things. Galaxy does not assume the existence of tables in its database that are not defined in its model (except for this one case). Thus, db schema migration logic may break, now or in the future when we add some feature that scans the database).
2. While adding this table to the model would appear to solve the issue above, it would introduce other problems, such as (a) this would be using the same dbapi connection that's used by galaxy's core functionality, which, I assume, is not what we want here; and (b) that would add a model definition that, unlike all other model definitions, is not part of galaxy's core functionality and, as such, does not belong in the same model, at least not conceptually.
3. The interactivetools_map config property is currently a path; there is nontrivial config logic that pre-processes path properties; if this were not a path in some cases this would either (a) break current config processing; or (b) would require special case handling to be added to the path processing logic. A better option, in my opinion, would be to add a new config property and then handle any necessary setup in the _process_config method, which is a "catch-all" for whatever special case handling has to take place in the config after all the automated processing steps have been executed.

Overall, yeah, sqlite is far from optimal as a backend here. But, in my opinion, we shouldn't be reusing the main db either. FWIW, beaker was mentioned as a possible approach during today's joint meeting (ref: #15216, beaker).

[jdavcs]

Also, whenever config_schema.yml is modified, make config-rebuild should be run that updates the rst and yml sample files (if this has been run here, please ignore)

[kysrpex]

A few critical notes:

1. I assume the `gxitproxy` table is meant to be part of the main galaxy's database (as per [Store interactive tool sessions in PostgreSQL table "gxitproxy" usegalaxy-eu/infrastructure-playbook#1251](https://github.com/usegalaxy-eu/infrastructure-playbook/pull/1251))? If so, that may break things. Galaxy does not assume the existence of tables in its database that are not defined in its model (except for [this one case](https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/model/migrations/base.py#L423)). Thus, db schema migration logic may break, now or in the future when we add some feature that scans the database).

2. While adding this table to the model would appear to solve the issue above, it would introduce other problems, such as (a) this would be using the same dbapi connection  that's used by galaxy's core functionality, which, I assume, is not what we want here; and (b) that would add a model definition that, unlike all other model definitions, is not part of galaxy's core functionality and, as such, does not belong in the same model, at least not conceptually.

3. The `interactivetools_map` config property is currently a path; there is nontrivial config logic that pre-processes path properties; if this were not a path in some cases this would either (a) break current config processing; or (b) would require special case handling to be added to the path processing logic. A better option, in my opinion, would be to add a _new_ config property and then handle any necessary setup in the [`_process_config` method](https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/config/__init__.py#L786), which is a "catch-all" for whatever special case handling has to take place in the config _after_ all the automated processing steps have been executed.

Overall, yeah, sqlite is far from optimal as a backend here. But, in my opinion, we shouldn't be reusing the main db either. FWIW, beaker was mentioned as a possible approach during today's joint meeting (ref: #15216, beaker).

@jdavcs Thank you for the review, this is the sort of insight one does not easily get on their own.

On 1 and 2, although it is possible to use any SQLAlchemy database URL, and thus any PostgreSQL db, code is written under the assumption that it should work even if interactivetools_map is set to the Galaxy database. From what you said, now I understand why this may not be a good idea. On 3, when writing this line I was thinking: this looks wrong. I see now that's indeed the case.

Your review leads me though to ask you the following question: is supporting a PostgreSQL backend via SQLAlchemy a no-go (i.e. you would absolutely have a beaker-based solution for example), or would you be willing to go forward with this (of course addressing the concerns you have mentioned)?

In the latter case I propose the following:

• Forbid using the Galaxy database as mapping for interactive tools (e.g. via _process_config). If not possible, explicitly state in the documentation that the Galaxy database should NOT be used to hold the interactive tools mapping. This means the SQLAlchemy database url will have to point to another database.
• Add a new config property for mappings based on arbitrary SQLAlchemy database URLs.

As I stated in my first message

high-availability (redundant) setup for the proxy that we would like to have at useGalaxy.eu

the reason for proposing this change is that the interactive tools proxy has been blocking us from having two fully redundant Galaxy servers for a long time. This is negatively affecting us for a variety of reasons, the most important being delayed os updates. For us, it would not be a problem to have a dedicated PostgreSQL database just for the proxy.

[jdavcs]

@kysrpex thank you for addressing my comments! I think we can go ahead with your proposed approach (i.e., SQLAlchemy backend to support a Postgres database with with protection against reusing Galaxy's main database, programmatically in code + a note in the docs) + a separate config property for the db url. We may opt for a more generic approach down the road, but given EU's urgent need, I think we can be pragmatic and treat this is a good enough solution.

With regard to implementation, my suggestion would be to replace all the raw SQL with SQLAlchemy SQL expressions (see SQLAlchemy Core, not ORM). That way we'll get all the benefits of using SQLAlchemy (we rely on SA to translate our statements into SQL appropriate for the database we're using; we get Python objects instead of raw SQL strings, which is less error-prone, is testable, etc.)

[jdavcs]

One more general (and minor) comment. Would you be open to using identifiers that are not sqlalchemy-specific? That is, for the config property, the InteractiveToolPropagatorSQLAlchemy class and the parse_sqlalchemy_url import alias in the config module. Here's why: although we very heavily rely on SQLAlchemy, it's just a tool, and the functionality we use here could be, hypothetically, implemented with another similar tool. So, semantically, it's really database-related, not sqlalchemy-related. The database URL format is, for the most part, an open standard (ref). Finally, from a practical standpoint, it's much easier to manage the code base when grepping for sqlalchemy returns only import statements.

EDIT: that said, I think it's perfectly OK to mention SQLAlchemy in the config property description, because SQLAlchemy determines what databases are currently supported.

[kysrpex]

@kysrpex thanks, this is looking good! I'll add a few comments inline; however, here's my main question. I've tested this manually and it didn't work correctly for me when enabling the sqlalchemy option: the database table is populated and the job is launched, but when the tool is supposed to become available, I see Proxy target missing on a blank screen. It may be due to some obvious local config options I haven't set correctly. However, given that it worked with sqlite settings, I think we should expect it to work with the other setting as well; if any additional config is required, that should be made clear in the documentation. Does it run for you?

@jdavcs Meet the elephant in the room, galaxyproject/gx-it-proxy#21! You need to use that version of the proxy.

One more general (and minor) comment. Would you be open to using identifiers that are not sqlalchemy-specific? That is, for the config property, the InteractiveToolPropagatorSQLAlchemy class and the parse_sqlalchemy_url import alias in the config module. Here's why: although we very heavily rely on SQLAlchemy, it's just a tool, and the functionality we use here could be, hypothetically, implemented with another similar tool. So, semantically, it's really database-related, not sqlalchemy-related. The database URL format is, for the most part, an open standard (ref). Finally, from a practical standpoint, it's much easier to manage the code base when grepping for sqlalchemy returns only import statements.

EDIT: that said, I think it's perfectly OK to mention SQLAlchemy in the config property description, because SQLAlchemy determines what databases are currently supported.

Yes, definitely! I think the same. interactivetools_map_sqlalchemy is a terrible choice. Considering that it may even make sense to deprecate interactivetools_map in favor of this one at some point, it would be better to choose a name not tied to SQLAlchemy. Something as simple as interactivetoolsproxymap might work, but please suggest whatever you have in mind, I think brainstorming it is worth it. Another concern is how long-lived this setting would be. That should have an influence in the name and the decision is on you and the rest of the dev team.

[jdavcs]

...Considering that it may even make sense to deprecate interactivetools_map in favor of this one at some point, it would be better to choose a name not tied to SQLAlchemy. Something as simple as interactivetoolsproxymap might work...

Yes, I agree: ideally, we'd have one config property since both of these specify the same "type of thing". That one property would, likely, require a proper db url, just like the database_connection property (so we wouldn't do any path resolution with path_resolves_to like we do now). The client code would then make any decisions on how to handle the database depending on whether it is sqlite or not. But if we go that route, we'd need to ensure there's a simple upgrade path for existing instances. I think, the following sequence would be a safe approach: (1) add the new db-based property (this PR); (2) make the new property also handle sqlite db urls + deprecate old property; (3) remove old property + add code to graciously handle transition (i.e., something like this or this).

[kysrpex]

...Considering that it may even make sense to deprecate interactivetools_map in favor of this one at some point, it would be better to choose a name not tied to SQLAlchemy. Something as simple as interactivetoolsproxymap might work...

Yes, I agree: ideally, we'd have one config property since both of these specify the same "type of thing". That one property would, likely, require a proper db url, just like the database_connection property (so we wouldn't do any path resolution with path_resolves_to like we do now). The client code would then make any decisions on how to handle the database depending on whether it is sqlite or not. But if we go that route, we'd need to ensure there's a simple upgrade path for existing instances. I think, the following sequence would be a safe approach: (1) add the new db-based property (this PR); (2) make the new property also handle sqlite db urls + deprecate old property; (3) remove old property + add code to graciously handle transition (i.e., something like this or this).

@jdavcs I removed dead code that was pending to be removed (0b6ad17) and renamed interactivetools_map_sqlalchemy to interactivetoolsproxy_map (9ee6636). The property already accepts sqlite db urls (it accepts any SQLAlchemy url).

If I did not miss anything else, then finally all points have been covered and the PR is ready to be merged.

[kysrpex]

@jdavcs Sorry, we also discussed privately that the config package should not depend on sqlalchemy. I changed (5c0b5d6, 7510372) the programmatic check that interactivetoolsproxy_map does not match database_connection nor install_database_connection so that it uses urlparse rather than sqlalchemy.engine.make_url. That makes the comparison less precise, but removes the dependency.

The documentation states that the the two/three databases should not be mixed, so as discussed, this should be enough. It prevents accidental mistakes such as copying and pasting the same value, although not more advanced mistakes such as accessing the same database using two different drivers. A more sophisticated check involving significant extra coding effort would be overengineering imo, specially considering that the old configuration setting interactivetools_map will later first be deprecated, then removed together with the programmatic check.

[kysrpex]

With the force push I just fixed a missing comma (Python linting), nothing of importance.

[jdavcs]

@kysrpex Looks great! Thank you for addressing all the comments!