Extend import/export unmanaged instances to KVM#7712
Closed
itsayushpandey wants to merge 20 commits intoapache:mainfrom
Closed
Extend import/export unmanaged instances to KVM#7712itsayushpandey wants to merge 20 commits intoapache:mainfrom
itsayushpandey wants to merge 20 commits intoapache:mainfrom
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
itsayushpandey
commented
Jul 2, 2023
...va/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetUnmanagedInstancesCommandWrapper.java
Outdated
Show resolved
Hide resolved
Contributor
|
Thanks @itsayushpandey - I'm setting the PR as draft |
Contributor
|
@blueorangutan package |
|
@nvazquez a [SF] 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]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 6380 |
Contributor
|
@itsayushpandey looks like the unit tests on |
itsayushpandey
force-pushed
the
ayushpandey/gsoc2023
branch
from
8f8b214 to
ad0ab57
Compare
DaanHoogland
reviewed
Jul 3, 2023
.../hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java
Outdated
Show resolved
Hide resolved
DaanHoogland
reviewed
Jul 3, 2023
...va/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetUnmanagedInstancesCommandWrapper.java
Show resolved
Hide resolved
Contributor
|
nice to see you are underway with this @itsayushpandey 👍 |
Contributor
|
@blueorangutan package |
|
@nvazquez a [SF] 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]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 6398 |
Author
|
@DaanHoogland I've implemented the changes you've suggested in the latest pull request. |
Author
@nvazquez - can you please help me kick off tests run again? I have fixed the issue in previous commits and can add more tests further after code completion of functionality? |
Author
|
@blueorangutan package |
|
@itsayushpandey a [SF] 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. |
Contributor
|
@blueorangutan package |
|
@nvazquez a [SF] 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. |
Contributor
|
Thanks @itsayushpandey I have tested the changes so far on an env with 2 KVM Alma Linux 8 hosts:
And an empty response on the API: |
Author
|
@nvazquez I have added support to list all domains in a cluster if name is not given. |
Contributor
|
Hi @itsayushpandey I've tried importing an unmanaged instance but I get a different failure this time: (tested on 2xKVM Oracle Linux 8 env) 
Can you please check? |
Author
|
Hey @nvazquez, Can you please invoke packaging and update the directory here (https://download.cloudstack.org/testing/7712/centos7/4.19/) as I'm not able to reproduce the error locally. I can try to reproduce it once I have the updated package and then try to fix it. |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Contributor
|
Hi @itsayushpandey sorry for the delay, I have updated the packages on https://download.cloudstack.org/testing/7712/ to the last packages built on 04/10 (same ones I could hit the issue) |
Author
|
Hi @nvazquez - looking at the log I see the API call: The hostname passed is 'import' (hostname=import), is that expected? I am unable to reproduce on monkeybox env, but I can try to check again if this is something off in UI. Also, are you able to reproduce the bug with API or just UI? |
Contributor
|
@itsayushpandey the import was just a text name for the VM, should be allowed. I kept testing a bit further and discovered two cases:
For case 1: (displayname = hostname = test-vm) For case 2: (displayname=test-vm2) I have tried also through the API but still hitting the same issue: Let me know if you need some help on this if you are not able to reproduce it, I could investigate and push a fix |
Author
|
@nvazquez my understanding is that expected behaviour for KVM is hostname is not same as VM Name. I think that is only for VMWare? Or may be I am misunderstanding the https://cloudstack.apache.org/api/apidocs-4.14/apis/importUnmanagedInstance.html ( |
Contributor
|
@blueorangutan package |
|
@nvazquez 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]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7452 |
Member
|
@nvazquez are you able to proceed further on this? |
|
@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]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7498 |
Contributor
|
Yes @rohityadavcloud I had a sync with @itsayushpandey and I will be checking the remaining issues |
shwstppr
reviewed
Oct 26, 2023
Contributor
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
Looks promising. Added some comments. Would need testing
If there are assumptions and prerequisites for KVM import/unmanage they must be updated in the official docs
Comment on lines
+90
to
+96
| @Parameter(name = ApiConstants.SERVICE_OFFERING_ID, | ||
| type = CommandType.UUID, | ||
| entityType = ServiceOfferingResponse.class, | ||
| required = true, | ||
| description = "the ID of the service offering for the virtual machine") | ||
| private Long serviceOfferingId; | ||
|
|
Contributor
There was a problem hiding this comment.
Is this refactoring needed?
Author
There was a problem hiding this comment.
I thought it was a harmless refactoring as it moves all required params together. I can change back if you think it doesn't add value?
Comment on lines
+528
to
+529
| if (diskIdsWithoutOffering.size() > 1 || rootDisk == null) { | ||
| throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("VM has total %d disks, disk offering mapping not provided for %d disks. Disk IDs that may need a disk offering - %s", disks.size(), diskIdsWithoutOffering.size() - 1, String.join(", ", diskIdsWithoutOffering))); |
Contributor
There was a problem hiding this comment.
In which case rootDisk is null at this point when we have already checked callerDiskIds.size() != disks.size() - 1?
| } else { | ||
| dataDisks.add(disk); | ||
| DiskOffering diskOffering = diskOfferingDao.findById(dataDiskOfferingMap.getOrDefault(disk.getDiskId(), null)); | ||
| if ((disk.getCapacity() == null || disk.getCapacity() <= 0) && diskOffering != null) { |
Contributor
There was a problem hiding this comment.
Why are we not getting capacity from the hypervisor?
| (StringUtils.isEmpty(networkBroadcastUri) || !String.format("pvlan://%d-%s%d", nic.getVlan(), pvLanType, nic.getPvlan()).equals(networkBroadcastUri))) { | ||
| throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("PVLAN of network(ID: %s) %s is found different from the VLAN of nic(ID: %s) pvlan://%d-%s%d during VM import", network.getUuid(), networkBroadcastUri, nic.getNicId(), nic.getVlan(), pvLanType, nic.getPvlan())); | ||
| private void checksOnlyNeededForVmware(UnmanagedInstanceTO.Nic nic, Network network, final Hypervisor.HypervisorType hypervisorType) { | ||
| if (hypervisorType == Hypervisor.HypervisorType.VMware) { |
Contributor
There was a problem hiding this comment.
Why not return early if it is not VMware
if (!Hypervisor.HypervisorType.VMware.equals(hypervisorType)) {
return;
//continue with rest
Question - why VLAN checks are not needed for KVM?
| allDetails.put(VmDetailConstants.NIC_ADAPTER, unmanagedInstance.getNics().get(0).getAdapterType()); | ||
| } | ||
|
|
||
| if (StringUtils.isNotEmpty(unmanagedInstance.getVncPassword())) { |
Contributor
There was a problem hiding this comment.
should there be a hypervisor check?
Author
There was a problem hiding this comment.
I don't think its needed as for missing VNC password, we don't execute it at all and code remains simple?
(My thinking was that hypervisors plugins that need to pass back VNC password will set it in UnmanagedInstanceTO, and if it is passed we process it here without worrying about hypervisor type.)
| throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Unable to retrieve details for unmanaged VM: %s", name)); | ||
| } | ||
|
|
||
| if (template.getName().equals(VM_IMPORT_DEFAULT_TEMPLATE_NAME) && cluster.getHypervisorType().equals(Hypervisor.HypervisorType.KVM)) { |
Contributor
There was a problem hiding this comment.
Question - why import with default template is not allowed for KVM?
| @Override | ||
| public ConfigKey<?>[] getConfigKeys() { | ||
| return new ConfigKey<?>[] { UnmanageVMPreserveNic }; | ||
| return new ConfigKey<?>[]{UnmanageVMPreserveNic}; |
Author
There was a problem hiding this comment.
I think this might be from IntelliJ auto formatting from some rules I had setup. If this is against formatting conventions, I can revert it?
| nullable(String.class), nullable(Hypervisor.HypervisorType.class), nullable(Map.class), nullable(VirtualMachine.PowerState.class))).thenReturn(userVm); | ||
| NetworkVO networkVO = Mockito.mock(NetworkVO.class); | ||
| when(networkVO.getGuestType()).thenReturn(Network.GuestType.L2); | ||
| when(networkVO.getBroadcastUri()).thenReturn(URI.create(String.format("vlan://%d", instanceNic.getVlan()))); |
Author
There was a problem hiding this comment.
This mocking was not needed (tests pass after removal) after refactoring in other PRs (not this PR). So after rebasing, I was getting Mockito failures and so is not needed anymore.
| :key="network.id" | ||
| :label="network.displaytext + (network.broadcasturi ? ' (' + network.broadcasturi + ')' : '')"> | ||
| {{ network.displaytext + (network.broadcasturi ? ' (' + network.broadcasturi + ')' : '') }} | ||
| <div v-if="this.hypervisor === 'KVM'">{{ network.displaytext + ' - ' + (network.id.slice(0,8)) }}</div> |
Contributor
There was a problem hiding this comment.
no idea about these constants. Would be better to make a new method with some doc
Author
There was a problem hiding this comment.
Sure, I can create a separate method for generating this string?
Context: This is the label for network name in selection dropdown. Since displaytext can be non-unique, I added 8 characters of the UUID for differentiating in case of KVM. For VMWare we had broadcast URI from Vlan.
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Contributor
|
@itsayushpandey cc @nvazquez can you please check and address merge conflicts? Will this be ready for testing for 4.19.0? |
Contributor
|
Yes @shwstppr I'll be addressing the issues soon and move it forward for testing along with KVM ingestion |
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds supports to cloudstack for managing and unmanaging VM's in KVM Hypervisor.
This PR addresses #7127
Description
The support is extended through API's and also the UI. The Apache Cloudstack KVM plugin has been extended with new command wrappers for fetching unmanaged VM's xml attribute. The code paths in management server have been cleaned up and refined for managing KVM specific importing commands.
API's involved are :
Testing steps:
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?