-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Introducing the External deployment or provisioning integration #9752
base: main
Are you sure you want to change the base?
Introducing the External deployment or provisioning integration #9752
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9752 +/- ##
============================================
- Coverage 15.81% 15.77% -0.05%
- Complexity 12553 12587 +34
============================================
Files 5629 5638 +9
Lines 492044 493479 +1435
Branches 63478 60299 -3179
============================================
+ Hits 77822 77837 +15
- Misses 405901 407108 +1207
- Partials 8321 8534 +213
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
@blueorangutan package |
@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 11530 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harikrishna-patnala , I noticed I had started reviewing this and abandonned it. I was about half way anf will leave you the comments now. I will revisit after 4.20 is out.
@Parameter(name = ApiConstants.EXTERNAL_PROVISIONER, type = CommandType.STRING, description = "Name of the provisioner for the external host, this is mandatory input in case of hypervisor type external", since = "4.21.0") | ||
private String provisioner; | ||
|
||
@Parameter(name = ApiConstants.EXTERNAL_DETAILS, type = CommandType.MAP, description = "Details in key/value pairs using format externaldetails[i].keyname=keyvalue. Example: externaldetails[0].endpoint.url=urlvalue", since = "4.21.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these externaldetails
and not just details
@@ -67,6 +70,9 @@ public class UpdateHostCmd extends BaseCmd { | |||
@Parameter(name = ApiConstants.ANNOTATION, type = CommandType.STRING, description = "Add an annotation to this host", since = "4.11", authorized = {RoleType.Admin}) | |||
private String annotation; | |||
|
|||
@Parameter(name = ApiConstants.EXTERNAL_DETAILS, type = CommandType.MAP, description = "Details in key/value pairs using format externaldetails[i].keyname=keyvalue. Example: externaldetails[0].endpoint.url=urlvalue", since = "4.21.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
details
instead of external...
@@ -251,6 +252,8 @@ public class CreateServiceOfferingCmd extends BaseCmd { | |||
since="4.20") | |||
private Boolean purgeResources; | |||
|
|||
@Parameter(name = ApiConstants.EXTERNAL_DETAILS, type = CommandType.MAP, description = "Details in key/value pairs using format externaldetails[i].keyname=keyvalue. Example: externaldetails[0].endpoint.url=urlvalue", since = "4.21.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need those extra external-details
? aren't these just details
?
@@ -359,9 +362,21 @@ public Map<String, String> getDetails() { | |||
} | |||
} | |||
} | |||
|
|||
detailsMap.putAll(getExternalDetails()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see? these are just details
.
public class RebootCommand extends Command { | ||
String vmName; | ||
VirtualMachineTO vm; | ||
private Map<String, String> _details; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private Map<String, String> _details; | |
private Map<String, String> details; |
} | ||
|
||
public void setDetails(Map<String, String> details) { | ||
_details = details; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_details = details; | |
this.details = details; |
} | ||
|
||
public Map<String, String> getDetails() { | ||
return _details; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return _details; | |
return details; |
} else if (template.getFormat() == ImageFormat.BAREMETAL) { | ||
logger.debug("%s has format [{}]. Skipping ROOT volume [{}] allocation.", template.toString(), ImageFormat.BAREMETAL, rootVolumeName); | ||
} else if (template.getFormat() == ImageFormat.BAREMETAL || template.getFormat() == ImageFormat.EXTERNAL) { | ||
logger.debug(String.format("%s has format [%s]. Skipping ROOT volume [%s] allocation.", template.toString(), template.getFormat(), rootVolumeName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.debug(String.format("%s has format [%s]. Skipping ROOT volume [%s] allocation.", template.toString(), template.getFormat(), rootVolumeName)); | |
logger.debug("{} has format [{}]. Skipping ROOT volume [{}] allocation.", template.toString(), template.getFormat(), rootVolumeName); |
if (HypervisorType.External.equals(vm.getHypervisorType())) { | ||
Long hostID = profile.getHostId(); | ||
if (hostID != null) { | ||
HostVO host = _hostDao.findById(hostID); | ||
HashMap<String, String> accessDetails = new HashMap<>(); | ||
loadExternalHostAccessDetails(host, accessDetails); | ||
loadExternalInstanceDetails(vm.getId(), accessDetails); | ||
|
||
stpCmd.setDetails(accessDetails); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be a method too?
if (HypervisorType.External.equals(vm.getHypervisorType())) { | ||
Long hostID = profile.getHostId(); | ||
HostVO host = _hostDao.findById(hostID); | ||
|
||
HashMap<String, String> accessDetails = new HashMap<>(); | ||
loadExternalHostAccessDetails(host, accessDetails); | ||
loadExternalInstanceDetails(vm.getId(), accessDetails); | ||
|
||
stop.setDetails(accessDetails); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another method? maybe even unify with the one at line 1910-1920.
@blueorangutan package |
@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11592 |
@@ -323,6 +326,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac | |||
@Inject | |||
private HostDao _hostDao; | |||
@Inject | |||
private HostDetailsDao _hostDetailsDao; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private HostDetailsDao _hostDetailsDao; | |
private HostDetailsDao hostDetailsDao; |
@@ -405,6 +410,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac | |||
@Inject | |||
private DomainDao domainDao; | |||
@Inject | |||
public NetworkService _networkService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public NetworkService _networkService; | |
public NetworkService networkService; |
|
||
NicTO[] nics = vmTO.getNics(); | ||
for (NicTO nic : nics) { | ||
if (nic.isDefaultNic()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could invert the condition here to decrease indentation.
@@ -285,6 +295,8 @@ public class NetworkOrchestrator extends ManagerBase implements NetworkOrchestra | |||
@Inject | |||
UserVmDao _userVmDao; | |||
@Inject | |||
UserVmDetailsDao _userVmDetailsDao; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UserVmDetailsDao _userVmDetailsDao; | |
UserVmDetailsDao userVmDetailsDao; |
@@ -358,6 +370,8 @@ public class NetworkOrchestrator extends ManagerBase implements NetworkOrchestra | |||
private ASNumberDao asNumberDao; | |||
@Inject | |||
private BGPService bgpService; | |||
@Inject | |||
private HypervisorGuruManager _hvGuruMgr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private HypervisorGuruManager _hvGuruMgr; | |
private HypervisorGuruManager hvGuruMgr; |
private AgentManager _agentMgr; | ||
@Inject | ||
private UserVmDetailsDao userVmDetailsDao; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these necessary? They are not used in this class.
if (dest.getCluster().getHypervisorType() == HypervisorType.Ovm) { | ||
hypervisors.add(getClusterToStartDomainRouterForOvm(dest.getCluster().getPodId())); | ||
HypervisorType destClusterHypType = dest.getCluster().getHypervisorType(); | ||
Set<HypervisorType> hypervisorsTypesToCheck = Set.of(HypervisorType.Ovm, HypervisorType.BareMetal, HypervisorType.External); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could move this to this class variable and use it in line 659
as well.
@@ -40,6 +41,8 @@ public abstract class DiscovererBase extends AdapterBase implements Discoverer { | |||
@Inject | |||
protected ClusterDao _clusterDao; | |||
@Inject | |||
protected ClusterDetailsDao _clusterDetailsDao; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove the _
?
@@ -2048,6 +2052,7 @@ | |||
"label.servicelist": "Services", | |||
"label.serviceofferingid": "Compute offering", | |||
"label.serviceofferingname": "Compute offering", | |||
"label.serviceofferingdetails": "Details", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already the label label.details
, couldn't you use it?
@@ -240,6 +238,7 @@ export default { | |||
}, | |||
onAddInputChange (val, obj) { | |||
this.error = false | |||
console.log(val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover from debugging?
console.log(val) |
@DaanHoogland I see many other custom detail parameters in a few other APIs, such as vnfdetails, domaindetails, accountdetails, and a few more. In this case, I would like to keep this as a separate parameter because these are the only details that are important if the hypervisor type is External. The other parameters are just meant to fit into CloudStack's workflow. The core purpose of this feature is to send these external details to the external provisioner. |
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Description
Apache CloudStack already supports a wide range of hypervisors, including KVM, VMware, and Xen, to provision and manage instances. We are now introducing a new feature or framework that allows seamless integration of "external" provisioning systems into CloudStack. For example, this enables the integration of any baremetal provisioning system, platforms like Proxmox, or even the addition of new hypervisor support through the use of simple scripts.
A design document is made with more details at https://cwiki.apache.org/confluence/display/CLOUDSTACK/External+Deployment+Integration+in+CloudStack
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale