7980 enhanced dsd #8915

poikilotherm · 2022-08-11T21:48:22Z

What this PR does / why we need it:
Create real defaults for the database connection, enable configuring advanced settings, add docs.

Which issue(s) this PR closes:

Closes #7980

Special notes for your reviewer:

The release docs of older releases do not need to be changed. The statements on how to upgrade etc are working just fine.
Only in a few occasions people might have hit the regression.
This PR replaces 7980 enhance data source definition #7983.
This PR requires people to update to Payara 5.2022.2 at least. It requires 8947 Update to Payara 5.2022.3 #8949 to be merged.
It might be debatable to add a few notes to this place about monitoring

Suggestions on how to test this:

Remove the properties from domain.xml. Should still work (if you had used the defaults in there)
Try setting the JDBC logging to enabled via system property: asadmin create-system-properties "dataverse.db.log-jdbc-calls=true"

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.

Is there a release notes update needed for this change?:
🔋 included.

Additional documentation:
None.

…IG. IQSS#7980 Previously, with Dataverse software 5.3, the option to configure the database connection has been moved into the codebase. Admins can set details via MicroProfile Config. With updating to Payara 5.2021.4, we can provide default values for the connection details. Before, this had been tried with adding them to META-INF/microprofile-config.properties. However, this is not possible due to the timing of resource creation in the application server vs. reading the properties file. IQSS#7980

…QSS#7980

…IQSS#7980

@pdurbin

As requested by @pdurbin, the long list was quite overwhelming. It's now damped down to 12 options in 3 subsubsections of the docs.

The avoid hacky parameter additions via the database name, this commit adds support for adding parameters to the JDBC URL. It defaults to empty (no parameters).

With the addition of the advanced (but proprietary, Payara-only) settings for database connection monitoring, the non-present default for connection validation triggered unnecessary log clutter. Adding an empty default makes these go away and is inline with the default of Payara.

coveralls · 2022-08-11T21:51:55Z

Coverage: 19.997%. Remained the same when pulling 8106ddf on poikilotherm:7980-enhanced-dsd into f63f0e8 on IQSS:develop.

mreekie · 2023-01-04T16:39:29Z

sizing: Waiting on Oliver input. He is out today.

mreekie · 2023-01-05T16:29:33Z

Phil is going to talk with Oliver to see if this can be lowered in priority in the community column

poikilotherm · 2023-01-06T13:48:06Z

I've just merged latest develop. This is a more or less trivial change, as there isn't much to test. The functionality is provided by Payara, so IMHO there is no point in testing that. One might test setting a setting, but at this point IMHO we should be confident enough that Payara handles these variable replacements well.

That said, yes, it may move down the column. I have no urgent business requiring this ATM.

pdurbin · 2023-01-06T19:16:45Z

That said, yes, it may move down the column. I have no urgent business requiring this ATM.

You know what? Based on comments by @landreev and @donsizemore in the issue regarding the usefulness of sslmode=require I'm fine with keeping this where it is priority-wise. I'll throw a 3 on it for size. I hope that's big enough.

By the way, there's a failing test but I suspect it's unrelated:

java.lang.AssertionError: 
Expected status code <200> doesn't match actual status code <403>.

	at edu.harvard.iq.dataverse.api.FilesIT.test_008_ReplaceFileAlreadyDeleted(FilesIT.java:846)

mreekie · 2023-01-09T13:05:37Z

Pdurbin reviewed and sized. Moving to sprint ready column.

pdurbin

Looks good.

The most immediate real world use case for this PR seems to be sslmode=require (see the issue this PR closes) so I mentioned this in the docs.

I did some cursory testing but didn't test the new default database name and user (both "dataverse") though I left a comment about how we could probably test this.

I love the new rst table format, by the way. Seems much easier than keeping the correct number of spaces here or there!

pdurbin · 2023-01-17T18:59:22Z

doc/sphinx-guides/source/installation/config.rst

@@ -263,6 +263,153 @@ As for the "Remote only" authentication mode, it means that:
 - ``:DefaultAuthProvider`` has been set to use the desired authentication provider
 - The "builtin" authentication provider has been disabled (:ref:`api-toggle-auth-provider`). Note that disabling the "builtin" authentication provider means that the API endpoint for converting an account from a remote auth provider will not work. Converting directly from one remote authentication provider to another (i.e. from GitHub to Google) is not supported. Conversion from remote is always to "builtin". Then the user initiates a conversion from "builtin" to remote. Note that longer term, the plan is to permit multiple login options to the same Dataverse installation account per https://github.com/IQSS/dataverse/issues/3487 (so all this talk of conversion will be moot) but for now users can only use a single login option, as explained in the :doc:`/user/account` section of the User Guide. In short, "remote only" might work for you if you only plan to use a single remote authentication provider such that no conversion between remote authentication providers will be necessary.

+
+
+Database Persistence


The docs are great but I wonder if they should be moved to "Advanced Installation" because the default database setup works fine and the list of steps before "going live" is getting longer and longer. We can defer this doc change for now. I created a dedicated issue for later:

reduce number of steps before "going live" steps in Installation Guide #9295

pdurbin · 2023-01-17T19:25:31Z

doc/sphinx-guides/source/installation/config.rst

+   * - dataverse.db.parameters
+     - Connection parameters, see `Postgres JDBC docs <https://jdbc.postgresql.org/documentation/head/connect.html>`_
+       Note: you don't need to provide the initial "?".
+     - *Empty string*


I tried putting the following in domain.xml...

<jvm-options>-Ddataverse.db.parameters=sslmode=require</jvm-options>

... and it seems to have worked. Dataverse failed to deploy and I get errors like this in server.log:

Connection could not be allocated because: The server does not support SSL.

Error in allocating a connection. Cause: Connection could not be allocated because: The server does not support SSL.

Exception while invoking class org.glassfish.ejb.startup.EjbApplication start method
javax.ejb.EJBException: javax.ejb.CreateException: Initialization failed for Singleton StartupFlywayMigrator
...
Caused by: org.flywaydb.core.internal.exception.FlywaySqlException: Unable to obtain connection from database: Error in allocating a connection. Cause: Connection could not be allocated because: The server does not support SSL.

SQL State : null
Error Code : 0
Message : Error in allocating a connection. Cause: Connection could not be allocated because: The server does not support SSL.

pdurbin · 2023-01-17T19:47:03Z

doc/sphinx-guides/source/installation/config.rst

+   * - dataverse.db.user
+     - The PostgreSQL user name to connect with.
+     - | ``dataverse``
+       | (installer sets to ``dvnapp``)
+   * - dataverse.db.password
+     - The PostgreSQL users password to connect with.
+
+       **Please note the safety advisory above.**
+     - *No default*
+   * - dataverse.db.name
+     - The PostgreSQL database name to use for the Dataverse installation.
+     - | ``dataverse``
+       | (installer sets to ``dvndb``)


I'm not sure how to see that the default database user and database are "dataverse".

To deploy this branch I used scripts/dev/dev-rebuild.sh which has the names the installer uses hard coded:

DB_NAME=dvndb
DB_USER=dvnapp

I guess I could try changing that script to this...

DB_NAME=dataverse
DB_USER=dataverse

... and then deleting these lines from domain.xml:

<system-property name="dataverse.db.user" value="dvnapp"></system-property> <system-property name="dataverse.db.name" value="dvndb"></system-property>

That is, without these system properties configured, it sounds like the default database user and database name will be "dataverse".

Yes. Sorry, but for container environments it really is common practice to use the application name for the database name. As the installer sets this explicitly for any installation out there, folks should be fine.

kcondon · 2023-01-23T19:35:41Z

@poikilotherm Can you refresh from develop? Getting flyway errors on deployment. Thanks!

pdurbin · 2023-01-23T21:02:16Z

@kcondon done! Refreshed! Ahhh.

poikilotherm added 7 commits August 11, 2022 21:17

feat(jpa): add advanced pool config options to DataSourceDefinition I…

9e16306

…QSS#7980

docs(jpa): add extensive docs about database connection configuration. …

11d6258

…IQSS#7980

feat(jpa): remove exotic database connection options. IQSS#7980

8df07cf

As requested by @pdurbin, the long list was quite overwhelming. It's now damped down to 12 options in 3 subsubsections of the docs.

docs(jpa): add release note for 5.6 about IQSS#7980

59dbdd8

feat(jpa): add JDBC connection parameter setting IQSS#7980

8a1ee7e

The avoid hacky parameter additions via the database name, this commit adds support for adding parameters to the JDBC URL. It defaults to empty (no parameters).

poikilotherm requested review from scolapasta and landreev August 11, 2022 21:48

poikilotherm mentioned this pull request Aug 11, 2022

7980 enhance data source definition #7983

Closed

landreev mentioned this pull request Aug 11, 2022

DB connection: defaults and more options #7980

Closed

4 tasks

poikilotherm mentioned this pull request Aug 31, 2022

8947 Update to Payara 5.2022.3 #8949

Merged

poikilotherm mentioned this pull request Sep 15, 2022

Loosing DB-connection on long inactivity #8959

Closed

Merge branch 'develop' into 7980-enhanced-dsd

8350af2

poikilotherm force-pushed the 7980-enhanced-dsd branch from 5144d16 to 8350af2 Compare September 20, 2022 08:01

poikilotherm added 4 commits November 4, 2022 17:18

Merge branch 'develop' into 7980-enhanced-dsd

e9ce81e

Merge branch 'develop' into 7980-enhanced-dsd

b53e6a4

Merge branch 'develop' into 7980-enhanced-dsd

f99f0cb

Merge branch 'develop' into 7980-enhanced-dsd

9cda103

pdurbin self-assigned this Jan 5, 2023

Merge branch 'develop' into 7980-enhanced-dsd

710e505

pdurbin added the User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh label Jan 6, 2023

pdurbin added the Size: 3 A percentage of a sprint. 2.1 hours. label Jan 6, 2023

pdurbin removed their assignment Jan 6, 2023

Merge branch 'develop' into 7980-enhanced-dsd IQSS#7980

8106ddf

pdurbin self-assigned this Jan 12, 2023

pdurbin mentioned this pull request Jan 17, 2023

reduce number of steps before "going live" steps in Installation Guide #9295

Closed

pdurbin added 2 commits January 17, 2023 14:51

minor doc tweaks IQSS#7980

725348d

Merge branch 'develop' into 7980-enhanced-dsd IQSS#7980

6a595f3

pdurbin approved these changes Jan 17, 2023

View reviewed changes

pdurbin removed their assignment Jan 17, 2023

kcondon self-assigned this Jan 23, 2023

Merge branch 'develop' into 7980-enhanced-dsd IQSS#7980

b5e2e1b

kcondon merged commit be5dfa3 into IQSS:develop Jan 24, 2023

pdurbin added this to the 5.13 milestone Jan 24, 2023

mikeaintworkin mentioned this pull request Jan 29, 2023

Retrieve a list of the issues & PRs in the global backlog and include Status thisaintwork/experimentsWithGithub#10

Closed

poikilotherm deleted the 7980-enhanced-dsd branch May 21, 2023 19:15

poikilotherm mentioned this pull request Jul 5, 2024

Missing dataverse.db.parameters from long list of JVM options #10669

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7980 enhanced dsd #8915

7980 enhanced dsd #8915

poikilotherm commented Aug 11, 2022 •

edited

Loading

coveralls commented Aug 11, 2022 •

edited

Loading

mreekie commented Jan 4, 2023

mreekie commented Jan 5, 2023

poikilotherm commented Jan 6, 2023 •

edited

Loading

pdurbin commented Jan 6, 2023

mreekie commented Jan 9, 2023

pdurbin left a comment

pdurbin Jan 17, 2023

pdurbin Jan 17, 2023

pdurbin Jan 17, 2023

poikilotherm Jan 17, 2023

kcondon commented Jan 23, 2023

pdurbin commented Jan 23, 2023

7980 enhanced dsd #8915

7980 enhanced dsd #8915

Conversation

poikilotherm commented Aug 11, 2022 • edited Loading

coveralls commented Aug 11, 2022 • edited Loading

mreekie commented Jan 4, 2023

mreekie commented Jan 5, 2023

poikilotherm commented Jan 6, 2023 • edited Loading

pdurbin commented Jan 6, 2023

mreekie commented Jan 9, 2023

pdurbin left a comment

Choose a reason for hiding this comment

pdurbin Jan 17, 2023

Choose a reason for hiding this comment

pdurbin Jan 17, 2023

Choose a reason for hiding this comment

pdurbin Jan 17, 2023

Choose a reason for hiding this comment

poikilotherm Jan 17, 2023

Choose a reason for hiding this comment

kcondon commented Jan 23, 2023

pdurbin commented Jan 23, 2023

poikilotherm commented Aug 11, 2022 •

edited

Loading

coveralls commented Aug 11, 2022 •

edited

Loading

poikilotherm commented Jan 6, 2023 •

edited

Loading