DaanHoogland commented on code in PR #11415:
URL: https://github.com/apache/cloudstack/pull/11415#discussion_r2293674833
##########
server/src/main/java/com/cloud/consoleproxy/AgentHookBase.java:
##########
@@ -213,10 +212,14 @@ public void startAgentHttpHandlerInVM(StartupProxyCommand
startupCmd) {
byte[] ksBits = null;
- String consoleProxyUrlDomain =
_configDao.getValue(Config.ConsoleProxyUrlDomain.key());
- String consoleProxySslEnabled =
_configDao.getValue(ConsoleProxyManager.ConsoleProxySslEnabled.key());
- if (!StringUtils.isEmpty(consoleProxyUrlDomain) &&
!StringUtils.isEmpty(consoleProxySslEnabled)
- && consoleProxySslEnabled.equalsIgnoreCase("true")) {
+ HostVO consoleProxyHost = findConsoleProxyHost(startupCmd);
+
+ assert (consoleProxyHost != null);
Review Comment:
when are we running with asserts? maybe we can log a fatal error if
`consoleProxyHost` is null?
##########
framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java:
##########
@@ -229,6 +229,10 @@ public ConfigKey(String category, Class<T> type, String
name, String defaultValu
this(type, name, category, defaultValue, description, isDynamic,
Scope.Global, null);
}
+ public ConfigKey(String category, Class<T> type, String name, String
defaultValue, String description, Scope scope, boolean isDynamic) {
+ this(type, name, category, defaultValue, description, isDynamic,
scope, null);
+ }
Review Comment:
is there really need for a new constructor? We can call `public
ConfigKey(Class<T> type, String name, String category, String defaultValue,
String description, boolean isDynamic, Scope scope, T multiplier)` with `null`
as last parameter, can we?
This class now has 15 constructors, but only one that does any real
constructing. (no real -1 but not really a fan either)
##########
server/src/main/java/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java:
##########
@@ -1661,4 +1606,54 @@ protected void updateConsoleProxyStatus(String
statusInfo, Long proxyVmId) {
consoleProxyDao.update(proxyVmId, count, DateUtil.currentGMTTime(),
details);
}
+
+ private boolean isSslEnabled(Long dataCenterId) {
+ boolean sslEnabled = ConsoleProxySslEnabled.valueIn(dataCenterId);
+ String consoleProxyUrlDomain =
ConsoleProxyUrlDomain.valueIn(dataCenterId);
+ if( sslEnabled && (consoleProxyUrlDomain == null ||
consoleProxyUrlDomain.isEmpty())) {
+ logger.warn("Empty console proxy domain, explicitly disabling
SSL");
+ sslEnabled = false;
+ }
+ return sslEnabled;
+ }
+
+ private Integer getStandbyCapacity(Long datacenterId) {
+ return
Integer.parseInt(ConsoleProxyCapacityStandby.valueIn(datacenterId));
+ }
+
+ private ServiceOfferingVO getConsoleProxyServiceOffering(Long
datacenterId) throws ConfigurationException {
+ String configKey = ConsoleProxyServiceOffering.key();
+ String cpvmSrvcOffIdStr =
ConsoleProxyServiceOffering.valueIn(datacenterId);
+ String warningMessage = String.format("Unable to find a service
offering by the UUID or ID for console proxy VM with the value [%s] set in the
configuration [%s]", cpvmSrvcOffIdStr, configKey);
+ ServiceOfferingVO serviceOfferingVO = null;
+ if (cpvmSrvcOffIdStr != null) {
+ serviceOfferingVO =
serviceOfferingDao.findByUuid(cpvmSrvcOffIdStr);
+ if (serviceOfferingVO == null) {
+ try {
+ logger.debug(warningMessage);
+ serviceOfferingVO =
serviceOfferingDao.findById(Long.parseLong(cpvmSrvcOffIdStr));
+ } catch (NumberFormatException ex) {
+ logger.warn(String.format("Unable to find a service
offering by the ID for console proxy VM with the value [%s] set in the
configuration [%s]. The value is not a valid integer number. Error: [%s].",
cpvmSrvcOffIdStr, configKey, ex.getMessage()), ex);
+ }
+ }
+ if (serviceOfferingVO == null) {
+ logger.warn(warningMessage);
+ }
+ }
+
+ if (serviceOfferingVO == null || !serviceOfferingVO.isSystemUse()) {
+ int ramSize =
NumbersUtil.parseInt(configurationDao.getValue("console.ram.size"),
DEFAULT_PROXY_VM_RAMSIZE);
+ int cpuFreq =
NumbersUtil.parseInt(configurationDao.getValue("console.cpu.mhz"),
DEFAULT_PROXY_VM_CPUMHZ);
+ List<ServiceOfferingVO> offerings =
serviceOfferingDao.createSystemServiceOfferings("System Offering For Console
Proxy",
+ ServiceOffering.consoleProxyDefaultOffUniqueName, 1,
ramSize, cpuFreq, 0, 0, false, null,
+ Storage.ProvisioningType.THIN, true, null, true,
VirtualMachine.Type.ConsoleProxy, true);
+
+ if (offerings == null || offerings.size() < 2) {
+ String msg = "Data integrity problem : System Offering For
Console Proxy has been removed?";
+ logger.error(msg);
+ throw new ConfigurationException(msg);
+ }
+ }
Review Comment:
can you extract these two blocks in separate methods , like
`getCpvmOfferingByUuid(..)` and `getCpvmOfferingBySpecification(..)` for
instance? (names may change during discussion/review ;)
--
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]
