Database VIEW management via separated files by GutoVeronezi · Pull Request #7417 · apache/cloudstack · GitHub

Description

This PR is a follow up of the discussion we had in the thread
https://lists.apache.org/thread/qvb3kx78m7lvc21d90ld00v8o8wd27ro

For changes in database VIEWs, currently, we have to look at the changes in the previous schema files and replicate the whole CREATE VIEW command in the current schema file, modifying what we need (adding/removing columns, and so on). This process makes the changes in a VIEW to be distributed through several files, increasing the number of lines modified for simple changes (e.g.: for adding a single field to the VIEW result we need to replicate the whole command); thus, making it difficult to maintain and track.

With that in mind, the proposal is to, instead of adding the VIEW changes to the schema files, we manage them in a separated file (or several files). That would allow us to easily track VIEW modifications, as we would just change the VIEW declaration in the same file, instead of re-declaring the whole VIEW in a different file; and we would have a better history of the changes. Also, we would not need to migrate all VIEWs right away; we could migrate as we change them (I already created some files according to what was changed in 4.18).

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?

I created an environment from scratch and observed the upgrade logs:
management-server.log.tar.gz

At this time this PR is a POC; some points need to be better discussed or still have to be addressed:

Topics to be better discussed:
a. (i) create a single file for all VIEWs or (ii) a file for each VIEW;
b. when removing a VIEW, (i) remove the file and add the DROP VIEW statement in the schema file or (ii) keep the VIEW file and add the DROP VIEW statement to it;
c. commands in the schema files that rely on VIEWs.

Points yet to be addressed:

via GitHub Actions, enforce the VIEW management through the new workflow;
update our Coding Convention with the new VIEW management workflow;

I also added a validation to execute this workflow only after upgrading to 4.19.0.0, for back-compatibility purposes.

If I am missing something, please, let me know.

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (288f066) 27.88% compared to head (b8cd051) 29.17%.
Report is 53 commits behind head on main.

Files	Patch %	Lines
...java/com/cloud/upgrade/DatabaseUpgradeChecker.java	5.55%	17 Missing ⚠️
utils/src/main/java/com/cloud/utils/FileUtil.java	0.00%	13 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7417      +/-   ##
============================================
+ Coverage     27.88%   29.17%   +1.29%     
- Complexity    29145    31072    +1927     
============================================
  Files          5165     5193      +28     
  Lines        364127   366263    +2136     
  Branches      53304    53548     +244     
============================================
+ Hits         101523   106874    +5351     
+ Misses       248630   244749    -3881     
- Partials      13974    14640     +666

