Upgrade: check systemvm template before db changes by ustcweizhou · Pull Request #4582 · apache/cloudstack

ustcweizhou · 2021-01-12T10:14:56Z

Description

Currently in upgrade cloudstack checks the systemvm template after db changes.
if user do not install systemvm template and uprgade cloudstack (for example install packages with newer version), upgrade will fail. in this case it will be very complicated to rollback the db if there is no db backup.

This pr checks the latest (only the last) systemvm template in upgrade path, and exits if required systemvm template is not registered.

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

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

ustcweizhou · 2021-01-12T10:15:40Z

@davidjumani @rhtyd

yadvr · 2021-01-12T10:44:54Z

@ustcweizhou isn't the whole db upgrade path wrapped in a transaction? I suppose the whole transaction would fail without committing anything to the DB?

weizhouapache · 2021-01-12T12:29:44Z

@ustcweizhou isn't the whole db upgrade path wrapped in a transaction? I suppose the whole transaction would fail without committing anything to the DB?

@rhtyd
unfortunately no...
In each upgrade, the transaction will be started, committed and finally closed.
https://github.com/apache/cloudstack/blob/master/engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java#L240-L310
it is bad, but not the worst.

new system template is checked in performMigration. Before that, sql in script will be committed
(1) autocommit is true.
in https://github.com/apache/cloudstack/blob/master/framework/db/src/main/java/com/cloud/utils/db/ScriptRunner.java#L148-L150
(2) autocommit is false.
https://github.com/apache/cloudstack/blob/master/framework/db/src/main/java/com/cloud/utils/db/ScriptRunner.java#L181-L183

options are
(1) check systemvm template at the beginning of whole upgrade, or
(2) check systemvm template at each version upgrade.

RodrigoDLopez

code lgtm

DaanHoogland

good functionality, good code.

DaanHoogland · 2021-01-18T19:40:11Z

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

+        for (int i = upgrades.length - 1; i >= 0; i--) {
+            DbUpgrade upgrade = upgrades[i];
+            if (upgrade instanceof DbUpgradeSystemVmTemplate) {
+                TransactionLegacy txn = TransactionLegacy.open("Upgrade");
+                txn.start();
+                try {
+                    Connection conn;
+                    try {
+                        conn = txn.getConnection();
+                    } catch (SQLException e) {
+                        String errorMessage = "Unable to upgrade the database";
+                        s_logger.error(errorMessage, e);
+                        throw new CloudRuntimeException(errorMessage, e);
+                    }
+                    ((DbUpgradeSystemVmTemplate)upgrade).updateSystemVmTemplates(conn);
+                    break;
+                } catch (CloudRuntimeException e) {
+                    String errorMessage = "Unable to upgrade the database";
+                    s_logger.error(errorMessage, e);
+                    throw new CloudRuntimeException(errorMessage, e);
+                } finally {
+                    txn.close();
+                }
+            }
+        }


can we put this in a separate method?

@DaanHoogland yes, done

DaanHoogland · 2021-01-18T19:45:17Z

test proposal:

don't upload templates,
upgrade,
start,
stop,
downgrade,
start,
upload templates,
stop,
start.
agree?

weizhouapache · 2021-01-19T08:51:19Z

test proposal:

don't upload templates,

upgrade,

start,

stop,

downgrade,

start,

upload templates,

stop,

start.
agree?

@DaanHoogland yes, exactly

weizhouapache · 2021-01-19T08:51:44Z

@DaanHoogland @rhtyd
can you kick off trillian test ?

shwstppr · 2021-01-25T09:34:41Z

@blueorangutan package

blueorangutan · 2021-01-25T09:35:56Z

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

blueorangutan · 2021-01-25T10:41:14Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2583

DaanHoogland · 2021-01-26T08:09:07Z

@blueorangutan test

blueorangutan · 2021-01-26T08:10:05Z

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

weizhouapache · 2021-01-26T09:22:02Z

jenkins build failed at point below. close/open this pr to retry.

