Skip to content

[Veeam] restored VMs without NICs #6282

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

Merged
DaanHoogland merged 14 commits into from
Jul 3, 2023
Merged

[Veeam] restored VMs without NICs #6282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
076ce8b
Fix bug where VMs restored from backups don't have NICs
Apr 18, 2022
44f3e0a
Address reviews
Apr 22, 2022
b87eeee
Merge 'main' into 'veeam-fix-restored-backup-vm-without-nics'
Apr 27, 2022
be140ee
Merge branch 'main' into veeam-fix-restored-backup-vm-without-nics
May 4, 2022
eb4b4ca
Mark removed NIC as non removed in restore process
May 4, 2022
4789669
Merge branch 'main' into veeam-fix-restored-backup-vm-without-nics
May 11, 2022
b72ef9c
Address reviews
May 11, 2022
ac0b9b5
Merge branch 'main' into veeam-fix-restored-backup-vm-without-nics
Jun 22, 2022
b388e38
Merge branch 'main' into 'veeam-fix-restored-backup-vm-without-nics'
Jul 27, 2022
b65abc1
Merge branch 'main' into veeam-fix-restored-backup-vm-without-nics
Aug 2, 2022
62ecef7
Fix compile errors
Aug 2, 2022
54d9e2e
Fix conflicts
Jan 16, 2023
14742bc
Merge branch 'main' into veeam-fix-restored-backup-vm-without-nics
Feb 21, 2023
1527032
Merge branch 'main' into veeam-fix-restored-backup-vm-without-nics
May 18, 2023
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
12 changes: 12 additions & 0 deletions engine/schema/src/main/java/com/cloud/vm/NicVO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@
import javax.persistence.Table;
import javax.persistence.Transient;

import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;

import com.cloud.network.Networks.AddressFormat;
import com.cloud.network.Networks.Mode;
import com.cloud.utils.db.GenericDao;
Expand Down Expand Up @@ -399,6 +402,15 @@ public void setNsxLogicalSwitchPortUuid(String nsxLogicalSwitchPortUuid) {
}

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Apr 20, 2022

Choose a reason for hiding this comment

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

@SadiJr
can you fix the issue in [VMwareGuru.java] ?

Copy link
Copy Markdown
Contributor Author

@SadiJr SadiJr Apr 22, 2022

Choose a reason for hiding this comment

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

Sorry, I don't understand. Can you please explain?

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Apr 28, 2022

Choose a reason for hiding this comment

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

@SadiJr
can you point out where two NicVO are compared in the source code ? is it possible to compare using id ?

one more question, if the nic is removed in cloudstack, then restore VM from veeam, will the nic be marked as not removed ?

Copy link
Copy Markdown
Contributor Author

@SadiJr SadiJr May 4, 2022

Choose a reason for hiding this comment

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

@weizhouapache

I'm sorry for the late reply. On line 909 of the VMWareGuru class, the remove method of a list object is called, passing a NicVO object as a reference. This is done to remove the NIC in question from the list of NICs to be removed from the VM (line 919); that is, NICs that are not in the list will be kept in the VM. It turns out that the NicVO object doesn't have the equals and hashcode methods overridden. Thus, the native method of Java is used, which uses the object reference to check if they are the same. With this, even if the NICs found in the database are the same discovered in VMware after a restore process, ACS will think they are different due to being two different object instances (objects of the same class), and will remove them of the VM.

About removed NICs, actually the NIC was not marked as not removed. I fixed that and thank you for pointing out this error.

PS: I don't know if I was able to express myself correctly (communication is not exactly one of my strong points), let me know if you don't understand something.

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache May 4, 2022

Choose a reason for hiding this comment

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

@SadiJr
my suggestion is, we could replace allNics with allNicIds which contains the id of the vm nics,

the other lines can be changed to

            if (nicVO != null) {
                allNicIds.remove(nicVO.getId());
            }

and

        for (final Long unMappedNicId : allNicIds) {
            vmManager.removeNicFromVm(vm, _nicDao.findById(unMappedNicId));
        }

Copy link
Copy Markdown
Contributor Author

@SadiJr SadiJr May 11, 2022

Choose a reason for hiding this comment

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

@weizhouapache yes, I can do that, but I think is better to override the equals and hashCode in NicVO, to make sure this error don't appears in other parts of code. What you think about?

Copy link
Copy Markdown
Member

@harikrishna-patnala harikrishna-patnala Jul 25, 2022

Choose a reason for hiding this comment

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

@SadiJr I get what you are saying and I'm up for both the solutions as you and @weizhouapache suggested.
My only concern with the equals method override is if it causes any regression in other places. To avoid such scenarios @weizhouapache 's solution makes it safer.

@weizhouapache any suggestions here!

Copy link
Copy Markdown
Contributor

@JoaoJandre JoaoJandre Aug 1, 2022

Choose a reason for hiding this comment

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

@harikrishna-patnala I don't see how this change could break other parts of the code, if two nicVO objects were being compared and they came out as equal before, they'll still continue doing so.

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Jan 16, 2023

Choose a reason for hiding this comment

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

I totally agree with what @harikrishna-patnala said.

this is a very large project with many features.
if there are two good options to fix a bug, it would be better to choose the one with less scope and impact.
If you will test all related features with different setup (hypervisors, network types and elements,etc), then go ahead with your solution, I am fine with it.

cc @SadiJr @JoaoJandre @DaanHoogland

@Override
public int hashCode() {
return new HashCodeBuilder(17, 31).append(id).toHashCode();
}

@Override
public boolean equals(Object obj) {
return EqualsBuilder.reflectionEquals(this, obj);
}

public Integer getMtu() {
return mtu;
}
Expand Down
2 changes: 2 additions & 0 deletions engine/schema/src/main/java/com/cloud/vm/dao/NicDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ public interface NicDao extends GenericDao<NicVO, Long> {

NicVO findByInstanceIdAndMacAddress(long instanceId, String macAddress);

NicVO findByNetworkIdAndMacAddressIncludingRemoved(long networkId, String mac);

List<NicVO> findNicsByIpv6GatewayIpv6CidrAndReserver(String ipv6Gateway, String ipv6Cidr, String reserverName);

NicVO findByIpAddressAndVmType(String ip, VirtualMachine.Type vmType);
Expand Down
8 changes: 8 additions & 0 deletions engine/schema/src/main/java/com/cloud/vm/dao/NicDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,14 @@ public NicVO findByNetworkIdAndMacAddress(long networkId, String mac) {
return findOneBy(sc);
}

@Override
public NicVO findByNetworkIdAndMacAddressIncludingRemoved(long networkId, String mac) {
SearchCriteria<NicVO> sc = AllFieldsSearch.create();
sc.setParameters("network", networkId);
sc.setParameters("macAddress", mac);
return findOneIncludingRemovedBy(sc);
Comment thread
SadiJr marked this conversation as resolved.
}

@Override
public NicVO findDefaultNicForVM(long instanceId) {
SearchCriteria<NicVO> sc = AllFieldsSearch.create();
Expand Down
17 changes: 14 additions & 3 deletions plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,8 +781,7 @@ private void syncVMVolumes(VMInstanceVO vmInstanceVO, List<VirtualDisk> virtualD
volume = createVolume(disk, vmToImport, domainId, zoneId, accountId, instanceId, poolId, templateId, backup, true);
operation = "created";
}
s_logger.debug(String.format("VM [id: %s, instanceName: %s] backup restore operation %s volume [id: %s].", instanceId, vmInstanceVO.getInstanceName(),
operation, volume.getUuid()));
s_logger.debug(String.format("Sync volumes to %s in backup restore operation: %s volume [id: %s].", vmInstanceVO, operation, volume.getUuid()));
}
}

Expand Down Expand Up @@ -879,9 +878,13 @@ private NetworkVO getGuestNetworkFromNetworkMorName(String name, long accountId,
String tag = parts[parts.length - 1];
String[] tagSplit = tag.split("-");
tag = tagSplit[tagSplit.length - 1];

s_logger.debug(String.format("Trying to find network with vlan: [%s].", vlan));
NetworkVO networkVO = networkDao.findByVlan(vlan);
if (networkVO == null) {
networkVO = createNetworkRecord(zoneId, tag, vlan, accountId, domainId);
s_logger.debug(String.format("Created new network record [id: %s] with details [zoneId: %s, tag: %s, vlan: %s, accountId: %s and domainId: %s].",
networkVO.getUuid(), zoneId, tag, vlan, accountId, domainId));
}
return networkVO;
}
Expand All @@ -893,6 +896,7 @@ private Map<String, NetworkVO> getNetworksMapping(String[] vmNetworkNames, long
Map<String, NetworkVO> mapping = new HashMap<>();
for (String networkName : vmNetworkNames) {
NetworkVO networkVO = getGuestNetworkFromNetworkMorName(networkName, accountId, zoneId, domainId);
s_logger.debug(String.format("Mapping network name [%s] to networkVO [id: %s].", networkName, networkVO.getUuid()));
mapping.put(networkName, networkVO);
}
return mapping;
Expand Down Expand Up @@ -927,12 +931,19 @@ private void syncVMNics(VirtualDevice[] nicDevices, DatacenterMO dcMo, Map<Strin
String macAddress = pair.first();
String networkName = pair.second();
NetworkVO networkVO = networksMapping.get(networkName);
NicVO nicVO = nicDao.findByNetworkIdAndMacAddress(networkVO.getId(), macAddress);
NicVO nicVO = nicDao.findByNetworkIdAndMacAddressIncludingRemoved(networkVO.getId(), macAddress);
if (nicVO != null) {
s_logger.warn(String.format("Find NIC in DB with networkId [%s] and MAC Address [%s], so this NIC will be removed from list of unmapped NICs of VM [id: %s, name: %s].",
networkVO.getId(), macAddress, vm.getUuid(), vm.getInstanceName()));
allNics.remove(nicVO);

if (nicVO.getRemoved() != null) {
nicDao.unremove(nicVO.getId());
}
}
}
for (final NicVO unMappedNic : allNics) {
s_logger.debug(String.format("Removing NIC [%s] from backup restored %s.", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(unMappedNic, "uuid", "macAddress"), vm));
vmManager.removeNicFromVm(vm, unMappedNic);
}
}
Expand Down