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.

Sign up for GitHub

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

Jump to bottom

Remove Subnet/SubnetSet Finalizer #769

Merged
merged 1 commit into from
Oct 11, 2024
Merged

Remove Subnet/SubnetSet Finalizer #769

merged 1 commit into from
Oct 11, 2024

Conversation

wenqiq
Copy link
Contributor

@wenqiq wenqiq commented Sep 19, 2024 •
edited
Loading

This PR tries to fix the issue that Subnet CR deletion takes about 2 minutes (due to the NSX IP release issue), resulting in a prolonged Namespace deletion time.
Remove NetworkInfo Finalizer in #778

TestDone:

  1. Create a Subnet and check its status; update the Subnet and check its status; delete the Subnet and check for any stale Subnet in NSX.(included in e2e test cases)
  2. Create a SubnetSet and check its status; update the SubnetSet and check its status; delete the SubnetSet and check for any stale Subnet in NSX.(included in e2e test cases)
  3. Rebase Remove NetworkInfo Finalizer #778 and test the deletion of a Namespace, then delete the Pod, the Namespace will not stuck in terminating status.

@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch 2 times, most recently from b69b21e to 1652a33 Compare September 20, 2024 10:44
@wenqiq wenqiq marked this pull request as ready for review September 23, 2024 01:30
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch 2 times, most recently from 7c8d783 to 2fb5e5e Compare September 24, 2024 07:05
@wenqiq wenqiq changed the title Remove Subnet Finalizer Remove Subnet/SubnetSet Finalizer Sep 24, 2024
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch from 927826a to aa23e65 Compare September 24, 2024 15:47
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch from aa23e65 to 251eb16 Compare September 26, 2024 03:01
yanjunz97
yanjunz97 reviewed Sep 26, 2024
View reviewed changes
pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
pkg/controllers/common/utils.go Outdated Show resolved Hide resolved
pkg/controllers/subnetset/subnetset_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnetset/subnetset_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnetset/subnetset_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnetset/subnetset_controller.go Outdated Show resolved Hide resolved
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch 2 times, most recently from a74cabd to 0fbef86 Compare September 28, 2024 20:41
yanjunz97
yanjunz97 reviewed Sep 29, 2024
View reviewed changes
Copy link
Contributor

@yanjunz97 yanjunz97 left a comment

Choose a reason for hiding this comment

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

Would be better if adding some testing done in the description.

By the way, would you know why the coverage report link does not show up in this PR?

pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnetset/subnetset_controller.go Outdated Show resolved Hide resolved
pkg/nsx/services/subnet/subnet_test.go Outdated Show resolved Hide resolved
pkg/nsx/services/subnet/subnet_test.go Outdated Show resolved Hide resolved
@yanjunz97
Copy link
Contributor

Also remove the unused variable?

nsx-operator/pkg/nsx/services/common/types.go

Lines 110 to 111 in c1ef61a

SubnetFinalizerName = "subnet.crd.nsx.vmware.com/finalizer"
SubnetSetFinalizerName = "subnetset.crd.nsx.vmware.com/finalizer"

timdengyun
timdengyun reviewed Sep 30, 2024
View reviewed changes
pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnet/subnet_controller.go Show resolved Hide resolved
pkg/controllers/subnet/subnet_controller.go Show resolved Hide resolved
pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch 2 times, most recently from da8e495 to 633087f Compare October 1, 2024 12:57
yanjunz97
yanjunz97 reviewed Oct 2, 2024
View reviewed changes
Copy link
Contributor

@yanjunz97 yanjunz97 left a comment

Choose a reason for hiding this comment

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

Add 2 more nits

pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch from 9ffd7e6 to f25edd1 Compare October 8, 2024 09:29
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch 3 times, most recently from 4b449d4 to 2cc98aa Compare October 9, 2024 13:29
@wenqiq
Copy link
Contributor Author

wenqiq commented Oct 10, 2024

/e2e

yanjunz97
yanjunz97 reviewed Oct 10, 2024
View reviewed changes
pkg/controllers/subnet/subnet_controller.go Outdated Show resolved Hide resolved
pkg/nsx/services/subnet/subnet.go Outdated Show resolved Hide resolved
pkg/controllers/subnetset/subnetset_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnetset/subnetset_controller.go Outdated Show resolved Hide resolved
pkg/controllers/subnetset/subnetset_controller.go Show resolved Hide resolved
pkg/controllers/subnetset/subnetset_controller.go Outdated Show resolved Hide resolved
yanjunz97
yanjunz97 reviewed Oct 10, 2024
View reviewed changes
Copy link
Contributor

@yanjunz97 yanjunz97 left a comment

Choose a reason for hiding this comment

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

Find one more comment

pkg/nsx/services/subnet/subnet.go Outdated Show resolved Hide resolved
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch from f5ac307 to 1629917 Compare October 10, 2024 07:45
yanjunz97
yanjunz97 reviewed Oct 10, 2024
View reviewed changes
Copy link
Contributor

@yanjunz97 yanjunz97 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, only 1 nit

pkg/nsx/services/subnet/store.go Outdated Show resolved Hide resolved
@wenqiq
Remove Subnet Finalizer
0f72d32 
Remove SubnetSet Finalizer
Add unit-test for Subnet controller

Signed-off-by: Wenqi Qiu <[email protected]>

Signed-off-by: Wenqi Qiu <[email protected]>
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch from c390e0d to 0f72d32 Compare October 10, 2024 11:30
zhengxiexie
zhengxiexie approved these changes Oct 11, 2024
View reviewed changes
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

@wenqiq wenqiq merged commit 762863a into vmware-tanzu:main Oct 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dantingl dantingl dantingl left review comments

@timdengyun timdengyun timdengyun left review comments

@zhengxiexie zhengxiexie zhengxiexie approved these changes

@yanjunz97 yanjunz97 yanjunz97 approved these changes

@TaoZou1 TaoZou1 Awaiting requested review from TaoZou1

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wenqiq @codecov-commenter @yanjunz97 @dantingl @zhengxiexie @timdengyun @vmwclabot