Skip to content
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.

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

Fix the issue that NSX VPC not deleted when the Namespace is terminating #948

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

wenyingd
Copy link
Contributor

@wenyingd wenyingd commented Dec 6, 2024

This change is to fix an issue with the NetworkInfo deletion logic that NSX VPC is not removed if the CR's Namesapce still exists.

The fix is to filter out the Namespace if it is already mark for deletion when a NetworkInfo CR is deleted.

Test Done:
With this fix, after the e2e is completed, the new created VPC by the test cases are deleted in time.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.37%. Comparing base (7c783d4) to head (a04f5c4).

Files with missing lines Patch % Lines
pkg/controllers/networkinfo/networkinfo_utils.go 93.75% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #948      +/-   ##
==========================================
- Coverage   73.43%   73.37%   -0.06%     
==========================================
  Files         118      118              
  Lines       16414    16397      -17     
==========================================
- Hits        12054    12032      -22     
- Misses       3575     3581       +6     
+ Partials      785      784       -1     
Flag Coverage Δ
unit-tests 73.37% <97.36%> (-0.06%) ⬇️
Files with missing lines Coverage Δ
.../controllers/networkinfo/networkinfo_controller.go 68.33% <100.00%> (+0.51%) ⬆️
pkg/nsx/services/vpc/vpc.go 55.08% <100.00%> (-1.09%) ⬇️
pkg/controllers/networkinfo/networkinfo_utils.go 78.09% <93.75%> (-0.68%) ⬇️

@wenyingd wenyingd force-pushed the bugfix_vpc_deletion branch 2 times, most recently from 67536db to 89fe570 Compare December 6, 2024 11:56
@wenyingd
Copy link
Contributor Author

wenyingd commented Dec 6, 2024

/e2e

2 similar comments
@wenyingd
Copy link
Contributor Author

wenyingd commented Dec 6, 2024

/e2e

@wenyingd
Copy link
Contributor Author

wenyingd commented Dec 9, 2024

/e2e

@wenyingd wenyingd force-pushed the bugfix_vpc_deletion branch from 89fe570 to b55a016 Compare December 9, 2024 08:21
@wenyingd
Copy link
Contributor Author

wenyingd commented Dec 9, 2024

/e2e

@wenyingd wenyingd force-pushed the bugfix_vpc_deletion branch 4 times, most recently from 2a87f1d to b71c919 Compare December 11, 2024 02:41
@wenyingd
Copy link
Contributor Author

/e2e

@wenyingd wenyingd force-pushed the bugfix_vpc_deletion branch 3 times, most recently from c693177 to 7caf932 Compare December 12, 2024 02:55
@wenyingd
Copy link
Contributor Author

/e2e

@wenqiq
Copy link
Contributor

wenqiq commented Dec 12, 2024

This change is intended to consider Namespaces in the terminating state when deleting stale VPCs, rather than ignore them, correct? That is, VPCs under a Namespace in the terminating state should be deleted.

@wenyingd wenyingd changed the title Ignore the terminating Namespace when deleting NSX VPC by NS name Delete NSX VPC when the Namespace is terminating Dec 12, 2024
@wenyingd wenyingd force-pushed the bugfix_vpc_deletion branch from 7caf932 to ba216d7 Compare December 12, 2024 10:15
@wenyingd
Copy link
Contributor Author

This change is intended to consider Namespaces in the terminating state when deleting stale VPCs, rather than ignore them, correct? That is, VPCs under a Namespace in the terminating state should be deleted.

Yes, updated the PR title.

zhengxiexie
zhengxiexie previously approved these changes Dec 13, 2024
Copy link
Contributor

@zhengxiexie zhengxiexie left a comment

Choose a reason for hiding this comment

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

lgtm

This change is to fix an issue with the NetworkInfo deletion logic that NSX VPC
is not removed if the CR's Namesapce still exists.

The fix is to add a wather on Namespace deletion event and ensure the NSX VPC is
deleted when the K8s Namespace is deleted.
@wenyingd wenyingd force-pushed the bugfix_vpc_deletion branch from a04f5c4 to 52387a1 Compare December 13, 2024 15:32
@wenyingd wenyingd changed the title Delete NSX VPC when the Namespace is terminating Fix the issue that NSX VPC not deleted when the Namespace is terminating Dec 13, 2024
Copy link
Contributor

@timdengyun timdengyun left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Copy link
Contributor

@lxiaopei lxiaopei left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyingd wenyingd merged commit 652107c into vmware-tanzu:main Dec 13, 2024
2 checks passed
wenyingd added a commit to wenyingd/nsx-operator that referenced this pull request Dec 13, 2024
…ing (vmware-tanzu#948)

* Fix issues with NetworkInfo reconciler that not delete NSX VPC in time

This change is to fix an issue with the NetworkInfo deletion logic that NSX VPC
is not removed if the CR's Namesapce still exists.

The fix is to add a wather on Namespace deletion event and ensure the NSX VPC is
deleted when the K8s Namespace is deleted.

* Filter out the Namespace which is in terminating state
wenyingd added a commit that referenced this pull request Dec 13, 2024
…ing (#948) (#969)

This change is to fix an issue with the NetworkInfo deletion logic that NSX VPC
is not removed if the CR's Namesapce still exists.

The fix is to add a wather on Namespace deletion event and ensure the NSX VPC is
deleted when the K8s Namespace is deleted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants