Copilot commented on code in PR #12140:
URL: https://github.com/apache/cloudstack/pull/12140#discussion_r2565221690
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +451,71 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
+ // not sure about the right moment to do this yet
+ checkIfStandalone();
+ doUpgrades(lock);
+ } finally {
+ lock.releaseRef();
+ }
+ }
+ private void checkIfStandalone() throws CloudRuntimeException {
+ boolean standalone = Transaction.execute(new TransactionCallback<>() {
+ @Override
+ public Boolean doInTransaction(TransactionStatus status) {
+ String sql = "SELECT COUNT(*) FROM `cloud`.`management_server`
WHERE `status` = '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 <= 1;
+ }
+ } catch (SQLException e) {
+ String errorMessage = "Unable to check if the management
server is running in standalone mode.";
+ LOGGER.error(errorMessage, e);
+ throw new CloudRuntimeException(errorMessage, e);
+ }
+ return true;
+ }
+ });
+ if (! standalone) {
+ String msg = "CloudStack is running multiple management servers
and attempting to upgrade. Upgrades can only be run in standalone mode.
Skipping database upgrade check.";
Review Comment:
The error message is misleading. It says "Skipping database upgrade check"
but the code is actually throwing an exception which will abort the upgrade
process, not skip it.
Consider revising the message to something like: "CloudStack is running
multiple management servers. Database upgrades can only be performed in
standalone mode. Aborting upgrade."
```suggestion
String msg = "CloudStack is running multiple management servers.
Database upgrades can only be performed in standalone mode. Aborting upgrade.";
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +451,71 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
+ // not sure about the right moment to do this yet
Review Comment:
This comment should be removed before merging. It indicates uncertainty
about the implementation ("not sure about the right moment to do this yet"),
which suggests the code may need further review or testing before being
production-ready.
```suggestion
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +451,71 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
+ // not sure about the right moment to do this yet
+ checkIfStandalone();
+ doUpgrades(lock);
+ } finally {
+ lock.releaseRef();
+ }
+ }
+ private void checkIfStandalone() throws CloudRuntimeException {
+ boolean standalone = Transaction.execute(new TransactionCallback<>() {
+ @Override
+ public Boolean doInTransaction(TransactionStatus status) {
+ String sql = "SELECT COUNT(*) FROM `cloud`.`management_server`
WHERE `status` = '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 <= 1;
+ }
+ } catch (SQLException e) {
+ String errorMessage = "Unable to check if the management
server is running in standalone mode.";
+ LOGGER.error(errorMessage, e);
+ throw new CloudRuntimeException(errorMessage, e);
+ }
+ return true;
+ }
+ });
Review Comment:
The `Transaction.execute()` wrapper is unnecessary here. The code inside the
callback is obtaining its own standalone connection via
`TransactionLegacy.getStandaloneConnection()`, which means the transaction
context provided by `Transaction.execute()` is not being used.
Consider simplifying this to directly execute the query without the
Transaction.execute wrapper, or use the TransactionStatus parameter properly if
transaction management is needed.
```suggestion
String sql = "SELECT COUNT(*) FROM `cloud`.`management_server` WHERE
`status` = 'UP'";
boolean standalone = true;
try (Connection conn = TransactionLegacy.getStandaloneConnection();
PreparedStatement pstmt = conn.prepareStatement(sql);
ResultSet rs = pstmt.executeQuery()) {
if (rs.next()) {
int count = rs.getInt(1);
standalone = count <= 1;
}
} catch (SQLException e) {
String errorMessage = "Unable to check if the management server
is running in standalone mode.";
LOGGER.error(errorMessage, e);
throw new CloudRuntimeException(errorMessage, e);
}
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +451,71 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
+ // not sure about the right moment to do this yet
+ checkIfStandalone();
+ doUpgrades(lock);
+ } finally {
+ lock.releaseRef();
+ }
+ }
+ private void checkIfStandalone() throws CloudRuntimeException {
+ boolean standalone = Transaction.execute(new TransactionCallback<>() {
+ @Override
+ public Boolean doInTransaction(TransactionStatus status) {
+ String sql = "SELECT COUNT(*) FROM `cloud`.`management_server`
WHERE `status` = '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 <= 1;
+ }
+ } catch (SQLException e) {
+ String errorMessage = "Unable to check if the management
server is running in standalone mode.";
+ LOGGER.error(errorMessage, e);
+ throw new CloudRuntimeException(errorMessage, e);
+ }
+ return true;
+ }
+ });
+ if (! standalone) {
+ String msg = "CloudStack is running multiple management servers
and attempting to upgrade. Upgrades can only be run in standalone mode.
Skipping database upgrade check.";
+ LOGGER.info(msg);
Review Comment:
The log level should be `LOGGER.error()` instead of `LOGGER.info()` since an
exception is being thrown immediately after. This is an error condition, not
informational.
```suggestion
LOGGER.error(msg);
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +451,71 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
+ // not sure about the right moment to do this yet
+ checkIfStandalone();
+ doUpgrades(lock);
+ } finally {
+ lock.releaseRef();
+ }
+ }
+ private void checkIfStandalone() throws CloudRuntimeException {
+ boolean standalone = Transaction.execute(new TransactionCallback<>() {
+ @Override
+ public Boolean doInTransaction(TransactionStatus status) {
+ String sql = "SELECT COUNT(*) FROM `cloud`.`management_server`
WHERE `status` = '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 <= 1;
+ }
+ } catch (SQLException e) {
+ String errorMessage = "Unable to check if the management
server is running in standalone mode.";
+ LOGGER.error(errorMessage, e);
+ throw new CloudRuntimeException(errorMessage, e);
+ }
+ return true;
+ }
+ });
+ if (! standalone) {
+ String msg = "CloudStack is running multiple management servers
and attempting to upgrade. Upgrades can only be run in standalone mode.
Skipping database upgrade check.";
+ LOGGER.info(msg);
+ throw new CloudRuntimeException(msg);
+ }
+ }
Review Comment:
The new `checkIfStandalone()` method lacks test coverage. Consider adding
unit tests to verify:
1. The behavior when no management servers are running (count = 0)
2. The behavior when exactly one management server is running (count = 1)
3. The behavior when multiple management servers are running (count > 1)
4. The behavior when a SQLException occurs during the check
This is particularly important since this check prevents upgrades from
proceeding when multiple servers are detected.
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +451,71 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
+ // not sure about the right moment to do this yet
+ checkIfStandalone();
+ doUpgrades(lock);
+ } finally {
+ lock.releaseRef();
+ }
+ }
+ private void checkIfStandalone() throws CloudRuntimeException {
+ boolean standalone = Transaction.execute(new TransactionCallback<>() {
+ @Override
+ public Boolean doInTransaction(TransactionStatus status) {
+ String sql = "SELECT COUNT(*) FROM `cloud`.`management_server`
WHERE `status` = '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 <= 1;
+ }
Review Comment:
The logic `count <= 1` may be problematic. If the count is 0, it means no
management servers are registered as 'UP' in the database, including the
current server executing this check. This could indicate:
1. The current server hasn't registered itself yet in the management_server
table
2. A database connectivity or query issue
Consider whether the check should fail if count is 0, or if this scenario is
expected during startup. The condition might be more accurate as `count == 1`
with explicit handling for the count == 0 case.
```suggestion
if (count == 1) {
return true;
} else if (count == 0) {
String errorMessage = "No management servers are
registered as 'UP' in the database. This may indicate a startup issue or a
database connectivity problem.";
LOGGER.error(errorMessage);
throw new CloudRuntimeException(errorMessage);
} else {
return false;
}
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +451,71 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
+ // not sure about the right moment to do this yet
+ checkIfStandalone();
Review Comment:
If `checkIfStandalone()` throws an exception, the database lock will remain
held (not unlocked) even though the reference is released in the finally block.
This could prevent other processes from acquiring the lock.
Consider ensuring `lock.unlock()` is called in all error paths, or move
`checkIfStandalone()` inside `doUpgrades()` so it's protected by the same
try-finally that handles `lock.unlock()`.
```suggestion
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +451,71 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
+ // not sure about the right moment to do this yet
+ checkIfStandalone();
+ doUpgrades(lock);
+ } finally {
+ lock.releaseRef();
+ }
+ }
+ private void checkIfStandalone() throws CloudRuntimeException {
+ boolean standalone = Transaction.execute(new TransactionCallback<>() {
+ @Override
+ public Boolean doInTransaction(TransactionStatus status) {
+ String sql = "SELECT COUNT(*) FROM `cloud`.`management_server`
WHERE `status` = 'UP'";
+ try (Connection conn =
TransactionLegacy.getStandaloneConnection();
Review Comment:
[nitpick] Extra whitespace after `conn` before the closing parenthesis.
Should be `Connection conn = TransactionLegacy.getStandaloneConnection();` for
consistency.
```suggestion
try (Connection conn =
TransactionLegacy.getStandaloneConnection();
```
##########
engine/schema/src/main/java/com/cloud/upgrade/DatabaseUpgradeChecker.java:
##########
@@ -448,39 +451,71 @@ public void check() {
throw new CloudRuntimeException("Unable to acquire lock to
check for database integrity.");
}
- try {
- initializeDatabaseEncryptors();
+ // not sure about the right moment to do this yet
+ checkIfStandalone();
+ doUpgrades(lock);
+ } finally {
+ lock.releaseRef();
+ }
+ }
+ private void checkIfStandalone() throws CloudRuntimeException {
Review Comment:
[nitpick] Consider renaming this method to be more descriptive of what it
does, such as `checkNoOtherManagementServersRunning()` or
`validateSingleManagementServerMode()`. The current name `checkIfStandalone()`
doesn't clearly indicate that it throws an exception on failure rather than
returning a boolean result.
```suggestion
validateSingleManagementServerMode();
doUpgrades(lock);
} finally {
lock.releaseRef();
}
}
private void validateSingleManagementServerMode() throws
CloudRuntimeException {
```
--
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]
