server: set Default NIC when VM has no default NIC by hsato03 · Pull Request #7859 · apache/cloudstack

hsato03 · 2023-08-11T19:56:53Z

Description

ACS allows you to add NICs to a VM without them, although it is not possible to set them as default since the updateDefaultNicForVirtualMachine API only updates the default NIC if a default NIC already exists, otherwise an exception is thrown.

For this reason, when a NIC is added and the VM that the NIC is being created for has no default NIC, the NIC will be marked as default.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Imported a VM without NICs, added some NICs and the first one was set as default.

codecov · 2023-08-12T22:57:15Z

Codecov Report

Merging #7859 (8f8fbe0) into 4.18 (b4032d9) will increase coverage by 0.04%.
Report is 85 commits behind head on 4.18.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               4.18    #7859      +/-   ##
============================================
+ Coverage     13.02%   13.06%   +0.04%     
- Complexity     9029     9108      +79     
============================================
  Files          2720     2720              
  Lines        257010   257548     +538     
  Branches      40083    40156      +73     
============================================
+ Hits          33467    33658     +191     
- Misses       219342   219660     +318     
- Partials       4201     4230      +29

Files	Coverage Δ
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	`7.51% <0.00%> (+0.15%)`	⬆️
...tack/engine/orchestration/NetworkOrchestrator.java	`6.04% <0.00%> (-0.02%)`	⬇️

... and 60 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kiranchavala · 2023-08-14T04:50:32Z

@blueorangutan package

blueorangutan · 2023-08-14T04:52:05Z

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

blueorangutan · 2023-08-14T05:45:58Z

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

weizhouapache · 2023-08-14T06:54:49Z

@hsato03
if you want this to be in 4.18.1.0, can you rebase with 4.18 ?

DaanHoogland

clgtm, but can you extract the new code into separate methods for readability please

hsato03 · 2023-08-14T19:09:56Z

@hsato03 if you want this to be in 4.18.1.0, can you rebase with 4.18 ?

@weizhouapache Yes, done.

DaanHoogland

clgtm

DaanHoogland · 2023-08-15T06:46:43Z

@blueorangutan package

blueorangutan · 2023-08-15T06:48:13Z

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

blueorangutan · 2023-08-15T07:55:50Z

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

weizhouapache · 2023-08-15T08:05:06Z

@hsato03
code lgtm

can you describe how to reproduce the issue the PR aim to fix ?

DaanHoogland · 2023-08-15T08:05:55Z

@blueorangutan test rocky8 vmware-70u3 keepEnv

blueorangutan · 2023-08-15T08:08:03Z

@DaanHoogland a [SF] Trillian-Jenkins test job (rocky8 mgmt + vmware-70u3) has been kicked to run smoke tests

hsato03 · 2023-08-15T18:18:45Z

@hsato03 code lgtm

can you describe how to reproduce the issue the PR aim to fix ?

@weizhouapache If an error occurs with the network adapter while importing a VM, it's necessary to remove the NIC via VCenter and create a new one in ACS. You can reproduce this by doing the following:

Using VMWare, create a VM and unmanage it;
On VCenter, go to the VM settings and remove all NICs from it;
Import the VM without NICs, create a new NIC for it and try to set the NIC as default.

blueorangutan · 2023-08-15T23:17:22Z

[SF] Trillian test result (tid-7420)
Environment: vmware-70u3 (x2), Advanced Networking with Mgmt server r8
Total time taken: 52641 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7859-t7420-vmware-70u3.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2023-08-17T14:40:05Z

@hsato03 , I am not allowed to import without NIC:

But I cannot select a network either as there is none to select.

I am trying your scenario from #7859 (comment)

hsato03 · 2023-08-29T22:12:01Z

@hsato03 , I am not allowed to import without NIC. But I cannot select a network either as there is none to select.

I am trying your scenario from #7859 (comment)

@DaanHoogland
Looks like you still have a NIC on your VM in VMware; this way, trying to import it without selecting the NIC makes ACS throw the exception.

The problem this PR intends to solve is that, when we have a VM without NIC and we add a NIC to it, we cannot set it as the default NIC. A way to reproduce this situation is importing a VM that does not have a NIC in VMware.
To reproduce the situation I did the following:

created a VM in ACS;
unmanaged the VM from ACS;
via vCenter, I removed the VM's NIC. This is what the VM looks like in vCenter:

Then, I imported the VM to ACS. This is the import form when a VM does not have NIC on vCenter (the Network adapter field is not presented):

With that, the VM is successfully imported to ACS, however, without NIC:

Without this patch, we can attach a NIC to the VM, however, it would not be the default NIC and we would not be able to set it as the default NIC; ACS returns the message Failed to find a nic profile for the existing default network. This is bad and probably means some sort of configuration corruption. With the patch, the first NIC of the VM is set as the default, and we can manage it afterwards.

@DaanHoogland, could you try following these steps?

Despite this PR solves the situation where we cannot set the default NIC of the VM (given the situation I mentioned), should ACS allow importing VMs without NICs, as ACS does not allow creating VMs without NIC neither remove the default NIC?. @DaanHoogland, what do you think? I can open the discussion in an issue or in the mailing list if valid.

weizhouapache

code lgtm

hsato03 · 2023-09-26T21:15:52Z

Hello @DaanHoogland,

Did you manage to reproduce the situation?

Regarding importing the VM without NICs, do you think it is valid to open a discussion or we should block it?

DaanHoogland · 2023-09-27T07:19:42Z

Did you manage to reproduce the situation?

I have not. I must admit I let other issues take precedence, however.

Regarding importing the VM without NICs, do you think it is valid to open a discussion or we should block it?

I think your question warrants a [DISCUSS] thread on dev@ . I am not against allowing it, but there may be some technical, procedural or plain historical objections. In that light it is always good to hide the behaviour of allowing VMs without NIC behind a global setting. A discuss thread should also resolve that question.

DaanHoogland · 2023-10-05T13:18:31Z

@hsato03 I have seen no significant discussions on your thread. Let's proceed. I would say a good implementation displays a warning asking to stop or create a default NIC. What do you think?

hsato03 · 2023-10-10T17:29:21Z

@DaanHoogland Good idea. I have added a warning message.

DaanHoogland · 2023-10-11T07:01:55Z

@blueorangutan package

blueorangutan · 2023-10-11T07:08:03Z

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

shwstppr · 2023-10-19T04:33:51Z

@blueorangutan package

blueorangutan · 2023-10-19T04:36:03Z

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

shwstppr

Code LGTM

blueorangutan · 2023-10-19T05:30:51Z

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

DaanHoogland · 2023-10-19T08:09:52Z

@blueorangutan test alma8 vmware-70u3

blueorangutan · 2023-10-19T08:12:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (alma8 mgmt + vmware-70u3) has been kicked to run smoke tests

blueorangutan · 2023-10-20T00:04:42Z

[SF] Trillian test result (tid-8010)
Environment: vmware-70u3 (x2), Advanced Networking with Mgmt server a8
Total time taken: 55421 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7859-t8010-vmware-70u3.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_create_template	`Error`	1800.76	test_templates.py

boring-cyborg Bot added the component:orchestration label Aug 11, 2023

DaanHoogland reviewed Aug 14, 2023

View reviewed changes

default first nic

7017eab

hsato03 force-pushed the default-first-nic branch from 6b4c1e3 to 7017eab Compare August 14, 2023 18:55

hsato03 changed the base branch from main to 4.18 August 14, 2023 18:59

DaanHoogland approved these changes Aug 15, 2023

View reviewed changes

weizhouapache added this to the 4.18.2.0 milestone Aug 25, 2023

weizhouapache approved these changes Aug 30, 2023

View reviewed changes

weizhouapache changed the title ~~Default NIC when VM has no default NIC~~ server: set Default NIC when VM has no default NIC Aug 30, 2023

DaanHoogland added the Severity:Minor label Oct 5, 2023

add warning message

8f8fbe0

boring-cyborg Bot added the component:UI label Oct 10, 2023

yadvr requested review from harikrishna-patnala and shwstppr October 16, 2023 08:05

shwstppr approved these changes Oct 19, 2023

View reviewed changes

DaanHoogland merged commit a8700bf into apache:4.18 Oct 20, 2023

Uh oh!

Conversation

hsato03 commented Aug 11, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

codecov Bot commented Aug 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kiranchavala commented Aug 14, 2023

Uh oh!

blueorangutan commented Aug 14, 2023

Uh oh!

blueorangutan commented Aug 14, 2023

Uh oh!

weizhouapache commented Aug 14, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

hsato03 commented Aug 14, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

weizhouapache commented Aug 15, 2023

Uh oh!

DaanHoogland commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

hsato03 commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

DaanHoogland commented Aug 17, 2023

Uh oh!

hsato03 commented Aug 29, 2023

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

hsato03 commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland commented Sep 27, 2023

Uh oh!

DaanHoogland commented Oct 5, 2023

Uh oh!

hsato03 commented Oct 10, 2023

Uh oh!

DaanHoogland commented Oct 11, 2023

Uh oh!

blueorangutan commented Oct 11, 2023

Uh oh!

shwstppr commented Oct 19, 2023

Uh oh!

blueorangutan commented Oct 19, 2023

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Oct 19, 2023

Uh oh!

DaanHoogland commented Oct 19, 2023

Uh oh!

blueorangutan commented Oct 19, 2023

codecov Bot commented Aug 12, 2023 •

edited

Loading

hsato03 commented Aug 15, 2023 •

edited

Loading

hsato03 commented Sep 26, 2023 •

edited

Loading