Copilot commented on code in PR #13202:
URL: https://github.com/apache/cloudstack/pull/13202#discussion_r3281066425
##########
server/src/main/java/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java:
##########
@@ -988,41 +988,43 @@ public boolean stopProxy(long proxyVmId) {
@Override
@DB
- public void setManagementState(final ConsoleProxyManagementState state) {
+ public void setManagementState(final ConsoleProxyManagementState newState)
{
try {
- final ConsoleProxyManagementState lastState = getManagementState();
- if (lastState == null) {
+ final ConsoleProxyManagementState currentState =
getManagementState();
+ if (currentState == null) {
return;
}
- if (lastState != state) {
+ if (currentState != newState) {
+ logger.debug("Updating console proxy management state config
to new state: {}, it's current state is {} and last management state config to
{}.", newState, currentState, currentState);
Transaction.execute(new TransactionCallbackNoReturn() {
@Override
public void doInTransactionWithoutResult(TransactionStatus
status) {
-
configurationDao.update(ConsoleProxyManagementLastState.key(),
ConsoleProxyManagementLastState.category(), lastState.toString());
-
configurationDao.update(ConsoleProxyServiceManagementState.key(),
ConsoleProxyServiceManagementState.category(), state.toString());
+
configurationDao.update(ConsoleProxyServiceManagementLastState.key(),
ConsoleProxyServiceManagementLastState.category(), currentState.toString());
+
configurationDao.update(ConsoleProxyServiceManagementState.key(),
ConsoleProxyServiceManagementState.category(), newState.toString());
}
});
+ } else {
+ logger.debug("Console proxy management state is already set to
{}, no need to update.", newState);
}
} catch (Exception e) {
- logger.error(String.format("Unable to set console proxy management
state to [%s] due to [%s].", state, e.getMessage()), e);
+ logger.error("Unable to update console proxy management state to
[{}] due to [{}].", newState, e.getMessage(), e);
}
}
@Override
public ConsoleProxyManagementState getManagementState() {
- String configKey = ConsoleProxyServiceManagementState.key();
- String value = ConsoleProxyServiceManagementState.value();
-
- if (value != null) {
- ConsoleProxyManagementState state =
ConsoleProxyManagementState.valueOf(value);
+ String stateConfigKey = ConsoleProxyServiceManagementState.key();
+ String stateConfigValue = ConsoleProxyServiceManagementState.value();
+ if (stateConfigValue != null) {
+ ConsoleProxyManagementState state =
ConsoleProxyManagementState.valueOf(stateConfigValue);
if (state != null) {
return state;
}
}
- logger.error(String.format("Value [%s] set in global configuration
[%s] is not a valid console proxy management state.", value, configKey));
+ logger.error("Console proxy management state value is null in the
global configuration [{}].", stateConfigKey);
return null;
Review Comment:
`ConsoleProxyManagementState.valueOf(stateConfigValue)` will throw
`IllegalArgumentException` if the config contains an unexpected value
(including empty string). Since `getManagementState()` is called from periodic
code paths (e.g. capacity scanning) without a surrounding try/catch, a bad
config can break the manager thread. Consider catching
`IllegalArgumentException`, logging the invalid value (not only the key), and
returning a safe default/null without throwing. Also, the current error log
always claims the value is null, which is misleading for invalid non-null
values.
##########
server/src/main/java/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java:
##########
@@ -1037,26 +1039,26 @@ public void resumeLastManagementState() {
}
if (lastState != state) {
+ logger.debug("Resuming console proxy management state to last
state {}, current state is {}.", lastState, state);
configurationDao.update(ConsoleProxyServiceManagementState.key(),
ConsoleProxyServiceManagementState.category(), lastState.toString());
}
} catch (Exception e) {
- logger.error(String.format("Unable to resume last management state
due to [%s].", e.getMessage()), e);
+ logger.error("Unable to resume last management state due to
[{}].", e.getMessage(), e);
}
}
private ConsoleProxyManagementState getLastManagementState() {
- String configKey = ConsoleProxyManagementLastState.key();
- String value = ConsoleProxyManagementLastState.value();
+ String lastStateConfigKey =
ConsoleProxyServiceManagementLastState.key();
+ String lastStateConfigValue =
ConsoleProxyServiceManagementLastState.value();
- if (value != null) {
- ConsoleProxyManagementState state =
ConsoleProxyManagementState.valueOf(value);
-
- if (state != null) {
- return state;
+ if (lastStateConfigValue != null) {
+ ConsoleProxyManagementState lastState =
ConsoleProxyManagementState.valueOf(lastStateConfigValue);
+ if (lastState != null) {
+ return lastState;
}
}
- logger.error(String.format("Value [%s] set in global configuration
[%s] is not a valid console proxy management state.", value, configKey));
+ logger.error("Console proxy last management state value is null in the
global configuration [{}].", lastStateConfigKey);
return null;
Review Comment:
`ConsoleProxyManagementState.valueOf(lastStateConfigValue)` can throw
`IllegalArgumentException` when the stored "last" state is invalid (or empty).
Even though some callers wrap in a broad try/catch, others may not, and this
method currently logs only a "value is null" message which is incorrect for
invalid non-null values. Suggest catching `IllegalArgumentException`, logging
the invalid value + key, and returning null/default without throwing.
--
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]
