Skip to content

Address reviewers from abandoned PR #1597. #3091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
GabrielBrascher wants to merge 1 commit into from
Closed

Address reviewers from abandoned PR #1597. #3091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 22 additions & 23 deletions server/src/main/java/com/cloud/deploy/FirstFitPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ public class FirstFitPlanner extends AdapterBase implements DeploymentClusterPla
protected String globalDeploymentPlanner = "FirstFitPlanner";
protected String[] implicitHostTags;

private final static String DEPLOY_VM = "deployvm";

@Override
public List<Long> orderClusters(VirtualMachineProfile vmProfile, DeploymentPlan plan, ExcludeList avoid) throws InsufficientServerCapacityException {
VirtualMachine vm = vmProfile.getVirtualMachine();
Expand Down Expand Up @@ -197,25 +199,24 @@ public List<Long> orderClusters(VirtualMachineProfile vmProfile, DeploymentPlan
return clusterList;
}

private void reorderClustersBasedOnImplicitTags(List<Long> clusterList, int requiredCpu, long requiredRam) {
final HashMap<Long, Long> UniqueTagsInClusterMap = new HashMap<Long, Long>();
Long uniqueTags;
for (Long clusterId : clusterList) {
uniqueTags = (long) 0;
public void reorderClustersBasedOnImplicitTags(List<Long> clusterList, int requiredCpu, long requiredRam) {
Map<Long, Long> UniqueTagsInClusterMap = new HashMap<>();
for (Long clusterId : clusterList) {
long uniqueTags = 0l;
List<Long> hostList = capacityDao.listHostsWithEnoughCapacity(requiredCpu, requiredRam, clusterId, Host.Type.Routing.toString());
if (!hostList.isEmpty() && implicitHostTags.length > 0) {
uniqueTags = new Long(hostTagsDao.getDistinctImplicitHostTags(hostList, implicitHostTags).size());
}
UniqueTagsInClusterMap.put(clusterId, uniqueTags);
uniqueTags = hostTagsDao.getDistinctImplicitHostTags(hostList, implicitHostTags).size();
}
Collections.sort(clusterList, new Comparator<Long>() {
@Override
public int compare(Long o1, Long o2) {
Long t1 = UniqueTagsInClusterMap.get(o1);
Long t2 = UniqueTagsInClusterMap.get(o2);
return t1.compareTo(t2);
}
});
UniqueTagsInClusterMap.put(clusterId, uniqueTags);
}
Collections.sort(clusterList, new Comparator<Long>() {
@Override
public int compare(Long o1, Long o2) {
Long t1 = UniqueTagsInClusterMap.get(o1);
Long t2 = UniqueTagsInClusterMap.get(o2);
return t1.compareTo(t2);
}
});
}

private List<Long> scanPodsForDestination(VirtualMachineProfile vmProfile, DeploymentPlan plan, ExcludeList avoid) {
Expand Down Expand Up @@ -304,10 +305,6 @@ private List<Short> getCapacitiesForCheckingThreshold() {

/**
* This method should remove the clusters crossing capacity threshold to avoid further vm allocation on it.
* @param clusterListForVmAllocation
* @param avoid
* @param vmProfile
* @param plan
*/
protected void removeClustersCrossingThreshold(List<Long> clusterListForVmAllocation, ExcludeList avoid,
VirtualMachineProfile vmProfile, DeploymentPlan plan) {
Expand All @@ -318,7 +315,7 @@ protected void removeClustersCrossingThreshold(List<Long> clusterListForVmAlloca
VirtualMachine vm = vmProfile.getVirtualMachine();
Map<String, String> details = vmDetailsDao.listDetailsKeyPairs(vm.getId());
Boolean isThresholdEnabled = ClusterThresholdEnabled.value();
if (!(isThresholdEnabled || (details != null && details.containsKey("deployvm")))) {
if (!isThresholdEnabled && !details.containsKey(DEPLOY_VM)) {
return;
}

Expand Down Expand Up @@ -362,7 +359,6 @@ private List<Long> scanClustersForDestinationInZoneOrPod(long id, boolean isZone

VirtualMachine vm = vmProfile.getVirtualMachine();
ServiceOffering offering = vmProfile.getServiceOffering();
DataCenter dc = dcDao.findById(vm.getDataCenterId());
int requiredCpu = offering.getCpu() * offering.getSpeed();
long requiredRam = offering.getRamSize() * 1024L * 1024L;

Expand Down Expand Up @@ -579,7 +575,6 @@ public boolean configure(String name, Map<String, Object> params) throws Configu

@Override
public DeployDestination plan(VirtualMachineProfile vm, DeploymentPlan plan, ExcludeList avoid) throws InsufficientServerCapacityException {
// TODO Auto-generated method stub
return null;
}

Expand All @@ -597,4 +592,8 @@ public String getConfigComponentName() {
public ConfigKey<?>[] getConfigKeys() {
return new ConfigKey<?>[] {ClusterCPUCapacityDisableThreshold, ClusterMemoryCapacityDisableThreshold, ClusterThresholdEnabled};
}

public void setImplicitHostTags(String[] implicitHostTags) {
this.implicitHostTags = implicitHostTags;
}
}
2 changes: 0 additions & 2 deletions server/src/test/java/com/cloud/vm/FirstFitPlannerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,6 @@ public void checkClusterListBasedOnHostTag() throws InsufficientServerCapacityEx
}

private List<Long> initializeForClusterListBasedOnHostTag(ServiceOffering offering) {


when(offering.getHostTag()).thenReturn("hosttag1");
initializeForClusterThresholdDisabled();
List<Long> matchingClusters = new ArrayList<>();
Expand Down
86 changes: 52 additions & 34 deletions ...sole-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,34 @@

public class ConsoleProxyHttpHandlerHelper {
private static final Logger s_logger = Logger.getLogger(ConsoleProxyHttpHandlerHelper.class);
private static final String AND = "&";
private static final String EQUALS = "=";
private static final String HOST = "host";
private static final String PORT = "port";
private static final String SID = "sid";
private static final String TAG = "tag";
private static final String CONSOLE_URL = "consoleurl";
private static final String SESSION_REF = "sessionref";
private static final String TICKET = "ticket";
private static final String LOCALE = "locale";
private static final String HYPERV_HOST = "hypervHost";
private static final String USERNAME = "username";
private static final String PASSWORD = "password";
private static final String TOKEN = "token";

public static Map<String, String> getQueryMap(String query) {
String[] params = query.split("&");
Copy link
Member

@rohityadavcloud rohityadavcloud Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to parse URL (possibly a get request) to get the key/value map of params?

Copy link
Member Author

@GabrielBrascher GabrielBrascher Dec 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is, thanks for pointing that!
I will ping you when update it (also extracting some lines to methods, bringing some test cases, and documenting).

String[] params = query.split(AND);
Map<String, String> map = new HashMap<String, String>();
Copy link
Contributor

@dhlaluku dhlaluku Jan 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable should be named "tokenMap" or something similar to show that it should only store a token key-value item.

This will save computation by getting rid of the guardUserInput(map) call in Line #59

for (String param : params) {
String[] paramTokens = param.split("=");
String[] paramTokens = param.split(EQUALS);
if (paramTokens != null && paramTokens.length == 2) {
Copy link
Contributor

@dhlaluku dhlaluku Jan 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GabrielBrascher can you change this line to this "If (ArrayUtils.isNotEmpty(paramTokens) && paramTokens.length == 2)"?

String name = param.split("=")[0];
String value = param.split("=")[1];
String name = paramTokens[0];
String value = paramTokens[1];
map.put(name, value);
Copy link
Contributor

@dhlaluku dhlaluku Jan 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think another "if n(name.equalsIgnoreCase(TOKEN)" is needed here considering that the map should only contain a "TOKEN" key. and then remove "guardUserInput(map)" call in line 59

} else if (paramTokens.length == 3) {
// very ugly, added for Xen tunneling url
String name = paramTokens[0];
String value = paramTokens[1] + "=" + paramTokens[2];
String value = paramTokens[1] + EQUALS + paramTokens[2];
map.put(name, value);
} else {
if (s_logger.isDebugEnabled())
Expand All @@ -46,53 +60,53 @@ public static Map<String, String> getQueryMap(String query) {

// This is a ugly solution for now. We will do encryption/decryption translation
// here to make it transparent to rest of the code.
if (map.get("token") != null) {
if (map.get(TOKEN) != null) {
ConsoleProxyPasswordBasedEncryptor encryptor = new ConsoleProxyPasswordBasedEncryptor(ConsoleProxy.getEncryptorPassword());

ConsoleProxyClientParam param = encryptor.decryptObject(ConsoleProxyClientParam.class, map.get("token"));
ConsoleProxyClientParam param = encryptor.decryptObject(ConsoleProxyClientParam.class, map.get(TOKEN));

// make sure we get information from token only
guardUserInput(map);
if (param != null) {
if (param.getClientHostAddress() != null) {
s_logger.debug("decode token. host: " + param.getClientHostAddress());
map.put("host", param.getClientHostAddress());
map.put(HOST, param.getClientHostAddress());
} else {
s_logger.error("decode token. host info is not found!");
logMessageIfCannotFindClientParam(HOST);
}
if (param.getClientHostPort() != 0) {
s_logger.debug("decode token. port: " + param.getClientHostPort());
map.put("port", String.valueOf(param.getClientHostPort()));
map.put(PORT, String.valueOf(param.getClientHostPort()));
} else {
s_logger.error("decode token. port info is not found!");
logMessageIfCannotFindClientParam(PORT);
}
if (param.getClientTag() != null) {
s_logger.debug("decode token. tag: " + param.getClientTag());
map.put("tag", param.getClientTag());
map.put(TAG, param.getClientTag());
} else {
s_logger.error("decode token. tag info is not found!");
logMessageIfCannotFindClientParam(TAG);
}
if (param.getClientHostPassword() != null) {
map.put("sid", param.getClientHostPassword());
map.put(SID, param.getClientHostPassword());
} else {
s_logger.error("decode token. sid info is not found!");
logMessageIfCannotFindClientParam(SID);
}
if (param.getClientTunnelUrl() != null)
map.put("consoleurl", param.getClientTunnelUrl());
map.put(CONSOLE_URL, param.getClientTunnelUrl());
if (param.getClientTunnelSession() != null)
map.put("sessionref", param.getClientTunnelSession());
map.put(SESSION_REF, param.getClientTunnelSession());
if (param.getTicket() != null)
map.put("ticket", param.getTicket());
map.put(TICKET, param.getTicket());
if (param.getLocale() != null)
map.put("locale", param.getLocale());
map.put(LOCALE, param.getLocale());
if (param.getHypervHost() != null)
map.put("hypervHost", param.getHypervHost());
map.put(HYPERV_HOST, param.getHypervHost());
if (param.getUsername() != null)
map.put("username", param.getUsername());
map.put(USERNAME, param.getUsername());
if (param.getPassword() != null)
map.put("password", param.getPassword());
map.put(PASSWORD, param.getPassword());
} else {
s_logger.error("Unable to decode token");
s_logger.error("Unable to decode token due to null console proxy client param");
}
} else {
Copy link
Contributor

@dhlaluku dhlaluku Jan 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else statement seems redundant and can go if the tokenMap only contains 1 TOKEN item

// we no longer accept information from parameter other than token
Expand All @@ -102,17 +116,21 @@ public static Map<String, String> getQueryMap(String query) {
return map;
}

private static void logMessageIfCannotFindClientParam(String param) {
s_logger.error("decode token. " + param + " info is not found!");
}

private static void guardUserInput(Map<String, String> map) {
map.remove("host");
map.remove("port");
map.remove("tag");
map.remove("sid");
map.remove("consoleurl");
map.remove("sessionref");
map.remove("ticket");
map.remove("locale");
map.remove("hypervHost");
map.remove("username");
map.remove("password");
map.remove(HOST);
map.remove(PORT);
map.remove(TAG);
map.remove(SID);
map.remove(CONSOLE_URL);
map.remove(SESSION_REF);
map.remove(TICKET);
map.remove(LOCALE);
map.remove(HYPERV_HOST);
map.remove(USERNAME);
map.remove(PASSWORD);
}
}