Skip to content

Extensions Framework & Orchestrate Anything #9752

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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

harikrishna-patnala
Copy link
Contributor

@harikrishna-patnala harikrishna-patnala commented Oct 1, 2024

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

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 1.12430% with 1583 lines in your changes missing coverage. Please review.

Project coverage is 16.37%. Comparing base (2df1ac5) to head (755a5a7).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
...r/simpleprovisioner/SimpleExternalProvisioner.java 0.00% 218 Missing ⚠️
.../cloud/agent/manager/ExternalAgentManagerImpl.java 0.00% 187 Missing ⚠️
...m/cloud/agent/manager/ExternalTemplateAdapter.java 0.00% 105 Missing ⚠️
...rvisor/external/resource/ExternalResourceBase.java 0.00% 93 Missing ⚠️
.../external/discoverer/ExternalServerDiscoverer.java 0.00% 78 Missing ⚠️
...n/java/com/cloud/vm/VirtualMachineManagerImpl.java 0.00% 71 Missing and 1 partial ⚠️
...tack/engine/orchestration/NetworkOrchestrator.java 4.47% 63 Missing and 1 partial ⚠️
...n/java/com/cloud/resource/ResourceManagerImpl.java 0.00% 60 Missing ⚠️
...apache/cloudstack/guru/ExternalHypervisorGuru.java 0.00% 52 Missing ⚠️
...com/cloud/agent/manager/ExternalServerPlanner.java 0.00% 47 Missing ⚠️
... and 60 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9752      +/-   ##
============================================
- Coverage     16.42%   16.37%   -0.06%     
- Complexity    13549    13553       +4     
============================================
  Files          5673     5704      +31     
  Lines        500080   501770    +1690     
  Branches      60506    60708     +202     
============================================
+ Hits          82153    82170      +17     
- Misses       408794   410453    +1659     
- Partials       9133     9147      +14     
Flag Coverage Δ
uitests 3.98% <ø> (-0.02%) ⬇️
unittests 17.24% <1.12%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@apache apache deleted a comment from blueorangutan Oct 1, 2024
@apache apache deleted a comment from blueorangutan Oct 1, 2024
Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@harikrishna-patnala
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@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.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 11530

Copy link
Contributor

@DaanHoogland DaanHoogland left a 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")
Copy link
Contributor

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")
Copy link
Contributor

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")
Copy link
Contributor

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());
Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private Map<String, String> _details;
private Map<String, String> details;

}

public void setDetails(Map<String, String> details) {
_details = details;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_details = details;
this.details = details;

}

public Map<String, String> getDetails() {
return _details;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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);

Comment on lines +1910 to +1931
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);
}
}
Copy link
Contributor

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?

Comment on lines +2249 to +2269
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);
}
Copy link
Contributor

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.

@sureshanaparti sureshanaparti self-assigned this Nov 13, 2024
@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@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.

@blueorangutan
Copy link

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public NetworkService _networkService;
public NetworkService networkService;


NicTO[] nics = vmTO.getNics();
for (NicTO nic : nics) {
if (nic.isDefaultNic()) {
Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private HypervisorGuruManager _hvGuruMgr;
private HypervisorGuruManager hvGuruMgr;

Comment on lines +127 to +129
private AgentManager _agentMgr;
@Inject
private UserVmDetailsDao userVmDetailsDao;
Copy link
Contributor

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);
Copy link
Contributor

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;
Copy link
Contributor

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",
Copy link
Contributor

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover from debugging?

Suggested change
console.log(val)

@harikrishna-patnala
Copy link
Contributor Author

@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.

Copy link

github-actions bot commented Dec 3, 2024

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Copy link

github-actions bot commented Dec 3, 2024

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@harikrishna-patnala harikrishna-patnala force-pushed the ExternalDeploymentIntegration branch from b03a8d8 to 755a5a7 Compare April 29, 2025 23:05
@harikrishna-patnala
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@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.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13218

Copy link

github-actions bot commented May 2, 2025

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants