CLOUDSTACK 9601: Upgrade: change logic for update path for files

[marcaurele]

For going from version A to version D, it uses to run the SQL files in
that order: A -> B -> C -> D -> A-cleanup -> B-cleanup -> C-cleanup ->
D-cleanup. If you had upgraded each version separatively you would have
run A -> A-cleanup -> B -> B-cleanup -> C -> C-cleanup -> D ->
D-cleanup.
This change the logic to follow the same path if you are jumping over
versions.

Signed-off-by: Marc-Aurèle Brothier m@brothier.org

[marcaurele]

The second commit can be removed if the cleanup is not wanted.

[rohityadavcloud]

This may break db upgrade, as the cleanup paths are run at the end. Can you prove if changing the sequence will be idempotent? Thanks.

[marcaurele]

@rhtyd Changing the sequence is only idempotent if you have been upgrading to each new versions step by step. If you jumped other versions, then the path is different for each original version. This fix wants to avoid such a difference.

[serg38]

@marcaurele I tend to agree with @rhtydthat it could break some of the upgrades. If I get it right with your proposed changes, upgrade scripts become obsolete since all the changes can be done in upgrade scripts.

[marcaurele]

@serg38 Not sure to understand what you mean with:

If I get it right with your proposed changes, upgrade scripts become obsolete since all the changes can be done in upgrade scripts.

You meant: If I get it right with your proposed changes, upgrade cleanup scripts become obsolete since all the changes can be done in upgrade scripts. ?

[marcaurele]

I'll try to make my point clearer with a better use case. Let say you were running version ACS 4.4.2 and wish to upgrade to 4.7.1. After installing the 4.7.1, when ACS starts for the first time you will execute SQL scripts in that order (case A):

schema-442to450.sql       -----> schema-442to450-cleanup.sql
      |                  |             |
      v                  |             v
schema-450to451.sql      |       schema-450to451-cleanup.sql
      |                  |             |
      v                  |             v
schema-451to452.sql      |       schema-451to452-cleanup.sql
      |                  |             |
      v                  |             v
schema-452to460.sql      |       schema-452to460-cleanup.sql
      |                  |             |
      v                  |             v
schema-460to461.sql      |       schema-460to461-cleanup.sql
      |                  |             |
      v                  |             v
schema-461to470.sql      |       schema-461to470-cleanup.sql
      |                  |             |
      v                  |             v
schema-470to471.sql >----        schema-470to471-cleanup.sql

But if you would have updated to each versions, one after the other, you would have run those scripts in that order (case B):

schema-442to450.sql -----> schema-442to450-cleanup.sql
                                 |
       --------------------------
      |
      v
schema-450to451.sql -----> schema-450to451-cleanup.sql
                                 |
       --------------------------
      |
      v
schema-451to452.sql -----> schema-451to452-cleanup.sql
                                 |
       --------------------------
      |
      v
schema-452to460.sql -----> schema-452to460-cleanup.sql
                                 |
       --------------------------
      |
      v
schema-460to461.sql -----> schema-460to461-cleanup.sql
                                 |
       --------------------------
      |
      v
schema-461to470.sql -----> schema-461to470-cleanup.sql
                                 |
       --------------------------
      |
      v
schema-470to471.sql -----> schema-470to471-cleanup.sql

Since case B is that most developer would expect when fixing bugs and doing changes, but case A is the most common case of production upgrade, I wanted to correct the algorithm so that everyone will follow the same route (case B).

Most -cleanup.sql scripts are either empty or only updating the configuration table, so it's safe. There is only one possible problematic script: https://github.com/apache/cloudstack/blob/master/setup/db/db/schema-481to490-cleanup.sql today. This one does change views, which IMO was a mistake to put in the cleanup script file, it should have gone into schema-481to490.sql (@rhtyd ?). Leaving the mechanism as it is today would leave people with a possible bug while upgrading from any version prior to 4.9.0 if any future SQL script was to change the views modified inside schema-481to490-cleanup.sql because of scenario case A. Did I lost people there?

Any comment @remibergsma @DaanHoogland @syed @nvazquez ?

[DaanHoogland]

@marcaurele your point is clear and valid. I see that the change worries people as it will actually change existing upgrade paths.
@serg38 @marcaurele 's version of your comment doesn't hold if changes in java are encoded as well!
/me about to look at the code

[DaanHoogland]

LGTM but
I hope you appreciate the concerns @marcaurele , on the other hand let's fix what breaks and make sure the 4.9 to 4.10 works with this. We can always advice to upgrade to 4.9 before going up.

[DaanHoogland]

btw @marcaurele @rhtyd idempotent will only be possible if we encode it. It is not encoded in older upgrades. Unless we clean those upgrades up we will have some issues.

[DaanHoogland]

