Flexible URI for connection with DB and new MariaDB driver

[JoaoJandre]

Description

Current properties on db.properties are limited and focused only on mysql. This PR adds a new property on db.properties, db.cloud.uri, that when set, will be used for the connection with the DBMS. This enables URI configuration and use with other DBMSs, such as mariaDB.

When using the db.cloud.uri property, the following properties will be ignored:

• db.cloud.host;
• db.cloud.port;
• db.cloud.name;
• db.cloud.autoReconnect;
• db.cloud.url.params;
• db.cloud.replicas;
• db.cloud.autoReconnect;
• db.cloud.failOverReadOnly;
• db.cloud.reconnectAtTxEnd;
• db.cloud.autoReconnectForPools;
• db.cloud.secondsBeforeRetrySource;
• db.cloud.queriesBeforeRetrySource;
• db.cloud.initialTimeout;

When using the db.usage.uri property, the following properties will be ignored:

• db.usage.host;
• db.usage.port;
• db.usage.name;
• db.usage.autoReconnect;
• db.usage.url.params;
• db.usage.replicas;
• db.usage.autoReconnect;
• db.usage.failOverReadOnly;
• db.usage.reconnectAtTxEnd;
• db.usage.autoReconnectForPools;
• db.usage.secondsBeforeRetrySource;
• db.usage.queriesBeforeRetrySource;
• db.usage.initialTimeout;

When using both, the following properties will also be ignored:

• db.ha.enable;
• db.ha.loadBalanceStrategy;

This PR also adds support for the MariaDB driver.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

How Has This Been Tested?

Using the current version in a setup with mariaDB and Galera, with a cluster size of 3 and the following configuration on the db.properties file:

# High Availability And Cluster Properties
db.ha.enabled=true
db.ha.loadBalanceStrategy=com.cloud.utils.db.StaticStrategy
# cloud stack Database
db.cloud.replicas=192.168.201.161,192.168.201.162
db.cloud.autoReconnect=false
db.cloud.failOverReadOnly=false
db.cloud.reconnectAtTxEnd=false
db.cloud.autoReconnectForPools=true
db.cloud.secondsBeforeRetrySource=1800
db.cloud.queriesBeforeRetrySource=5000
db.cloud.initialTimeout=3600

When the mysql service stops in the main node, ACS is able to switch to one of the other two. But if the host is shut down, this switch never occurs.

Using the changes proposed in this PR, by configuring the db.cloud.uri:

db.cloud.uri=jdbc:mariadb:sequential://192.168.201.160:3306,192.168.201.161:3306,192.168.201.162:3306/cloud?autoReconnect=true&prepStmtCacheSize=517&cachePrepStmts=true&sessionVariables=sql_mode='STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'&serverTimezone=UTC

I was able to configure and use the sequential fail over mode. This way, when the mysql service stops in the main node and even if the host is shut down, ACS was able to switch to the other DBs.

[weizhouapache]

good improvement.
cc @NuxRo

[yadvr]

Thanks for the PR @JoaoJandre would these work on either/both MariaDB and MySQL, do you see any operations differences between the two?

[weizhouapache]

@JoaoJandre
can the uri be generated in java code ?
this looks like there will be some duplicated settings in db.properties

[JoaoJandre]

Thanks for the PR @JoaoJandre would these work on either/both MariaDB and MySQL, do you see any operations differences between the two?

Yes, @rohityadavcloud , this should work with any DBMS that uses URI connections (MariaDB, MySQL, PostgreSQL, and others). CloudStack though would probably not work with other DBMSs with the current code base.

About the operation differences between the two (MariaDB and MySQL); each DBMS has different forms of configuring the URI, the URI I used in my tests will not work with MySQL, for example. That is why I am proposing a way for users to specify manually the URI; therefore, we enable operators to configure ACS to handle multiple DBMSs (MariaDB or MySQL) without any need to change the code.

Note that this PR only adds a new property, the old behavior is still maintained if you do not inform the property introduced here (no regression).

[JoaoJandre]

@JoaoJandre can the uri be generated in java code ? this looks like there will be some duplicated settings in db.properties

The current behavior is to generate the URI in the java code, using the db.properties. The problem with that is that it lacks flexibility. Every time someone wants to use a new feature in a URI connection, a new property will have to be created and the java code will have to be extended. It is much easier to just give the users the option to directly inform the URI that ACS should use.
Also, I don't see any duplicated settings in db.properties, what will happen is that there will be some properties that are mutually exclusive.

[weizhouapache]

@JoaoJandre can the uri be generated in java code ? this looks like there will be some duplicated settings in db.properties

The current behavior is to generate the URI in the java code, using the db.properties. The problem with that is that it lacks flexibility. Every time someone wants to use a new feature in a URI connection, a new property will have to be created and the java code will have to be extended. It is much easier to just give the users the option to directly inform the URI that ACS should use. Also, I don't see any duplicated settings in db.properties, what will happen is that there will be some properties that are mutually exclusive.

@JoaoJandre
there are following settings in db.properties

db.cloud.replicas=localhost,localhost
db.cloud.autoReconnect=true
db.cloud.url.params=prepStmtCacheSize=517&cachePrepStmts=true&sessionVariables=sql_mode='STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'&serverTimezone=UTC

[JoaoJandre]

@JoaoJandre can the uri be generated in java code ? this looks like there will be some duplicated settings in db.properties

The current behavior is to generate the URI in the java code, using the db.properties. The problem with that is that it lacks flexibility. Every time someone wants to use a new feature in a URI connection, a new property will have to be created and the java code will have to be extended. It is much easier to just give the users the option to directly inform the URI that ACS should use. Also, I don't see any duplicated settings in db.properties, what will happen is that there will be some properties that are mutually exclusive.

@JoaoJandre there are following settings in db.properties

db.cloud.replicas=localhost,localhost
db.cloud.autoReconnect=true
db.cloud.url.params=prepStmtCacheSize=517&cachePrepStmts=true&sessionVariables=sql_mode='STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'&serverTimezone=UTC

@weizhouapache , these settings will be maintained for backwards compatibility. They are used when the full URI is not set (db.cloud.uri and db.usage.uri).
I'll add a comment to db.properties to make this more clear.

[weizhouapache]

@JoaoJandre can the uri be generated in java code ? this looks like there will be some duplicated settings in db.properties

The current behavior is to generate the URI in the java code, using the db.properties. The problem with that is that it lacks flexibility. Every time someone wants to use a new feature in a URI connection, a new property will have to be created and the java code will have to be extended. It is much easier to just give the users the option to directly inform the URI that ACS should use. Also, I don't see any duplicated settings in db.properties, what will happen is that there will be some properties that are mutually exclusive.

@JoaoJandre there are following settings in db.properties

db.cloud.replicas=localhost,localhost
db.cloud.autoReconnect=true
db.cloud.url.params=prepStmtCacheSize=517&cachePrepStmts=true&sessionVariables=sql_mode='STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'&serverTimezone=UTC

@weizhouapache , these settings will be maintained for backwards compatibility. They are used when the full URI is not set (db.cloud.uri and db.usage.uri). I'll add a comment to db.properties to make this more clear.

I suggest to add the following, in additional to existing settings

• db.cloud.engine (mysql/mariadb, use mysql if null )
• db.cloud.mariadb.failover.mode (replication/sequential/loadbalance)

a predefined uri is flexible but I do not think it will happen often . If we want to support other engines, normally some jave changes are required as well.

[JoaoJandre]

@JoaoJandre can the uri be generated in java code ? this looks like there will be some duplicated settings in db.properties

The current behavior is to generate the URI in the java code, using the db.properties. The problem with that is that it lacks flexibility. Every time someone wants to use a new feature in a URI connection, a new property will have to be created and the java code will have to be extended. It is much easier to just give the users the option to directly inform the URI that ACS should use. Also, I don't see any duplicated settings in db.properties, what will happen is that there will be some properties that are mutually exclusive.

@JoaoJandre there are following settings in db.properties

db.cloud.replicas=localhost,localhost
db.cloud.autoReconnect=true
db.cloud.url.params=prepStmtCacheSize=517&cachePrepStmts=true&sessionVariables=sql_mode='STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'&serverTimezone=UTC

@weizhouapache , these settings will be maintained for backwards compatibility. They are used when the full URI is not set (db.cloud.uri and db.usage.uri). I'll add a comment to db.properties to make this more clear.

I suggest to add the following, in additional to existing settings

* db.cloud.engine (mysql/mariadb, use mysql if null )

* db.cloud.mariadb.failover.mode (replication/sequential/loadbalance)

a predefined uri is flexible but I do not think it will happen often . If we want to support other engines, normally some jave changes are required as well.

db.cloud.driver is already used to choose which DBMS will be conected (and should be set even when setting the URI!).
There are other parameters that could be informed through the URI, and mapping all of them would only create even more properties. I really do not see the case for not letting users choose the connection URI.

[weizhouapache]

@JoaoJandre can the uri be generated in java code ? this looks like there will be some duplicated settings in db.properties

The current behavior is to generate the URI in the java code, using the db.properties. The problem with that is that it lacks flexibility. Every time someone wants to use a new feature in a URI connection, a new property will have to be created and the java code will have to be extended. It is much easier to just give the users the option to directly inform the URI that ACS should use. Also, I don't see any duplicated settings in db.properties, what will happen is that there will be some properties that are mutually exclusive.

@JoaoJandre there are following settings in db.properties

db.cloud.replicas=localhost,localhost
db.cloud.autoReconnect=true
db.cloud.url.params=prepStmtCacheSize=517&cachePrepStmts=true&sessionVariables=sql_mode='STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'&serverTimezone=UTC

@weizhouapache , these settings will be maintained for backwards compatibility. They are used when the full URI is not set (db.cloud.uri and db.usage.uri). I'll add a comment to db.properties to make this more clear.

I suggest to add the following, in additional to existing settings

* db.cloud.engine (mysql/mariadb, use mysql if null )

* db.cloud.mariadb.failover.mode (replication/sequential/loadbalance)

a predefined uri is flexible but I do not think it will happen often . If we want to support other engines, normally some jave changes are required as well.

db.cloud.driver is already used to choose which DBMS will be conected (and should be set even when setting the URI!). There are other parameters that could be informed through the URI, and mapping all of them would only create even more properties. I really do not see the case for not letting users choose the connection URI.

@JoaoJandre
You can generate the uri by settings in db properties

• driver
• replicas
• failover mode (new)
• url params

what else are missing ?

[weizhouapache]

@JoaoJandre You can generate the uri by settings in db properties

• driver
• replicas
• failover mode (new)
• url params

what else are missing ?

@JoaoJandre
I am ok if you add a new db.cloud.uri, however, please ensure that the following configurations will generate the same uri

db.cloud.driver=jdbc:mariadb
db.cloud.failover.mode=sequential
db.cloud.replicas=192.168.201.160:3306,192.168.201.161:3306,192.168.201.162:3306
db.cloud.autoReconnect=true
db.cloud.url.params=prepStmtCacheSize=517&cachePrepStmts=true&sessionVariables=sql_mode='STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'&serverTimezone=UTC

to

db.cloud.driver=jdbc:mariadb
db.cloud.uri=jdbc:mariadb:sequential://192.168.201.160:3306,192.168.201.161:3306,192.168.201.162:3306/cloud?autoReconnect=true&prepStmtCacheSize=517&cachePrepStmts=true&sessionVariables=sql_mode='STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'&serverTimezone=UTC

[GutoVeronezi]

@JoaoJandre You can generate the uri by settings in db properties

• driver
• replicas
• failover mode (new)
• url params

what else are missing ?

@JoaoJandre I am ok if you add a new db.cloud.uri, however, please ensure that the following configurations will generate the same uri

db.cloud.driver=jdbc:mariadb
db.cloud.failover.mode=sequential
db.cloud.replicas=192.168.201.160:3306,192.168.201.161:3306,192.168.201.162:3306
db.cloud.autoReconnect=true
db.cloud.url.params=prepStmtCacheSize=517&cachePrepStmts=true&sessionVariables=sql_mode='STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'&serverTimezone=UTC

to

db.cloud.driver=jdbc:mariadb
db.cloud.uri=jdbc:mariadb:sequential://192.168.201.160:3306,192.168.201.161:3306,192.168.201.162:3306/cloud?autoReconnect=true&prepStmtCacheSize=517&cachePrepStmts=true&sessionVariables=sql_mode='STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'&serverTimezone=UTC

@weizhouapache seems to me that the PR's scope is to allow the full URI definition, not to extend the current hard coded properties behavior; therefore, it does not make sense to me to require an extension out of the scope.

Also, what is the point on having (and adding) several configurations that operators will have to learn how they work on CloudStack instead of having a single one that represents the full URI, that operators are used to work with?

[weizhouapache]

@GutoVeronezi cc @JoaoJandre
You only mentioned the flexible URI in the title and description.
However, there are lots of changes for mariadb support.
please remove the changes for mariadb support, if it is out of scope, as you said.

@weizhouapache seems to me that the PR's scope is to allow the full URI definition, not to extend the current hard coded properties behavior; therefore, it does not make sense to me to require an extension out of the scope.

Also, what is the point on having (and adding) several configurations that operators will have to learn how they work on CloudStack instead of having a single one that represents the full URI, that operators are used to work with?

Because URI is not user-friendly. it is suitable to premium users, but not for normal users.

[GutoVeronezi]

@GutoVeronezi cc @JoaoJandre You only mentioned the flexible URI in the title and description. However, there are lots of changes for mariadb support. please remove the changes for mariadb support, if it is out of scope, as you said.

@weizhouapache it is the contributors will to add the MariaDB support, not a requirement from another contributor; also there are 6 lines adding support for MariaDB, which is far from "lots of changes", but I do agree this should be added to the PR's description, as the PR's description should describe in details what the PR is introducing and changing, to facilitate revision and tracking of changes. @JoaoJandre, please, could you mention the MariaDB support in the PR's title and description?

@weizhouapache seems to me that the PR's scope is to allow the full URI definition, not to extend the current hard coded properties behavior; therefore, it does not make sense to me to require an extension out of the scope.
Also, what is the point on having (and adding) several configurations that operators will have to learn how they work on CloudStack instead of having a single one that represents the full URI, that operators are used to work with?

Because URI is not user-friendly. it is suitable to premium users, but not for normal users.

Normal users are able to describe a simple URI, it only needs to be well-documented. Anyways, the support for lots of properties to configure the connection is kept (the properties that we have so far); therefore, no problems will be created for users that are used to the current pattern.

[weizhouapache]

@GutoVeronezi cc @JoaoJandre You only mentioned the flexible URI in the title and description. However, there are lots of changes for mariadb support. please remove the changes for mariadb support, if it is out of scope, as you said.

@weizhouapache it is the contributors will to add the MariaDB support, not a requirement from another contributor; also there are 6 lines adding support for MariaDB, which is far from "lots of changes", but I do agree this should be added to the PR's description, as the PR's description should describe in details what the PR is introducing and changing, to facilitate revision and tracking of changes. @JoaoJandre, please, could you mention the MariaDB support in the PR's title and description?

@GutoVeronezi
If you add the mariadb support to the scope, I think it is not fully done: the failover mode is not configurable (except uri).

@weizhouapache seems to me that the PR's scope is to allow the full URI definition, not to extend the current hard coded properties behavior; therefore, it does not make sense to me to require an extension out of the scope.
Also, what is the point on having (and adding) several configurations that operators will have to learn how they work on CloudStack instead of having a single one that represents the full URI, that operators are used to work with?

Because URI is not user-friendly. it is suitable to premium users, but not for normal users.

Normal users are able to describe a simple URI, it only needs to be well-documented. Anyways, the support for lots of properties to configure the connection is kept (the properties that we have so far); therefore, no problems will be created for users that are used to the current pattern.

Currently for normal users, they just need to specify the db host ip. In order to support mariadb, they might need to specify the driver, failover mode and replicas, but it is still much more simple and friendly than the URI. To achieve it, we just need to add the new property for failover mode, which is not complicated.

[GutoVeronezi]

@GutoVeronezi cc @JoaoJandre You only mentioned the flexible URI in the title and description. However, there are lots of changes for mariadb support. please remove the changes for mariadb support, if it is out of scope, as you said.

@weizhouapache it is the contributors will to add the MariaDB support, not a requirement from another contributor; also there are 6 lines adding support for MariaDB, which is far from "lots of changes", but I do agree this should be added to the PR's description, as the PR's description should describe in details what the PR is introducing and changing, to facilitate revision and tracking of changes. @JoaoJandre, please, could you mention the MariaDB support in the PR's title and description?

@GutoVeronezi If you add the mariadb support to the scope, I think it is not fully done: the failover mode is not configurable (except uri).

@weizhouapache seems to me that the PR's scope is to allow the full URI definition, not to extend the current hard coded properties behavior; therefore, it does not make sense to me to require an extension out of the scope.
Also, what is the point on having (and adding) several configurations that operators will have to learn how they work on CloudStack instead of having a single one that represents the full URI, that operators are used to work with?

Because URI is not user-friendly. it is suitable to premium users, but not for normal users.

Normal users are able to describe a simple URI, it only needs to be well-documented. Anyways, the support for lots of properties to configure the connection is kept (the properties that we have so far); therefore, no problems will be created for users that are used to the current pattern.

Currently for normal users, they just need to specify the db host ip. In order to support mariadb, they might need to specify the driver, failover mode and replicas, but it is still much more simple and friendly than the URI. To achieve it, we just need to add the new property for failover mode, which is not complicated.

@weizhouapache, a contributor is not bound to address all the use cases. If the author is adding support to MariaDB with the full URI, we have to guarantee that the use case is being addressed and the changes are not breaking compatibility; other than that, is the author's choice to add support for other use cases or not. Therefore, it is on @JoaoJandre to choose adding the support for hardcoded properties or not.

[JoaoJandre]

@GutoVeronezi sure, I've added it to the description and title.

@weizhouapache about adding more properties, as previously written by @GutoVeronezi, I think that it is out of the scope of this PR, which is meant to add a single new property and add compatibility for MariaDB driver. If the community believes that new properties are needed, then I am sure that someone else will be happy to create them in another PR.

I think that the functionality added in this PR is already complete, and there is no need for further changes.

[weizhouapache]

@GutoVeronezi sure, I've added it to the description and title.

@weizhouapache about adding more properties, as previously written by @GutoVeronezi, I think that it is out of the scope of this PR, which is meant to add a single new property and add compatibility for MariaDB driver. If the community believes that new properties are needed, then I am sure that someone else will be happy to create them in another PR.

I think that the functionality added in this PR is already complete, and there is no need for further changes.

OK

[codecov]

Codecov Report

Merging #7895 (8fa0f4e) into main (819dd7b) will increase coverage by 15.86%.
Report is 175 commits behind head on main.
The diff coverage is 62.12%.

@@              Coverage Diff              @@
##               main    #7895       +/-   ##
=============================================
+ Coverage     14.40%   30.26%   +15.86%     
- Complexity    10109    32510    +22401     
=============================================
  Files          2748     5182     +2434     
  Lines        259390   391259   +131869     
  Branches      40381    63968    +23587     
=============================================
+ Hits          37353   118409    +81056     
- Misses       217207   256740    +39533     
- Partials       4830    16110    +11280     

Flag	Coverage Δ
simulator-marvin-tests	26.73% <62.12%> (?)
uitests	4.50% <ø> (?)
unit-tests	14.80% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...src/main/java/com/cloud/utils/db/DriverLoader.java	73.68% <100.00%> (ø)
...ain/java/com/cloud/utils/db/TransactionLegacy.java	48.88% <61.53%> (ø)

... and 3683 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[DaanHoogland]

@GutoVeronezi @weizhouapache @JoaoJandre , is this ready?

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[GutoVeronezi]

CLGTM

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[JoaoJandre]

@GutoVeronezi @weizhouapache @JoaoJandre , is this ready?

Yes, it's ready :)

[DaanHoogland]

@weizhouapache are your concerns all met?

[blueorangutan]

[SF] Trillian test result (tid-8198)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43257 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7895-t8198-kvm-centos7.zip
Smoke tests completed. 115 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[DaanHoogland]

@weizhouapache can you concur?

[weizhouapache]

@weizhouapache are your concerns all met?

@DaanHoogland
no, I am 0 with this PR.

[shwstppr]

Code looks okay. Haven't tested but smoke tests are okay

[DaanHoogland]

@JoaoJandre @GutoVeronezi will you be making follow ups to this PR?

[DaanHoogland]

testing some more to be sure...

[DaanHoogland]

@blueorangutan test alma9 vmware-70u3

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (alma9 mgmt + vmware-70u3) has been kicked to run smoke tests

[GutoVeronezi]

@JoaoJandre @GutoVeronezi will you be making follow ups to this PR?

@DaanHoogland, PR is good to me as it is; other improvements can be addressed further.

[blueorangutan]

[SF] Trillian test result (tid-8297)
Environment: vmware-70u3 (x2), Advanced Networking with Mgmt server a9
Total time taken: 54170 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7895-t8297-vmware-70u3.zip
Smoke tests completed. 113 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_balanced_drs_algorithm	Failure	133.27	test_cluster_drs.py
test_08_upgrade_kubernetes_ha_cluster	Failure	730.40	test_kubernetes_clusters.py