prevent nic removal on out of bounds router stop by DaanHoogland · Pull Request #8371 · apache/cloudstack

DaanHoogland · 2023-12-18T15:19:21Z

Description

This PR...

Fixes: #8284

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)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov · 2023-12-18T17:31:34Z

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (1411da1) 30.88% compared to head (c51d5ce) 30.94%.
Report is 88 commits behind head on 4.19.

Files	Patch %	Lines
...ork/router/VirtualNetworkApplianceManagerImpl.java	42.85%	9 Missing and 3 partials ⚠️
...ava/com/cloud/network/guru/ControlNetworkGuru.java	14.28%	6 Missing ⚠️
...tack/engine/orchestration/NetworkOrchestrator.java	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.19    #8371      +/-   ##
============================================
+ Coverage     30.88%   30.94%   +0.06%     
- Complexity    34079    34232     +153     
============================================
  Files          5341     5347       +6     
  Lines        374861   375574     +713     
  Branches      54518    54629     +111     
============================================
+ Hits         115769   116220     +451     
- Misses       243825   244046     +221     
- Partials      15267    15308      +41

Flag	Coverage Δ
simulator-marvin-tests	`24.83% <67.21%> (+0.01%)`	⬆️
uitests	`4.39% <ø> (-0.01%)`	⬇️
unit-tests	`16.55% <40.98%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

DaanHoogland · 2023-12-18T21:26:24Z

@blueorangutan package

blueorangutan · 2023-12-18T21:28:03Z

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

blueorangutan · 2023-12-18T22:35:22Z

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

weizhouapache · 2023-12-18T23:05:36Z

This code snippet seems not related to private ip release.

blueorangutan · 2023-12-19T12:09:35Z

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

blueorangutan · 2023-12-19T16:10:41Z

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

blueorangutan · 2023-12-20T09:31:21Z

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

blueorangutan · 2023-12-20T16:26:31Z

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

weizhouapache

The title needs to be updated

ip release, not nic removal
it is not related to out-of-band stop. currently control ip is released even if vr is stopped by cloudstack api. If VR is started out of band, but the previous used / released control ip is allocated to other VRs, there will be some issues.

DaanHoogland · 2023-12-21T10:01:16Z

just noticed that the client vlan is being removed as well. I think this must be fixed as well @weizhouapache @andrijapanicsb .

weizhouapache · 2023-12-21T10:17:15Z

just noticed that the client vlan is being removed as well. I think this must be fixed as well @weizhouapache @andrijapanicsb .

@DaanHoogland
I think the vlan is released when isolated network is being shutdown (not during stopping of VR)
the whole process of the issue we faced is

(due to whatever reason) number of active nics is 0 (incorrect)
ACS detects the active nics is 0 and tries to shutdown the network as it is not persistent
stop the VR as part of network shutdown. (the control IP is VR is released)
release the vlan

if only VR is stopped, the vlan is not released.

weizhouapache · 2023-12-21T11:08:39Z

just noticed that the client vlan is being removed as well. I think this must be fixed as well @weizhouapache @andrijapanicsb .

@DaanHoogland I think the vlan is released when isolated network is being shutdown (not during stopping of VR) the whole process of the issue we faced is

(due to whatever reason) number of active nics is 0 (incorrect)

ACS detects the active nics is 0 and tries to shutdown the network as it is not persistent

stop the VR as part of network shutdown. (the control IP is VR is released)

release the vlan

if only VR is stopped, the vlan is not released.

ignore this. what @DaanHoogland mentioned is, the vlan of VR nics is set to empty, when VR is stopped (by CloudStack API/UI or OOB)

DaanHoogland · 2023-12-21T11:11:34Z

ignore this. what @DaanHoogland mentioned is, the vlan of VR nics is set to empty, when VR is stopped (by CloudStack API/UI or OOB)

thanks @weizhouapache , we also discussed and agreed that this does need fixing (in the scope of this fix). The more general issue (might become) handling of OOB actions in genenral and OOB DRS specifically.

I will wrap/clean up the PR

yadvr · 2024-02-13T05:36:14Z

@blueorangutan package

blueorangutan · 2024-02-13T05:38:05Z

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

blueorangutan · 2024-02-13T06:44:32Z

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

DaanHoogland · 2024-02-13T09:46:31Z

@blueorangutan test

blueorangutan · 2024-02-13T09:48:02Z

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

DaanHoogland · 2024-02-13T13:23:58Z

have a look @weizhouapache . should implement what you suggested.

blueorangutan · 2024-02-14T00:02:15Z

[SF] Trillian test result (tid-9190)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 49755 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8371-t9190-kvm-centos7.zip
Smoke tests completed. 128 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_08_upgrade_kubernetes_ha_cluster	`Failure`	3678.17	test_kubernetes_clusters.py

yadvr · 2024-02-15T10:29:02Z

@blueorangutan package

blueorangutan · 2024-02-15T10:30:04Z

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

weizhouapache

@DaanHoogland
overall lgtm

left some comments

blueorangutan · 2024-02-15T11:34:30Z

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

Co-authored-by: Wei Zhou <weizhou@apache.org>

DaanHoogland · 2024-02-15T11:53:18Z

@blueorangutan package

blueorangutan · 2024-02-15T11:54:03Z

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

blueorangutan · 2024-02-15T12:59:23Z

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

DaanHoogland · 2024-02-15T14:04:33Z

@blueorangutan test

blueorangutan · 2024-02-15T14:06:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

weizhouapache

code lgtm

blueorangutan · 2024-02-16T09:35:59Z

[SF] Trillian test result (tid-9237)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43139 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8371-t9237-kvm-centos7.zip
Smoke tests completed. 129 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

borisstoyanov

LGTM, code review

Co-authored-by: Vishesh <vishesh92@gmail.com> Co-authored-by: Wei Zhou <weizhou@apache.org>

boring-cyborg Bot added the component:networking label Dec 18, 2023

DaanHoogland changed the title ~~move nic removal from shutdown to expunge~~ prevent nic removal on out of bounds router stop Dec 19, 2023

boring-cyborg Bot added the component:orchestration label Dec 19, 2023

DaanHoogland force-pushed the destroyNicOnVRDestroy branch from 4213673 to b61b4e9 Compare December 19, 2023 14:59

boring-cyborg Bot added the component:api label Dec 20, 2023

DaanHoogland added 7 commits December 20, 2023 08:33

move nic removal from shutdown to expunge

6b2e992

make nic removals conditional

d2d1072

clean injections from VirtualNetworkApplianceManagerImpl

75a3659

allow serialising NaN

7062c7f

log and exception messages

b247e68

allign string representation of nic and nicProfile

baf4ec9

add condition on releasing of nic

a5d8a72

DaanHoogland force-pushed the destroyNicOnVRDestroy branch from 94eacbd to a5d8a72 Compare December 20, 2023 07:33

weizhouapache reviewed Dec 20, 2023

View reviewed changes

Comment thread ...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java Outdated

move conditional to ControlNetworkGuru

7945582

weizhouapache reviewed Dec 20, 2023

View reviewed changes

DaanHoogland mentioned this pull request Dec 21, 2023

Better handling for outband power operations on VR #8284

Closed

DaanHoogland added the status:ready-for-merge label Feb 6, 2024

weizhouapache reviewed Feb 13, 2024

View reviewed changes

DaanHoogland and others added 2 commits February 13, 2024 14:21

pull up DC retrieval

3076520

Apply suggestions from code review

ea9c32c

weizhouapache reviewed Feb 15, 2024

View reviewed changes

Apply suggestions from code review

3cd46ee

DaanHoogland and others added 2 commits February 15, 2024 12:51

extract pvland rules method

c465e2f

Apply suggestions from code review

c51d5ce

Co-authored-by: Wei Zhou <weizhou@apache.org>

weizhouapache approved these changes Feb 15, 2024

View reviewed changes

borisstoyanov approved these changes Feb 16, 2024

View reviewed changes

DaanHoogland merged commit a0e592e into apache:4.19 Feb 16, 2024

DaanHoogland deleted the destroyNicOnVRDestroy branch February 16, 2024 13:33

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Feb 23, 2024

prevent nic removal on out of bounds router stop (apache#8371)

ece1cfb

Co-authored-by: Vishesh <vishesh92@gmail.com> Co-authored-by: Wei Zhou <weizhou@apache.org>

DaanHoogland removed the status:ready-for-merge label Dec 4, 2024

Conversation

DaanHoogland commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Dec 18, 2023

Uh oh!

blueorangutan commented Dec 18, 2023

Uh oh!

blueorangutan commented Dec 18, 2023

Uh oh!

weizhouapache commented Dec 18, 2023

Uh oh!

blueorangutan commented Dec 19, 2023

Uh oh!

blueorangutan commented Dec 19, 2023

Uh oh!

blueorangutan commented Dec 20, 2023

Uh oh!

Uh oh!

blueorangutan commented Dec 20, 2023

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Dec 21, 2023

Uh oh!

weizhouapache commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weizhouapache commented Dec 21, 2023

Uh oh!

DaanHoogland commented Dec 21, 2023

Uh oh!

yadvr commented Feb 13, 2024

Uh oh!

blueorangutan commented Feb 13, 2024

Uh oh!

blueorangutan commented Feb 13, 2024

Uh oh!

DaanHoogland commented Feb 13, 2024

Uh oh!

blueorangutan commented Feb 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Feb 13, 2024

Uh oh!

blueorangutan commented Feb 14, 2024

Uh oh!

yadvr commented Feb 15, 2024

Uh oh!

blueorangutan commented Feb 15, 2024

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Dec 18, 2023 •

edited

Loading

codecov Bot commented Dec 18, 2023 •

edited

Loading

weizhouapache commented Dec 21, 2023 •

edited

Loading