[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user karuturi commented on the issue: https://github.com/apache/cloudstack/pull/1768 Thanks everyone. merging this now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user DaanHoogland commented on the issue: https://github.com/apache/cloudstack/pull/1768 [1768.network.results.txt](https://github.com/apache/cloudstack/files/805988/1768.network.results.txt) [1768.vpc_routers.results.txt](https://github.com/apache/cloudstack/files/805989/1768.vpc_routers.results.txt) just for form @karuturi . I see failures in network but can not relate these to the upgrades --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user DaanHoogland commented on the issue: https://github.com/apache/cloudstack/pull/1768 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user rhtyd commented on the issue: https://github.com/apache/cloudstack/pull/1768 @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 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user karuturi commented on the issue: https://github.com/apache/cloudstack/pull/1768 Actually, BVT is not going to verify this as this is db upgrade related and travis would have tested it. I am merging this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user marcaurele commented on the issue: https://github.com/apache/cloudstack/pull/1768 @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? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user rhtyd commented on the issue: https://github.com/apache/cloudstack/pull/1768 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user blueorangutan commented on the issue: https://github.com/apache/cloudstack/pull/1768 @DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user DaanHoogland commented on the issue: https://github.com/apache/cloudstack/pull/1768 @blueorangutan test --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user karuturi commented on the issue: https://github.com/apache/cloudstack/pull/1768 @DaanHoogland Can you post test results? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user remibergsma commented on the issue: https://github.com/apache/cloudstack/pull/1768 @marcaurele Nice improvement, thanks! Tested it and works as you described. LGTM. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user marcaurele commented on the issue: https://github.com/apache/cloudstack/pull/1768 Forget my last comment, the files can remains as they are. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user DaanHoogland commented on the issue: https://github.com/apache/cloudstack/pull/1768 @blueorangutan test --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user marcaurele commented on the issue: https://github.com/apache/cloudstack/pull/1768 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user blueorangutan commented on the issue: https://github.com/apache/cloudstack/pull/1768 Packaging result: âcentos6 âcentos7 âdebian. JID-532 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user borisstoyanov commented on the issue: https://github.com/apache/cloudstack/pull/1768 @blueorangutan package --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user blueorangutan commented on the issue: https://github.com/apache/cloudstack/pull/1768 @borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user DaanHoogland commented on the issue: https://github.com/apache/cloudstack/pull/1768 @BlueOrangUtan help --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user DaanHoogland commented on the issue: https://github.com/apache/cloudstack/pull/1768 @BlueOrangUtan package --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user DaanHoogland commented on the issue: https://github.com/apache/cloudstack/pull/1768 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user marcaurele commented on the issue: https://github.com/apache/cloudstack/pull/1768 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user syed commented on the issue: https://github.com/apache/cloudstack/pull/1768 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-vxlan 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user marcaurele commented on the issue: https://github.com/apache/cloudstack/pull/1768 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 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user DaanHoogland commented on the issue: https://github.com/apache/cloudstack/pull/1768 @karuturi I think this should go in. Even being a bit of a risk, it will potentially reduce the support hours on upgrades. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user DaanHoogland commented on the issue: https://github.com/apache/cloudstack/pull/1768 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user DaanHoogland commented on the issue: https://github.com/apache/cloudstack/pull/1768 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user marcaurele commented on the issue: https://github.com/apache/cloudstack/pull/1768 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 ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user marcaurele commented on the issue: https://github.com/apache/cloudstack/pull/1768 @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.* ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user serg38 commented on the issue: https://github.com/apache/cloudstack/pull/1768 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user marcaurele commented on the issue: https://github.com/apache/cloudstack/pull/1768 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user rhtyd commented on the issue: https://github.com/apache/cloudstack/pull/1768 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack issue #1768: CLOUDSTACK 9601: Upgrade: change logic for update pa...
Github user marcaurele commented on the issue: https://github.com/apache/cloudstack/pull/1768 The second commit can be removed if the cleanup is not wanted. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