Flag	Coverage Δ
simulator-marvin-tests	25.14% <3.22%> (+1.69%)	⬆️
uitests	4.49% <ø> (-0.04%)	⬇️
unit-tests	14.81% <5.55%> (+0.07%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
1 Security Hotspot
0 Code Smells

0.0% Coverage
0.0% Duplication

great effort, looks good to me.
We might want to apply this on 4.18.1, if for nothing else, just for the extra test it gives

so we upgrade the views on each step after the scripted upgrades but before the java based upgrades.
I wonder if we should just do it ones after everything, but maybe that will hamper some upgrades that depend on views for data collection.

any progress on this @GutoVeronezi ?

@GutoVeronezi , I just tried to review the link in the description and it seems to be gone, can you check?
An error code 404 occured while trying to fetch https://lists.apache.org/api/thread.lua?id=qvb3kx78m7lvc21d90ld00v8o8wd27r: Content not found

@GutoVeronezi , I just tried to review the link in the description and it seems to be gone, can you check? An error code 404 occured while trying to fetch https://lists.apache.org/api/thread.lua?id=qvb3kx78m7lvc21d90ld00v8o8wd27r: Content not found

@DaanHoogland I may have bumped the backspace button accidentally while writing the description, the link was missing the final character. I adjusted the description, the correct link is https://lists.apache.org/thread/qvb3kx78m7lvc21d90ld00v8o8wd27ro.

any progress on this @GutoVeronezi ?

The topics described in #7417 (comment) will guide us in defining how to use it and guaranteeing that everyone complies with the new pattern. Resuming the discussion of them:

Regarding point a, I believe creating a file for each VIEW is the best option; it will require more effort on backporting, however, it will bring us more organization and better backtracking.

About point b, I do not have a formed opinion about it; we could go either way; we would just have to adjust the GH Actions (yet to be done) to comply with it.

In respect of point c (and also @DaanHoogland's comment -- #7417 (comment)), the point in creating VIEW is to help us better handle the data via Java. In the schema files, we have total flexibility to use SQL as we want; therefore, if necessary to use a VIEW logic at some point, we could reproduce it easily in the query. With that, we might want to execute the VIEW scripts only after applying all the patches, as @DaanHoogland wondered.

What are your thoughts on this?

as for point b, i suggest keep a file with only a drop if exists statement. It will make explicit to readers of the code what the history is and is not a big overhead. A drop statement in one of the upgrade files is easier to overlook.

a and c I agree with you.

@GutoVeronezi , I just tried to review the link in the description and it seems to be gone, can you check? An error code 404 occured while trying to fetch https://lists.apache.org/api/thread.lua?id=qvb3kx78m7lvc21d90ld00v8o8wd27r: Content not found

@DaanHoogland I may have bumped the backspace button accidentally while writing the description, the link was missing the final character. I adjusted the description, the correct link is https://lists.apache.org/thread/qvb3kx78m7lvc21d90ld00v8o8wd27ro.

any progress on this @GutoVeronezi ?

The topics described in #7417 (comment) will guide us in defining how to use it and guaranteeing that everyone complies with the new pattern. Resuming the discussion of them:

Regarding point a, I believe creating a file for each VIEW is the best option; it will require more effort on backporting, however, it will bring us more organization and better backtracking.

About point b, I do not have a formed opinion about it; we could go either way; we would just have to adjust the GH Actions (yet to be done) to comply with it.

In respect of point c (and also @DaanHoogland's comment -- #7417 (comment)), the point in creating VIEW is to help us better handle the data via Java. In the schema files, we have total flexibility to use SQL as we want; therefore, if necessary to use a VIEW logic at some point, we could reproduce it easily in the query. With that, we might want to execute the VIEW scripts only after applying all the patches, as @DaanHoogland wondered.

What are your thoughts on this?

a looks good to me
b agree with Daan on it
c looks complicated, it needs further investigation

c looks complicated, it needs further investigation

@weizhouapache, what are you concerns?

c looks complicated, it needs further investigation

@weizhouapache, what are you concerns?

@GutoVeronezi
As I remember, there were some SQL sentences which use views in the upgrade SQL or upgrade java.
I might be wrong. it would be good to check.

c looks complicated, it needs further investigation

@weizhouapache, what are you concerns?

@GutoVeronezi As I remember, there were some SQL sentences which use views in the upgrade SQL or upgrade java. I might be wrong. it would be good to check.

@weizhouapache, I checked both the SQL file and java method performDataMigration since 4.17.0 and, besides dropping and creating views, the only use I found was in schema-41720to41800.sql, in an UPDATE that uses the following as a condition:

AND (SELECT COUNT(id) AS count FROM `network_offering_view` WHERE supports_vm_autoscaling = 1) = 0

However, if you look at the view definition (right above the UPDATE query), you will notice that the view was used unnecessarily; the same result would be achieved with the following:

AND (SELECT COUNT(id) AS count FROM `network_offerings` WHERE supports_vm_autoscaling = 1) = 0

We have rare cases where we use a view in the SQL files, and they can be easily replaced.

Though I have not found any use of views in the java method performDataMigration, one might use them. Therefore, rethinking it, seems that the approach of upgrading the views on each step after the scripted upgrades and before the java based upgrades would be the best choice. (edit: this will not work for multiple upgrades)

I have not found any use of views in the Java method performDataMigration in the last releases. If one might need to use them in the future, it will be a specific case, and that can be handled in other ways as well. We would just need to clarify those aspects in the documentation.

…-management-via-separated-files

@GutoVeronezi do you think we can make this work before 4.19?

@GutoVeronezi do you think we can make this work before 4.19?

@DaanHoogland yes, I'm working on the fix for the tests RN.

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

This PR have many database changes, which require intensive upgrade tests (from various older versions) IMHO.

ok, I'll do a simple upgrade test before merging. We'll have to test the RC intensively for upgrade anyway.

I test upgraded from 4.16 and 4.17, @weizhouapache . Can we be satisfied with that?

@DaanHoogland cool. The changes have only impact on the views which are changed in the upgrade from 4.18.1.0 to 4.19.0.0 the impact is small I think. looks ok to me.

@shwstppr , can you concur?

@DaanHoogland I'm +0 on this

@DaanHoogland I'm +0 on this

that sounds like you don't find it useful, @shwstppr ?

any others have opinions on this? cc @rohityadavcloud @nvazquez @weizhouapache @wido @soreana at anybody on the thread?

@DaanHoogland I'm +0 on this

that sounds like you don't find it useful, @shwstppr ?

any others have opinions on this? cc @rohityadavcloud @nvazquez @weizhouapache @wido @soreana at anybody on the thread?

I am +1 on this based on the idea. I haven't been able to verify all the code, but splitting these kinds of things into seperate files is always a good thing.

@DaanHoogland I'm +0 on this

that sounds like you don't find it useful, @shwstppr ?

any others have opinions on this? cc @rohityadavcloud @nvazquez @weizhouapache @wido @soreana at anybody on the thread?

we need another review on this people.

@DaanHoogland I'm +0 on this

that sounds like you don't find it useful, @shwstppr ?
any others have opinions on this? cc @rohityadavcloud @nvazquez @weizhouapache @wido @soreana at anybody on the thread?

we need another review on this people.

@DaanHoogland I'm all for it based on the concept. I haven't had a chance to check all the codes. But from what I've seen so far, it looks promising.
I can check the upgrade from 4.17 to 4.18 and 4.19. Currently I'm busy with something else, I will get back to you on Thursday.

@DaanHoogland I'm all for it based on the concept. I haven't had a chance to check all the codes. But from what I've seen so far, it looks promising. I can check the upgrade from 4.17 to 4.18 and 4.19. Currently I'm busy with something else, I will get back to you on Thursday.

I have tested some upgrades @soreana , Of course you're welcome to do more tests, but I would like extra eyes on the code especially.

@DaanHoogland Overall the code looks good just some minor adjustments.

@GutoVeronezi Thanks for the PR, it was well done. Did you use an automated script to extract the views or did you do that manually? Do I need to cross-validate to check if all the views have been transferred or not?

@GutoVeronezi Thanks for the PR, it was well done. Did you use an automated script to extract the views or did you do that manually? Do I need to cross-validate to check if all the views have been transferred or not?

@soreana
The initial purpose of the PR is to introduce the concept of VIEW management via separate files. As I stated in the PR description, we do not need to migrate all VIEWs right away; we could migrate as we change them. For the POC, I [manually] created files for the views that were changed since 4.18; therefore, not all the VIEWs were transferred. The idea now is to guide the contributors to use the new workflow and block VIEW changes on the upgrade scripts; this way, doing a gradual migration.

If someone else could put effort into checking all the VIEWs we have and create the remaining files, it would be nice as well.

@GutoVeronezi Thanks for clarification, now it is crystal clear.

@GutoVeronezi @DaanHoogland @harikrishna-patnala this PR removed some OAuth related changes. Can you please check if that was needed?

Perhaps a mistake solving a conflict. I will add it again. Thanks @shwstppr.

Created a draft PR https://github.com/apache/cloudstack/pull/8245/files

Fixes OAuth(2) schema changes removed in Database VIEW Management improvement apache#7417 Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

the trillian test was run 2 weeks ago, too long

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

Co-authored-by: Daniel Augusto Veronezi Salvador <gutoveronezi@apache.org>

…che#8245) Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

GutoVeronezi added 3 commits April 8, 2023 00:08

Execute view scripts each upgrade

10342db

Create file for some views

14219bc

Improve logs

4e38f9c

GutoVeronezi requested review from DaanHoogland, nvazquez, weizhouapache and yadvr April 8, 2023 04:06

GutoVeronezi marked this pull request as draft April 11, 2023 12:16

DaanHoogland approved these changes Apr 17, 2023

View reviewed changes

DaanHoogland added this to the 4.19.0.0 milestone Jun 22, 2023

GutoVeronezi added 2 commits October 30, 2023 09:21

Merge remote-tracking branch 'apache-github/main' into database-views…

02a56b3

…-management-via-separated-files

Run VIEW scripts only once after all upgrades

706ddda

GutoVeronezi added 2 commits October 31, 2023 23:25

Rewrite method to list files under directory

6f31d06

Move views to files

e70aa6b

github-actions Bot added the status:has-conflicts label Nov 1, 2023

Merge branch 'main' into database-views-management-via-separated-files

7ba2f4a

DaanHoogland added the component:database label Nov 7, 2023

DaanHoogland assigned shwstppr Nov 7, 2023

shwstppr removed their assignment Nov 7, 2023

DaanHoogland assigned soreana Nov 15, 2023

soreana requested changes Nov 16, 2023

View reviewed changes

Comment thread utils/src/main/java/com/cloud/utils/FileUtil.java Outdated

Comment thread engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java

GutoVeronezi added 2 commits November 16, 2023 14:29

Join try statements

6d4671e

Switch order in execution

b8cd051

GutoVeronezi requested a review from soreana November 16, 2023 20:21

soreana approved these changes Nov 17, 2023

View reviewed changes

DaanHoogland merged commit adbb552 into apache:main Nov 17, 2023

shwstppr reviewed Nov 17, 2023

View reviewed changes

shwstppr mentioned this pull request Nov 17, 2023

schema: revert incorrect removal of oauth changes in #7417 #8245

Merged

13 tasks

GutoVeronezi mentioned this pull request Nov 17, 2023

Re add script removed by mistake in #7417 #8246

Closed

11 tasks

DaanHoogland pushed a commit that referenced this pull request Nov 17, 2023

schema: revert incorrect removal of oauth changes in #7417 (#8245 )

c7ed4ca

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr mentioned this pull request Nov 17, 2023

Flexible tags for hosts and storage pools #7489

Merged

12 tasks

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Nov 29, 2023

Database VIEW management via separated files (apache#7417 )

699bd6f

Co-authored-by: Daniel Augusto Veronezi Salvador <gutoveronezi@apache.org>

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Nov 29, 2023

schema: revert incorrect removal of oauth changes in apache#7417 (apa…

340a91b

…che#8245) Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr mentioned this pull request Dec 6, 2023

API: Fix listing zones and stoage pools #8311

Closed

13 tasks

DaanHoogland mentioned this pull request Feb 13, 2024

Move views into separate files for easier modification #8647

Merged

31 tasks

-              UPDATE `cloud`.`configuration` SET
-                  `options` = concat(`options`, ',OAUTH2'),
-                  `default_value` = concat(`default_value`, ',OAUTH2'),
-                  `value` = concat(`value`, ',OAUTH2')
-              WHERE `name` = 'user.authenticators.order' ;
-              UPDATE `cloud`.`configuration` SET
-                  `options` = concat(`options`, ',OAUTH2Auth'),
-                  `default_value` = concat(`default_value`, ',OAUTH2Auth'),
-                  `value` = concat(`value`, ',OAUTH2Auth')
-              where `name` = 'pluggableApi.authenticators.order' ;
-              -- Create table for OAuth provider details
-              DROP TABLE IF EXISTS `cloud`.`oauth_provider`;
-              CREATE TABLE `cloud`.`oauth_provider` (
-                `id` bigint unsigned NOT NULL auto_increment COMMENT 'id',
-                `uuid` varchar(40) NOT NULL COMMENT 'unique identifier',
-                `description` varchar(1024) COMMENT 'description of the provider',
-                `provider` varchar(40) NOT NULL COMMENT 'name of the provider',
-                `client_id` varchar(255) NOT NULL COMMENT 'client id which is configured in the provider',
-                `secret_key` varchar(255) NOT NULL COMMENT 'secret key which is configured in the provider',
-                `redirect_uri` varchar(255) NOT NULL COMMENT 'redirect uri which is configured in the provider',
-                `enabled` int(1) NOT NULL DEFAULT 1 COMMENT 'Enabled or disabled',
-                `created` datetime NOT NULL COMMENT 'date created',
-                `removed` datetime COMMENT 'date removed if not null',
-                PRIMARY KEY (`id`)
-                ) ENGINE=InnoDB DEFAULT CHARSET=utf8;

                               }
+                              String upgradedVersion = upgrade.getUpgradedVersion();
+                              executeViewScripts(conn, upgradedVersion);

Database VIEW management via separated files by GutoVeronezi · Pull Request #7417 · apache/cloudstack · GitHub

Uh oh!

Database VIEW management via separated files - #7417

Database VIEW management via separated files#7417DaanHoogland merged 12 commits intoapache:mainapache/cloudstack:mainfrom GutoVeronezi:database-views-management-via-separated-filesGutoVeronezi/cloudstack:database-views-management-via-separated-filesCopy head branch name to clipboard

Conversation

GutoVeronezi commented Apr 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

How Has This Been Tested?

Uh oh!

GutoVeronezi commented Apr 8, 2023

Uh oh!

codecov Bot commented Apr 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud Bot commented Apr 8, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Sep 28, 2023

Uh oh!

DaanHoogland commented Sep 28, 2023

Uh oh!

GutoVeronezi commented Sep 28, 2023

Uh oh!

DaanHoogland commented Sep 28, 2023

Uh oh!

weizhouapache commented Sep 28, 2023

Uh oh!

GutoVeronezi commented Sep 28, 2023

Uh oh!

weizhouapache commented Sep 28, 2023

Uh oh!

GutoVeronezi commented Sep 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland commented Oct 31, 2023

Uh oh!

GutoVeronezi commented Oct 31, 2023

Uh oh!

github-actions Bot commented Nov 1, 2023

Uh oh!

DaanHoogland commented Nov 6, 2023

Uh oh!

shwstppr commented Nov 7, 2023

Uh oh!

DaanHoogland commented Nov 8, 2023

Uh oh!

wido commented Nov 9, 2023

Uh oh!

DaanHoogland commented Nov 13, 2023

Uh oh!

soreana commented Nov 13, 2023

Uh oh!

DaanHoogland commented Nov 15, 2023

Uh oh!

Uh oh!

Uh oh!

soreana commented Nov 16, 2023

Uh oh!

GutoVeronezi commented Nov 16, 2023

Uh oh!

soreana left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Database VIEW management via separated files#7417
DaanHoogland merged 12 commits into
apache:mainfrom
GutoVeronezi:database-views-management-via-separated-files

GutoVeronezi commented Apr 8, 2023 •
edited
Loading

codecov Bot commented Apr 8, 2023 •
edited
Loading

GutoVeronezi commented Sep 30, 2023 •
edited
Loading

GutoVeronezi Nov 17, 2023 •
edited
Loading