[INFO] --< org.apache.cloudstack:cloud-plugin-integrations-cloudian-connector >--
[INFO] Building Apache CloudStack Plugin - Cloudian Connector 4.15.1.0-SNAPSHOT [74/116]
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- maven-checkstyle-plugin:3.1.0:check (cloudstack-checkstyle) @ cloud-plugin-integrations-cloudian-connector ---
[INFO] Starting audit...
Audit done.
[INFO] 
[INFO] --- jacoco-maven-plugin:0.8.3:prepare-agent (prepare-coverage-agent) @ cloud-plugin-integrations-cloudian-connector ---
[INFO] argLine set to -javaagent:/home/jenkins/.m2/repository/org/jacoco/org.jacoco.agent/0.8.3/org.jacoco.agent-0.8.3-runtime.jar=destfile=/home/jenkins/jenkins-agent/workspace/Cloudstack/cloudstack-pr-analysis/plugins/integrations/cloudian/target/jacoco.exec
[INFO] 
[INFO] --- maven-remote-resources-plugin:1.3:process (default) @ cloud-plugin-integrations-cloudian-connector ---
[INFO] 
[INFO] --- maven-resources-plugin:2.5:resources (default-resources) @ cloud-plugin-integrations-cloudian-connector ---
[debug] execute contextualize
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] Copying 2 resources
[INFO] Copying 3 resources
[INFO] 
[INFO] --- maven-compiler-plugin:3.8.1:compile (default-compile) @ cloud-plugin-integrations-cloudian-connector ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 10 source files to /home/jenkins/jenkins-agent/workspace/Cloudstack/cloudstack-pr-analysis/plugins/integrations/cloudian/target/classes
[INFO] 
[INFO] --- maven-resources-plugin:2.5:testResources (default-testResources) @ cloud-plugin-integrations-cloudian-connector ---
[debug] execute contextualize
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] skip non existing resourceDirectory /home/jenkins/jenkins-agent/workspace/Cloudstack/cloudstack-pr-analysis/plugins/integrations/cloudian/src/test/resources
[INFO] Copying 3 resources
[INFO] 
[INFO] --- maven-compiler-plugin:3.8.1:testCompile (default-testCompile) @ cloud-plugin-integrations-cloudian-connector ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 1 source file to /home/jenkins/jenkins-agent/workspace/Cloudstack/cloudstack-pr-analysis/plugins/integrations/cloudian/target/test-classes
[INFO] 
[INFO] --- maven-surefire-plugin:2.22.2:test (default-test) @ cloud-plugin-integrations-cloudian-connector ---
[INFO] 
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.cloudstack.cloudian.CloudianClientTest
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
log4j:WARN No appenders could be found for logger (org.apache.cloudstack.cloudian.client.CloudianClient).
log4j:WARN Please initialize the log4j system properly.
log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info.
[ERROR] Tests run: 25, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 9.1 s <<< FAILURE! - in org.apache.cloudstack.cloudian.CloudianClientTest
[ERROR] addUserAccountFail(org.apache.cloudstack.cloudian.CloudianClientTest)  Time elapsed: 5.447 s  <<< ERROR!
org.apache.cloudstack.api.ServerApiException: Operation timed out, please try again.
	at org.apache.cloudstack.cloudian.CloudianClientTest.addUserAccountFail(CloudianClientTest.java:145)

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Errors: 
[ERROR]   CloudianClientTest.addUserAccountFail:145 » ServerApi Operation timed out, ple...
[INFO] 
[ERROR] Tests run: 25, Failures: 0, Errors: 1, Skipped: 0

blueorangutan · 2021-01-26T19:01:46Z

Trillian test result (tid-3413)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37454 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4582-t3413-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Smoke tests completed. 86 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

Pearl1594 · 2021-02-01T09:53:33Z

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

+                    s_logger.error(errorMessage, e);
+                    throw new CloudRuntimeException(errorMessage, e);
+                } finally {
+                    txn.close();


@ustcweizhou I believe there's a need to do a txn.commit() before closing the transaction as the DB changes made as part of the updateSystemVmTemplate() gets rolled back

Rolling back the transaction: Time = 22 Name = Upgrade; called by -TransactionLegacy.rollback:888-TransactionLegacy.removeUpTo:831-TransactionLegacy.close:655-DatabaseUpgradeChecker.updateSystemVmTemplates:264-DatabaseUpgradeChecker.upgrade:275-DatabaseUpgradeChecker.check:379-CloudStackExtendedLifeCycle.checkIntegrity:64-CloudStackExtendedLifeCycle.start:54-DefaultLifecycleProcessor.doStart:182-DefaultLifecycleProcessor.access$200:53-DefaultLifecycleProcessor$LifecycleGroup.start:360-DefaultLifecycleProcessor.startBeans:158 2021-02-01 09:50:57,795 DEBUG [c.c.u.DatabaseUpgradeChecker] (main:null) (logid:) Running upgrade Upgrade41500to41510 to upgrade from 4.15.0.0-4.15.1.0 to 4.15.1.0

@ustcweizhou I believe there's a need to do a txn.commit() before closing the transaction as the DB changes made as part of the updateSystemVmTemplate() gets rolled back

Rolling back the transaction: Time = 22 Name = Upgrade; called by -TransactionLegacy.rollback:888-TransactionLegacy.removeUpTo:831-TransactionLegacy.close:655-DatabaseUpgradeChecker.updateSystemVmTemplates:264-DatabaseUpgradeChecker.upgrade:275-DatabaseUpgradeChecker.check:379-CloudStackExtendedLifeCycle.checkIntegrity:64-CloudStackExtendedLifeCycle.start:54-DefaultLifecycleProcessor.doStart:182-DefaultLifecycleProcessor.access$200:53-DefaultLifecycleProcessor$LifecycleGroup.start:360-DefaultLifecycleProcessor.startBeans:158 2021-02-01 09:50:57,795 DEBUG [c.c.u.DatabaseUpgradeChecker] (main:null) (logid:) Running upgrade Upgrade41500to41510 to upgrade from 4.15.0.0-4.15.1.0 to 4.15.1.0

@Pearl1594 thanks for testing.
could you tell how to reproduce the issue ?

@weizhouapache I tested this by merging this code to master. However, the steps are equivalent to :

Deploy a 4.14 env

Register the template (4.15 template)

upgrade to 4.15

The Management server comes up fine, but, if we check the cloud.configuration value (in the DB) for attribute - "router.template." notice it doesn't update the template name to the new one.

The case of not applying the DB changes when template isn't registered, works absolutely fine.

@Pearl1594 yeah, you are right !
pushed a change.
thanks for review and testing.

yadvr · 2021-02-02T10:36:13Z

@blueorangutan package

blueorangutan · 2021-02-02T10:37:53Z

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

blueorangutan · 2021-02-02T11:32:08Z

Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2630

Pearl1594 · 2021-02-02T12:06:30Z

@blueorangutan package

blueorangutan · 2021-02-02T12:07:32Z

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

blueorangutan · 2021-02-02T12:45:02Z

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2636

Pearl1594

LGTM

yadvr · 2021-02-19T09:02:54Z

@blueorangutan package

blueorangutan · 2021-02-19T09:04:01Z

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

blueorangutan · 2021-02-19T11:31:27Z

Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2754

shwstppr · 2021-02-22T09:59:56Z

@blueorangutan package

blueorangutan · 2021-02-22T10:00:58Z

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

blueorangutan · 2021-02-23T09:15:11Z

Packaging result: ✖centos7 ✔centos8 ✔debian. JID-2807

yadvr · 2021-02-23T10:59:04Z

@blueorangutan package

blueorangutan · 2021-02-23T11:00:02Z

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

blueorangutan · 2021-02-23T11:58:21Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2815

DaanHoogland · 2021-02-23T13:48:46Z

I think we don't need this as the upgrade was tested manually by @Pearl1594 . better safe though
@blueorangutan test

blueorangutan · 2021-02-23T13:50:03Z

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

blueorangutan · 2021-02-23T23:32:56Z

Trillian test result (tid-3608)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32793 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4582-t3608-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_human_readable_logs.py
Smoke tests completed. 86 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

shwstppr

LGTM

yadvr · 2021-02-24T10:56:14Z

engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41400to41500.java


    @Override
    public void performDataMigration(Connection conn) {
-        updateSystemVmTemplates(conn);


+1 important thing to remember now is not to run this again

Upgrade: check systemvm template before db changes

7e9babc

ustcweizhou force-pushed the 4.15-check-systemvm-template-before-db branch from 575cbcd to 7e9babc Compare January 12, 2021 12:33

RodrigoDLopez approved these changes Jan 15, 2021

View reviewed changes

DaanHoogland approved these changes Jan 18, 2021

View reviewed changes

Upgrade: move some codes to a separated method

93c7c22

shwstppr added this to the 4.15.1.0 milestone Jan 25, 2021

weizhouapache closed this Jan 25, 2021

weizhouapache reopened this Jan 25, 2021

weizhouapache closed this Jan 26, 2021

weizhouapache reopened this Jan 26, 2021

ustcweizhou closed this Jan 27, 2021

ustcweizhou reopened this Jan 27, 2021

yadvr requested review from Pearl1594, shwstppr and sureshanaparti January 28, 2021 10:26

Pearl1594 reviewed Feb 1, 2021

View reviewed changes

apache#4582 add txn.commit()

6d63f87

Pearl1594 approved these changes Feb 2, 2021

View reviewed changes

yadvr closed this Feb 19, 2021

yadvr reopened this Feb 19, 2021

weizhouapache closed this Feb 22, 2021

weizhouapache reopened this Feb 22, 2021

apache deleted a comment from blueorangutan Feb 23, 2021

shwstppr approved these changes Feb 24, 2021

View reviewed changes

yadvr approved these changes Feb 24, 2021

View reviewed changes

yadvr reviewed Feb 24, 2021

View reviewed changes

yadvr merged commit 5a3ae15 into apache:4.15 Feb 24, 2021

weizhouapache mentioned this pull request Mar 12, 2021

skip livemigration for centos #4801

Merged

12 tasks

anniejili mentioned this pull request Jan 13, 2024

Adding vmId as part of error response when vm create fails. #8484

Merged

12 tasks

Conversation

ustcweizhou commented Jan 12, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

ustcweizhou commented Jan 12, 2021

Uh oh!

yadvr commented Jan 12, 2021

Uh oh!

weizhouapache commented Jan 12, 2021

Uh oh!

RodrigoDLopez left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jan 18, 2021

Choose a reason for hiding this comment

Uh oh!

weizhouapache Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jan 18, 2021

Uh oh!

weizhouapache commented Jan 19, 2021

Uh oh!

weizhouapache commented Jan 19, 2021

Uh oh!

shwstppr commented Jan 25, 2021

Uh oh!

blueorangutan commented Jan 25, 2021

Uh oh!

blueorangutan commented Jan 25, 2021

Uh oh!

DaanHoogland commented Jan 26, 2021

Uh oh!

blueorangutan commented Jan 26, 2021

Uh oh!

weizhouapache commented Jan 26, 2021

Uh oh!

blueorangutan commented Jan 26, 2021

Uh oh!

Pearl1594 Feb 1, 2021 • 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!

weizhouapache Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

Pearl1594 Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

weizhouapache Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

yadvr commented Feb 2, 2021

Uh oh!

blueorangutan commented Feb 2, 2021

Uh oh!

blueorangutan commented Feb 2, 2021

Uh oh!

Pearl1594 commented Feb 2, 2021

Uh oh!

blueorangutan commented Feb 2, 2021

Uh oh!

blueorangutan commented Feb 2, 2021

Uh oh!

Pearl1594 left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Feb 19, 2021

Uh oh!

blueorangutan commented Feb 19, 2021

Uh oh!

blueorangutan commented Feb 19, 2021

Pearl1594 Feb 1, 2021 •

edited

Loading