Uh oh!
There was an error while loading. Please reload this page.
VM deployment heuristsics - #29
Conversation
201786b to
e1336ffComparedhlaluku
commented
Feb 15, 2019
@borisstoyanov I am working on the Marvin tests for this PR and would like some suggestions around negative test cases. Thanks in advance /CC @DaanHoogland |
DaanHoogland
left a comment
There was a problem hiding this comment.
code looks good, i have some functional doubt, though.
Uh oh!
There was an error while loading. Please reload this page.
| Account account = CallContext.current().getCallingAccount(); | ||
| boolean isRootAdmin = _accountService.isRootAdmin(account.getId()); | ||
| Pod destinationPod = getDestinationPod(podId, isRootAdmin); |
There was a problem hiding this comment.
what if,
the user passed a pod, ...
There was a problem hiding this comment.
pod will be chosen for the deployment if cluster and host have not been specified.
| boolean isRootAdmin = _accountService.isRootAdmin(account.getId()); | ||
| Pod destinationPod = getDestinationPod(podId, isRootAdmin); | ||
| Cluster destinationCluster = getDestinationCluster(clusterId, isRootAdmin); |
There was a problem hiding this comment.
... and then passes a cluster that is not in the passed pod, ...
| Pod destinationPod = getDestinationPod(podId, isRootAdmin); | ||
| Cluster destinationCluster = getDestinationCluster(clusterId, isRootAdmin); | ||
| Host destinationHost = getDestinationHost(hostId, isRootAdmin); |
There was a problem hiding this comment.
and next passes a host that is not in the passed cluster?
There was a problem hiding this comment.
I have simplified this to by using an if-else if block that checks from the lowest unit (host), followed cluster if host is not specified, and lastly pod.if (host != null){ planner = host planner } else if(cluster != null) { planner = cluster planner }else if(pod != null) { planner = pod planner }
Removed the validation that checks the parent-child relations between pod, cluster and host
| return vmParamPair; | ||
| } | ||
| private void validateDeploymentDestination(Pod destinationPod, Cluster destinationCluster, Host destinationHost) { |
There was a problem hiding this comment.
I would rather have that the user would be able to pass either a pod, or a cluster, or a host. the smaller implies the bigger and makes it redundant as parameter.
There was a problem hiding this comment.
This validation is not necessary since we take the lowest unit supplied and use that for the deployment.
Check has been removed
| Cluster destinationCluster = null; | ||
| if (clusterId != null) { | ||
| if (!isRootAdmin) { |
There was a problem hiding this comment.
so a user can not specify a cluster they have acces to?
There was a problem hiding this comment.
This feature is only available to Admin users. (Root and Domain)
dhlaluku
commented
Feb 20, 2019
@DaanHoogland thanks for the review, I will look your comments and address as requested. |
87d8ca4 to
3f42254Compare2f765d1 to
24194c2Compare234bdaa to
989d51eCompare989d51e to
e3886d9Compareanuragaw
commented
May 9, 2019
Fixed some bugs that were found and some that I observed in code. Also rebased against latest master. |
Currently an admin can choose which host a VM is to be started on. They should be able to 'override' the allocation algorthm to a greater or lesser extent at will, and be able to choose the pod, cluster or host that they wish a new VM to be deployed in. DeployVirtualMachine API has been extended with additional, optional parameters podid and clusterid that will be passed to and used in the deployment planner, when selecting a viable host. If the user supplies a pod, a suitable host in the given pod will be selected. If the user supplies a cluster, a suitable host in the given cluster will be selected. Based on the parameter supplied and on passing validation, the VM will then be deployed on the selected host, cluster or pod.
e3886d9 to
db8172bCompareanuragaw
commented
Jul 2, 2019
Had lost track of this last thing on this PR. Rebased against master. |
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Description
VM deployment;
Allow users to choose a pod, cluster, and host when deploying a VM instance
FR Doc: https://shapeblue.sharepoint.com/:w:/r/Shared%20Documents/Development/Roadmap/FR%20Documents/VM%20Deployment%20Planner/DeploymentPlannerFR.docx?d=w0079dc479ca5446985cbb9226a14a08b&csf=1&e=EsWcnl
Types of changes
Screenshots (if appropriate):
How Has This Been Tested?