@karuturi I think this should go in. Even being a bit of a risk, it will potentially reduce the support hours on upgrades.

[marcaurele]

As a reminder for -cleanup SQL scripts:

We rarely need cleanup scripts, only when some field/value is required during the data migration and has to be dropped later on.

https://cwiki.apache.org/confluence/display/CLOUDSTACK/DB+Upgrade+in+CloudStack

[syed]

I agree with @DaanHoogland , as @marcaurele mentioned, the only file we need to worry about is schema-481to490-cleanup.sql the rest of them are either empty or change the configuration where the order of execution doesn't really matter. Looking at it briefly it looks like it modifies ovs_tunnel_network table which is used to manage OVS tunnels when using non-vlan isolation. I've never seen anyone use this in production. Would be useful if we can find someone who has used this before. Or if no one is using, it is LGTM from me as well.

[marcaurele]

@syed I would prefer to move what's inside schema-481to490-cleanup.sql to the end of the file schema-481to490.sql as it would have been the way of processing during an update from 481 to 490.
If I get the go about the move, I'll do it too in this PR.

[DaanHoogland]

@syed the -cleanup scripts are for removing data that had been migrated, not for temporary tables. also a use case for those may be if things are done partly in the migrate script and partly in the migrate class, though I can't think of a real instance of that use case.
as for the ovs-tunnel code, we don't have to loose that, do we? SO even with people using it we can apply this.

[DaanHoogland]

@blueorangutan package

[DaanHoogland]

@blueorangutan help

[borisstoyanov]

@blueorangutan package

[blueorangutan]

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-532

[marcaurele]

I looked at the code inside Upgrade481to490.java and I cannot understand why the table alterations have been made inside the Java class instead of the SQL file. Because now we cannot move the code from the -cleanup without changing that class too, other the column role_id won't be present when creating the account_view.
To make the change transparent, I'll have to change those too. I'll push another commit for that.

[DaanHoogland]

@blueorangutan test

[marcaurele]

Forget my last comment, the files can remains as they are.

[remibergsma]

@marcaurele Nice improvement, thanks! Tested it and works as you described. LGTM.

[karuturi]

@DaanHoogland Can you post test results?

[DaanHoogland]

@blueorangutan test

[blueorangutan]

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

[rohityadavcloud]

@marcaurele can we have a unit test to confirm that this does not break upgrade for older systems, also by changing the order for all older versions, the way someone would upgrade from say 4.3 to 4.10, will be different from someone who upgraded from 4.3->4.9->4.10 -- this may have side effects.

[marcaurele]

@rhtyd I don't see how I could write such a test case. My point is exactly what you're saying, the upgrade would be different today for anyone who comes from an older version, which I don't think you're currently testing. I want to avoid such situation, therefore I'm pushing this PR to stop having different upgrade paths.
Can you be more explicit in what you think can become a problem compared to the current situation we're in today?

[karuturi]

Actually, BVT is not going to verify this as this is db upgrade related and travis would have tested it.
I will merge this once @rhtyd and @marcaurele come to an agreement

[rohityadavcloud]

I'm not sure about the implications, but since 2.x is a very old version -- we need to put in our release notes that we won't support upgrades from very old 2.x releases. We can in that case even remove all 2.x upgrade paths.

[marcaurele]

I thought that anything older than 4.x should be removed. The simulator and DB schema create a structure based on 4.0.0 or 4.1.0 if I recall correctly.
I initially added another commit (removed now) which was only leaving upgrade paths from version 4.0.0. That can be done later, and I think it would be a good cleanup.

[rohityadavcloud]

@marcaurele I appreciate the cleanup, it should have been like this from the beginning but since we've the workflow to execute all the upgrades first and then the cleanup. In https://github.com/apache/cloudstack/blob/master/setup/db/db/schema-481to490-cleanup.sql the views are executed at the end because certain columns are added by the java based upgrade path and views can only access them after the script/java-based-upgrade is complete. I think with this change future db upgrade paths need to written carefully.

I've no objections with the overall goal to cleanup the sequence logic, though I have a concern that this might cause upgrade regressions in some environments as writing upgrade validation tests may not be possible. /cc @DaanHoogland @karuturi

[DaanHoogland]

@rhtyd I don't see how your concern is more true for the new code by @marcaurele then it is true for old code. cleanup is still going to be executed after the scripts, is it? only directly after instead of waiting for unrelated upgrade scripts to complete as well. We never have and never can have full coverage of upgrades as every cloud is a different snowflake. So we'll need to fix problems after intitial release most of the time. That is no worse or better with this code it is just a little more predictable.

[DaanHoogland]

1768.network.results.txt
1768.vpc_routers.results.txt

just for form @karuturi . I see failures in network but can not relate these to the upgrades

[karuturi]

Thanks everyone. merging this now.