[Veeam] restored VMs without NICs#6282
Conversation
|
Found Java/XML changes, kicking packaging job |
PR Coverage Report
|
|
@blueorangutan package |
| public void setNsxLogicalSwitchPortUuid(String nsxLogicalSwitchPortUuid) { | ||
| this.nsxLogicalSwitchPortUuid = nsxLogicalSwitchPortUuid; | ||
| } | ||
|
|
There was a problem hiding this comment.
@SadiJr
can you fix the issue in [VMwareGuru.java] ?
There was a problem hiding this comment.
Sorry, I don't understand. Can you please explain?
There was a problem hiding this comment.
@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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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));
}
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
|
Found UI changes, kicking a new UI QA build |
|
@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build: ✔️ |
PR Coverage Report
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
SadiJr
force-pushed
the
veeam-fix-restored-backup-vm-without-nics
branch
from
1e58000 to
44f3e0a
Compare
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@DaanHoogland a 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: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5290 |
|
@weizhouapache @SadiJr are you clear on how we should proceed with this? |
|
@DaanHoogland sorry for the delay in answer, about the discussion of the usage of override of methods In addition, I will change this PR to draft, until this point of disagreement is resolved, thus avoiding an early merge. |
@SadiJr Anyway, this change should be ok. cc @DaanHoogland @harikrishna-patnala |
Sorry @SadiJr , I am not offended and I don ´t even recall the discussion. Though the subject you describe is a known cause for bugs, I don´t recall seeing this happening in cloudstack yet. |
|
SonarCloud Quality Gate failed. |
|
ping @harikrishna-patnala can you review this? Thanks. |
|
@rohityadavcloud 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: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6107 |
|
@blueorangutan test rocky8 vmware-72 |
|
@DaanHoogland [SF] unsupported parameters provided. Supported mgmt server os are: |
|
@blueorangutan test rocky8 vmware-70u2 |
|
@DaanHoogland a [SF] Trillian-Jenkins test job (rocky8 mgmt + vmware-70u2) has been kicked to run smoke tests |
|
Trillian Build Failed (tid-6570) |
|
@blueorangutan test rocky8 vmware-70u3 |
|
@DaanHoogland a [SF] Trillian-Jenkins test job (rocky8 mgmt + vmware-70u3) has been kicked to run smoke tests |
@DaanHoogland |
|
@weizhouapache [SF] unsupported parameters provided. Supported mgmt server os are: |
|
Trillian test result (tid-6572)
|
12 participants
Description
Using the VMWare hypervisor with Veeam integration enabled, restoring a VM from a backup removes the VM NICs. This happens because the NicVO object does not override the
equalsandhashCodemethods; thereforen, when ACS tries to sync the NICs of the restored VM with the NICs saved in the database, the comparison always returnsfalse, and ACS removes the VM NIC.Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
How Has This Been Tested?
It's tested in a local lab:
Also, I added more logs.