Copilot commented on code in PR #12140:
URL: https://github.com/apache/cloudstack/pull/12140#discussion_r2580365347
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +450,76 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
-
- final CloudStackVersion dbVersion =
CloudStackVersion.parse(_dao.getCurrentVersion());
- final String currentVersionValue =
this.getClass().getPackage().getImplementationVersion();
+ doUpgrades(lock);
+ } finally {
+ lock.releaseRef();
+ }
+ }
- if (StringUtils.isBlank(currentVersionValue)) {
- return;
+ private boolean isStandalone() throws CloudRuntimeException {
+ return Transaction.execute(new TransactionCallback<>() {
+ @Override
+ public Boolean doInTransaction(TransactionStatus status) {
+ String sql = "SELECT COUNT(*) FROM `cloud`.`mshost` WHERE
`state` = 'UP'";
+ try (Connection conn =
TransactionLegacy.getStandaloneConnection();
+ PreparedStatement pstmt = conn.prepareStatement(sql);
+ ResultSet rs = pstmt.executeQuery()) {
+ if (rs.next()) {
+ int count = rs.getInt(1);
+ return count == 0;
Review Comment:
The logic here has a subtle issue. The query counts management servers with
`state = 'UP'`, and returns `true` (standalone) when count is 0. However, this
check is being performed by a management server that is currently running.
This means:
1. If this MS has already registered itself in the `mshost` table as 'UP',
the count would be at least 1, and it would never be considered standalone.
2. If this MS hasn't registered yet, the count might be 0 even in a
clustered environment if other MSes are down or not yet started.
The check should likely exclude the current management server from the
count, or check for count <= 1 instead of count == 0, to properly detect if
this is the only running MS.
```suggestion
return count <= 1;
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +450,76 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
-
- final CloudStackVersion dbVersion =
CloudStackVersion.parse(_dao.getCurrentVersion());
- final String currentVersionValue =
this.getClass().getPackage().getImplementationVersion();
+ doUpgrades(lock);
+ } finally {
+ lock.releaseRef();
+ }
+ }
- if (StringUtils.isBlank(currentVersionValue)) {
- return;
+ private boolean isStandalone() throws CloudRuntimeException {
+ return Transaction.execute(new TransactionCallback<>() {
+ @Override
+ public Boolean doInTransaction(TransactionStatus status) {
+ String sql = "SELECT COUNT(*) FROM `cloud`.`mshost` WHERE
`state` = 'UP'";
+ try (Connection conn =
TransactionLegacy.getStandaloneConnection();
+ PreparedStatement pstmt = conn.prepareStatement(sql);
+ ResultSet rs = pstmt.executeQuery()) {
+ if (rs.next()) {
+ int count = rs.getInt(1);
+ return count == 0;
+ }
+ } catch (SQLException e) {
+ String errorMessage = "Unable to check if the management
server is running in standalone mode.";
+ LOGGER.error(errorMessage, e);
+ return false;
+ } catch (NullPointerException npe) {
+ String errorMessage = "Unable to check if the management
server is running in standalone mode. Not able to get a Database connection.";
+ LOGGER.error(errorMessage, npe);
+ return false;
}
+ return true;
+ }
+ });
+ }
- String csVersion =
SystemVmTemplateRegistration.parseMetadataFile();
- final CloudStackVersion sysVmVersion =
CloudStackVersion.parse(csVersion);
- final CloudStackVersion currentVersion =
CloudStackVersion.parse(currentVersionValue);
- SystemVmTemplateRegistration.CS_MAJOR_VERSION =
String.valueOf(sysVmVersion.getMajorRelease()) + "." +
String.valueOf(sysVmVersion.getMinorRelease());
- SystemVmTemplateRegistration.CS_TINY_VERSION =
String.valueOf(sysVmVersion.getPatchRelease());
+ private void doUpgrades(GlobalLock lock) {
+ try {
+ initializeDatabaseEncryptors();
- LOGGER.info("DB version = " + dbVersion + " Code Version = " +
currentVersion);
+ final CloudStackVersion dbVersion =
CloudStackVersion.parse(_dao.getCurrentVersion());
+ final String currentVersionValue =
this.getClass().getPackage().getImplementationVersion();
- if (dbVersion.compareTo(currentVersion) > 0) {
- throw new CloudRuntimeException("Database version " +
dbVersion + " is higher than management software version " +
currentVersionValue);
- }
+ if (StringUtils.isBlank(currentVersionValue)) {
+ return;
+ }
- if (dbVersion.compareTo(currentVersion) == 0) {
- LOGGER.info("DB version and code version matches so no
upgrade needed.");
- return;
- }
+ String csVersion =
SystemVmTemplateRegistration.parseMetadataFile();
+ final CloudStackVersion sysVmVersion =
CloudStackVersion.parse(csVersion);
+ final CloudStackVersion currentVersion =
CloudStackVersion.parse(currentVersionValue);
+ SystemVmTemplateRegistration.CS_MAJOR_VERSION =
String.valueOf(sysVmVersion.getMajorRelease()) + "." +
String.valueOf(sysVmVersion.getMinorRelease());
+ SystemVmTemplateRegistration.CS_TINY_VERSION =
String.valueOf(sysVmVersion.getPatchRelease());
+
+ LOGGER.info("DB version = " + dbVersion + " Code Version = " +
currentVersion);
+ if (dbVersion.compareTo(currentVersion) > 0) {
+ throw new CloudRuntimeException("Database version " +
dbVersion + " is higher than management software version " +
currentVersionValue);
+ }
+
+ if (dbVersion.compareTo(currentVersion) == 0) {
+ LOGGER.info("DB version and code version matches so no upgrade
needed.");
+ return;
+ }
+
+ if (isStandalone()) {
upgrade(dbVersion, currentVersion);
- } finally {
- lock.unlock();
+ } else {
+ String errorMessage = "Database upgrade is required but the
management server is running in a clustered environment. " +
+ "Please perform the database upgrade when the
management server is not running in a clustered environment.";
+ LOGGER.error(errorMessage);
+ throw new CloudRuntimeException(errorMessage);
}
} finally {
- lock.releaseRef();
+ lock.unlock();
}
}
Review Comment:
The new `isStandalone()` method and the clustering check logic in
`doUpgrades()` lack test coverage. Given that this is a critical safety check
to prevent data corruption during upgrades, tests should be added to verify:
1. Correct behavior when no management servers are running (count = 0)
2. Correct behavior when only this management server is running
3. Correct behavior when multiple management servers are UP
4. Error handling when database connection fails
5. The interaction between `isStandalone()` and the upgrade flow
Tests exist for other methods in `DatabaseUpgradeChecker` (see
`DatabaseUpgradeCheckerTest`), so adding tests for this new functionality would
be consistent with the existing test coverage.
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +450,76 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
-
- final CloudStackVersion dbVersion =
CloudStackVersion.parse(_dao.getCurrentVersion());
- final String currentVersionValue =
this.getClass().getPackage().getImplementationVersion();
+ doUpgrades(lock);
+ } finally {
+ lock.releaseRef();
+ }
+ }
- if (StringUtils.isBlank(currentVersionValue)) {
- return;
+ private boolean isStandalone() throws CloudRuntimeException {
+ return Transaction.execute(new TransactionCallback<>() {
+ @Override
+ public Boolean doInTransaction(TransactionStatus status) {
+ String sql = "SELECT COUNT(*) FROM `cloud`.`mshost` WHERE
`state` = 'UP'";
+ try (Connection conn =
TransactionLegacy.getStandaloneConnection();
+ PreparedStatement pstmt = conn.prepareStatement(sql);
+ ResultSet rs = pstmt.executeQuery()) {
+ if (rs.next()) {
+ int count = rs.getInt(1);
+ return count == 0;
+ }
+ } catch (SQLException e) {
+ String errorMessage = "Unable to check if the management
server is running in standalone mode.";
+ LOGGER.error(errorMessage, e);
+ return false;
+ } catch (NullPointerException npe) {
+ String errorMessage = "Unable to check if the management
server is running in standalone mode. Not able to get a Database connection.";
+ LOGGER.error(errorMessage, npe);
+ return false;
}
+ return true;
+ }
+ });
+ }
- String csVersion =
SystemVmTemplateRegistration.parseMetadataFile();
- final CloudStackVersion sysVmVersion =
CloudStackVersion.parse(csVersion);
- final CloudStackVersion currentVersion =
CloudStackVersion.parse(currentVersionValue);
- SystemVmTemplateRegistration.CS_MAJOR_VERSION =
String.valueOf(sysVmVersion.getMajorRelease()) + "." +
String.valueOf(sysVmVersion.getMinorRelease());
- SystemVmTemplateRegistration.CS_TINY_VERSION =
String.valueOf(sysVmVersion.getPatchRelease());
+ private void doUpgrades(GlobalLock lock) {
+ try {
+ initializeDatabaseEncryptors();
- LOGGER.info("DB version = " + dbVersion + " Code Version = " +
currentVersion);
+ final CloudStackVersion dbVersion =
CloudStackVersion.parse(_dao.getCurrentVersion());
+ final String currentVersionValue =
this.getClass().getPackage().getImplementationVersion();
- if (dbVersion.compareTo(currentVersion) > 0) {
- throw new CloudRuntimeException("Database version " +
dbVersion + " is higher than management software version " +
currentVersionValue);
- }
+ if (StringUtils.isBlank(currentVersionValue)) {
+ return;
+ }
- if (dbVersion.compareTo(currentVersion) == 0) {
- LOGGER.info("DB version and code version matches so no
upgrade needed.");
- return;
- }
+ String csVersion =
SystemVmTemplateRegistration.parseMetadataFile();
+ final CloudStackVersion sysVmVersion =
CloudStackVersion.parse(csVersion);
+ final CloudStackVersion currentVersion =
CloudStackVersion.parse(currentVersionValue);
+ SystemVmTemplateRegistration.CS_MAJOR_VERSION =
String.valueOf(sysVmVersion.getMajorRelease()) + "." +
String.valueOf(sysVmVersion.getMinorRelease());
+ SystemVmTemplateRegistration.CS_TINY_VERSION =
String.valueOf(sysVmVersion.getPatchRelease());
+
+ LOGGER.info("DB version = " + dbVersion + " Code Version = " +
currentVersion);
+ if (dbVersion.compareTo(currentVersion) > 0) {
+ throw new CloudRuntimeException("Database version " +
dbVersion + " is higher than management software version " +
currentVersionValue);
+ }
+
+ if (dbVersion.compareTo(currentVersion) == 0) {
+ LOGGER.info("DB version and code version matches so no upgrade
needed.");
+ return;
+ }
+
+ if (isStandalone()) {
upgrade(dbVersion, currentVersion);
- } finally {
- lock.unlock();
+ } else {
+ String errorMessage = "Database upgrade is required but the
management server is running in a clustered environment. " +
+ "Please perform the database upgrade when the
management server is not running in a clustered environment.";
+ LOGGER.error(errorMessage);
Review Comment:
The error message could be more actionable. Instead of just stating that
"the management server is running in a clustered environment", it would be more
helpful to specify:
1. How many management servers are currently UP
2. Which management servers are UP (or at least their count/IDs)
3. Clearer instructions on what the user needs to do (e.g., "Stop all other
management servers before running the upgrade")
This would help administrators quickly identify and resolve the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
