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

Remove SubnetPort/Pod Finalizer #792

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

yanjunz97
Copy link
Contributor

This PR removes the Finalizer in SubnetPort and Pod controller to provide a smooth
deletion experience for the user while ensure the stale NSX SubnetPort will be deleted
as expected.

Testing done:

  • Create a SubnetPort and delete it. Observed the deletion will not stuck and check
    in the log that corresponding NSX SubnetPort will be deleted by the controller
    when a delete event is detected.
  • Create a Pod and delete it. The deletion will still need a short period as we still have
    lifecycle-controller/system.vmware.com finalizer on the pod. But the log shows the
    corresponding NSX SubnetPort will be deleted by the controller when a delete event
    is detected.

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 17.69231% with 107 lines in your changes missing coverage. Please review.

Project coverage is 48.75%. Comparing base (762863a) to head (9b9bb82).

Files with missing lines Patch % Lines
pkg/nsx/services/subnetport/subnetport.go 0.00% 41 Missing ⚠️
pkg/controllers/pod/pod_controller.go 0.00% 36 Missing ⚠️
...kg/controllers/subnetport/subnetport_controller.go 56.09% 18 Missing ⚠️
pkg/nsx/services/subnetport/store.go 0.00% 12 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #792      +/-   ##
==========================================
- Coverage   49.05%   48.75%   -0.31%     
==========================================
  Files          94       94              
  Lines       12194    12245      +51     
==========================================
- Hits         5982     5970      -12     
- Misses       5709     5773      +64     
+ Partials      503      502       -1     
Flag Coverage Δ
unit-tests 48.75% <17.69%> (-0.31%) ⬇️
Files with missing lines Coverage Δ
pkg/nsx/services/common/types.go 100.00% <ø> (ø)
pkg/nsx/services/subnetport/builder.go 54.36% <ø> (+1.03%) ⬆️
pkg/nsx/services/subnetport/store.go 25.84% <0.00%> (-4.03%) ⬇️
...kg/controllers/subnetport/subnetport_controller.go 18.73% <56.09%> (-3.27%) ⬇️
pkg/controllers/pod/pod_controller.go 0.00% <0.00%> (ø)
pkg/nsx/services/subnetport/subnetport.go 5.80% <0.00%> (-0.86%) ⬇️

wenqiq
wenqiq previously approved these changes Oct 10, 2024
Copy link
Contributor

@wenqiq wenqiq left a comment

Choose a reason for hiding this comment

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

lgtm, not related to this PR, just curious about the podIsDeleted function.

func podIsDeleted(pod *v1.Pod) bool {
	return !pod.ObjectMeta.DeletionTimestamp.IsZero() || pod.Status.Phase == "Succeeded" || pod.Status.Phase == "Failed"
}

@yanjunz97
Copy link
Contributor Author

lgtm, not related to this PR, just curious about the podIsDeleted function.

func podIsDeleted(pod *v1.Pod) bool {
	return !pod.ObjectMeta.DeletionTimestamp.IsZero() || pod.Status.Phase == "Succeeded" || pod.Status.Phase == "Failed"
}

I think it's because when the pod is in phase Succeeded/Failed, it means the pod will never run again and thus the corresponding subnetport is no longer required. Then we can delete the nsx subnetport.
Ref to https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-phase

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

@yanjunz97
Copy link
Contributor Author

/e2e

@yanjunz97 yanjunz97 merged commit fa4dbe6 into vmware-tanzu:main Oct 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants