[VMware] Support for removal of NIC on IP disassociation on the VR by Pearl1594 · Pull Request #5985 · apache/cloudstack

Pearl1594 · 2022-02-11T12:28:47Z

Description

This PR fixes #5934

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

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

(1) create isolated network
(2) add new public IP range
(3) acquire public IP in new IP range
(4) create firewall rule. a new NIC is added to the network VR
(5) release the public IP. - if it is the only IP on the NIC, the nic will be removed

Test 1:
Initial output of ip a with default NIC:

Acquired an IP in another Public IP range and added a firewall rule: New nic was added

On deletion of the firewall rule on the NIC, the nic is removed:

Test2:

Add another Public IP range in the same VLAN as the one before
Acquire & add firewall rule for 2 IPs belonging to 2 different Public IP ranges but the same VLAN, such that there are 2 IPs on a NIC (check eth3)

Remove the rule from one of the IPs, and verify that the IP is disassociated from the VRs NIC, but the NIC isn't removed:

Pearl1594 · 2022-02-11T12:46:03Z

@blueorangutan package

blueorangutan · 2022-02-11T12:47:03Z

@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-02-11T13:23:44Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2578

Pearl1594 · 2022-02-11T13:24:37Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2022-02-11T13:25:03Z

@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

nvazquez · 2022-02-11T18:15:14Z

+            // the check and will try to find it within cluster
+            if (vmMo == null) {
+                if (hyperHost instanceof HostMO) {
+                    ClusterMO clusterMo = new ClusterMO(hyperHost.getContext(), ((HostMO) hyperHost).getParentMor());


What about datacenter instead of cluster search?

nvazquez · 2022-02-11T18:19:12Z

+                throw new Exception(msg);
+            }
+
+            for (IpAddressTO ip : ips) {


I think this for loop is not needed since we ensure the logic is executed only when it has one item as is not add. Could be refactored to pre-checks and logic to reduce indentation

weizhouapache · 2022-02-11T21:16:53Z

code works well. @Pearl1594
could you please retarget to 4.16 ?

blueorangutan · 2022-02-11T23:46:36Z

Trillian test result (tid-3298)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36156 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5985-t3298-vmware-67u3.zip
Smoke tests completed. 90 look OK, 2 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_offline_migrate_VM_and_root_volume	`Error`	101.04	test_vm_life_cycle.py
test_03_live_migrate_VM_with_two_data_disks	`Error`	61.35	test_vm_life_cycle.py
test_06_disk_offering_strictness_false	`Error`	142.03	test_service_offerings.py

Pearl1594 · 2022-02-14T06:12:33Z

@blueorangutan package

blueorangutan · 2022-02-14T06:13:03Z

@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Pearl1594 · 2022-02-14T06:42:05Z

@nvazquez I've addressed the comments. @weizhouapache I've rebased the branch off 4.16.

blueorangutan · 2022-02-14T06:54:07Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2597

Pearl1594 · 2022-02-14T06:55:05Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2022-02-14T06:56:02Z

@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

weizhouapache · 2022-02-14T09:44:06Z

@Pearl1594 @nvazquez @sureshanaparti
this PR needs to be tested/approved.
However 4.16.1.0 is close to be released, let's test/approve/merge this after 4.16.1.0 release, is that ok to you ?

DaanHoogland · 2022-02-14T09:52:35Z

agree @weizhouapache
in addition I wonder why this apparently generic feature requires such a vmware specific implementation. Isn't a more hypevisor independent implementation required?
I understand that it already works for Xen and KVM, so is there a way to fix it so it works for all, instead of adding ahypervisor specific code?

Pearl1594 · 2022-02-14T09:59:13Z

Sure @weizhouapache I am fine with moving to 4.17, I rebased it to 4.16, per a request made earlier : )
@DaanHoogland I am not sure if there is a hypervisor agnostic way to solve this, as each hypervisor has it's own way of handling hot unplugging of NICs.

blueorangutan · 2022-02-14T17:15:04Z

Trillian test result (tid-3322)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36003 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5985-t3322-vmware-67u3.zip
Smoke tests completed. 90 look OK, 2 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_offline_migrate_VM_and_root_volume	`Error`	94.64	test_vm_life_cycle.py
test_03_live_migrate_VM_with_two_data_disks	`Error`	58.53	test_vm_life_cycle.py
test_06_disk_offering_strictness_false	`Error`	143.41	test_service_offerings.py

weizhouapache · 2022-02-15T07:55:14Z

@Pearl1594
in my testing, if public ips are used for port forwarding or load balancer, everything seems ok.
If public ips are used for static nat, the interface will be removed even if there is still public ip in use (for example, pf or lb or nat). This issue happens with eth2 as well which is the interface for source nat.

steps to reproduce the issue
(1) acquire public IP in source NAT ip range or additional IP range
(2) enable static nat
(3) disbale static nat

expected result: interface is plugged when enable static nat if needed. interface is unplugged if IP is the last attached IP on the interface, when disable static nat.
actual result: interface is unplugged every time when disable static nat, no matter if it is the last IP.

I think we should use similar code like

            final String lastIp = cmd.getAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP);
            for (final IpAddressTO ip : ips) {
                if (!StringUtils.equalsIgnoreCase(lastIp, "true") || ip.isAdd()) {
                    continue;
                }

and

        if (publicNicInfo.first() == 2) {
            s_logger.debug("Do not remove eth2 in network VR because it is the public NIC of source NAT.");
            continue;
        }

Pearl1594 · 2022-02-15T09:10:20Z

Thanks for the review/testing @weizhouapache . I'll fix the issue.

…NIC)

Pearl1594 · 2022-02-16T07:07:08Z

@weizhouapache Could you please help verify if the fix addresses your comment. Thanks.

weizhouapache · 2022-02-16T14:21:28Z

@Pearl1594
the following code works in my tesing

            final String lastIp = cmd.getAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP);
            if (ips.length == 1 && !ips[0].isAdd()) {
                Pair<VirtualDevice, Integer> nicInfo = getVirtualDevice(vmMo, ips[0]);

                if (nicInfo.first() != null && nicInfo.second() != 2 && lastIp.equalsIgnoreCase("true")) {
                    configureNicDevice(vmMo, nicInfo.first(), VirtualDeviceConfigSpecOperation.REMOVE, "unplugNicCommand");
                }
            }

weizhouapache

did lots of testing, all seem fine.

weizhouapache · 2022-02-17T13:27:59Z

@blueorangutan package

blueorangutan · 2022-02-17T13:29:03Z

@weizhouapache a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-02-17T14:20:30Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2642

nvazquez · 2022-02-17T14:24:30Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2022-02-17T14:25:03Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

weizhouapache · 2022-02-17T14:35:08Z

@blueorangutan test centos7 vmware-70u3

blueorangutan · 2022-02-17T14:36:03Z

@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + vmware-70u3) has been kicked to run smoke tests

blueorangutan · 2022-02-18T01:04:06Z

Trillian test result (tid-3378)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37145 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5985-t3378-vmware-67u3.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · 2022-02-18T01:36:12Z

Trillian test result (tid-3382)
Environment: vmware-70u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38052 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5985-t3382-vmware-70u3.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez

LGTM

Pearl1594 requested review from nvazquez and weizhouapache February 11, 2022 12:29

Pearl1594 added this to the 4.17.0.0 milestone Feb 11, 2022

nvazquez added component:vmware type:bug labels Feb 11, 2022

nvazquez reviewed Feb 11, 2022

View reviewed changes

Pearl1594 force-pushed the fix-vmware-cleanup-nic branch from e4b9313 to a03e88e Compare February 14, 2022 06:41

Pearl1594 changed the base branch from main to 4.16 February 14, 2022 06:41

Pearl1594 modified the milestones: 4.17.0.0, 4.16.1.0 Feb 14, 2022

Pearl1594 force-pushed the fix-vmware-cleanup-nic branch from a03e88e to 5447b05 Compare February 14, 2022 10:01

Pearl1594 changed the base branch from 4.16 to main February 14, 2022 10:01

Pearl1594 force-pushed the fix-vmware-cleanup-nic branch from 5447b05 to 1f27c6a Compare February 14, 2022 10:03

Pearl1594 added the status:needs-testing label Feb 14, 2022

Pearl1594 marked this pull request as ready for review February 15, 2022 04:26

Pearl1594 marked this pull request as draft February 15, 2022 09:10

Pearl1594 added 3 commits February 16, 2022 10:13

[VMware] Support for removal of NIC on IP disassociation on the VR

c3d10c9

address comments - extract code + add appropriate operation name in logs

39a7499

Do not remove/unplug nic on removal of static nat on last IP (Public …

593f1b6

…NIC)

Pearl1594 force-pushed the fix-vmware-cleanup-nic branch from b20a106 to 593f1b6 Compare February 16, 2022 06:42

weizhouapache reviewed Feb 16, 2022

View reviewed changes

Comment thread ...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java

Comment thread ...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java

address comments

51a6429

weizhouapache reviewed Feb 17, 2022

View reviewed changes

Comment thread ...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java Outdated

address comment

3994ea6

Pearl1594 marked this pull request as ready for review February 17, 2022 11:24

weizhouapache approved these changes Feb 17, 2022

View reviewed changes

nvazquez approved these changes Feb 18, 2022

View reviewed changes

nvazquez merged commit 550c810 into apache:main Feb 18, 2022

DaanHoogland removed the status:needs-testing label Jan 28, 2026

Conversation

Pearl1594 commented Feb 11, 2022

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

Pearl1594 commented Feb 11, 2022

Uh oh!

blueorangutan commented Feb 11, 2022

Uh oh!

blueorangutan commented Feb 11, 2022

Uh oh!

Pearl1594 commented Feb 11, 2022

Uh oh!

blueorangutan commented Feb 11, 2022

Uh oh!

nvazquez Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

nvazquez Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueorangutan commented Feb 11, 2022

Uh oh!

Pearl1594 commented Feb 14, 2022

Uh oh!

blueorangutan commented Feb 14, 2022

Uh oh!

Pearl1594 commented Feb 14, 2022

Uh oh!

blueorangutan commented Feb 14, 2022

Uh oh!

Pearl1594 commented Feb 14, 2022

Uh oh!

blueorangutan commented Feb 14, 2022

Uh oh!

weizhouapache commented Feb 14, 2022

Uh oh!

DaanHoogland commented Feb 14, 2022

Uh oh!

Pearl1594 commented Feb 14, 2022

Uh oh!

blueorangutan commented Feb 14, 2022

Uh oh!

weizhouapache commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pearl1594 commented Feb 15, 2022

Uh oh!

Pearl1594 commented Feb 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weizhouapache commented Feb 16, 2022

Uh oh!

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Feb 17, 2022

Uh oh!

blueorangutan commented Feb 17, 2022

Uh oh!

blueorangutan commented Feb 17, 2022

Uh oh!

nvazquez commented Feb 17, 2022

Uh oh!

blueorangutan commented Feb 17, 2022

Uh oh!

weizhouapache commented Feb 17, 2022

Uh oh!

weizhouapache commented Feb 11, 2022 •

edited

Loading

weizhouapache commented Feb 15, 2022 •

edited

Loading